Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#130: Add default content length to progress bar #286

Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a6799be
Added default content length to progress bar
ndemirca Apr 12, 2024
be68571
added test data
ndemirca Apr 12, 2024
82a521f
Finished bug fix
ndemirca Apr 12, 2024
72b90ef
improved unittests
ndemirca Apr 12, 2024
7c81d4a
removed outdated JUnit dependency
ndemirca Apr 12, 2024
2651706
used wiremock for the unit tests
ndemirca Apr 15, 2024
301b5bb
Implemented change requests
ndemirca Apr 16, 2024
483311d
added improvements
ndemirca Apr 16, 2024
c4e6d0a
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 16, 2024
a95ac8d
implemented change requests
ndemirca Apr 17, 2024
1729d2e
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 22, 2024
0ad0b7b
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 23, 2024
356f7ec
Merge branch 'bug/130-Add-Default-Content-Length-To-ProgressBar' of h…
ndemirca Apr 23, 2024
9886857
line ending changes
ndemirca May 15, 2024
368f3a1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca May 15, 2024
2526187
fixed compile issue
ndemirca May 15, 2024
d931dff
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca May 17, 2024
7ac7fa4
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Jul 8, 2024
f7205aa
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Jul 17, 2024
1ecce2b
#130: cleanup
jan-vcapgemini Jul 17, 2024
fc4b554
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 9, 2024
50d38c4
#130: fixed tests and PluginInstaller
jan-vcapgemini Aug 9, 2024
8dccee1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 28, 2024
acf8e96
#130: refactored progress bar
jan-vcapgemini Aug 30, 2024
ba27c39
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 30, 2024
f3ccb0a
Update cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgress…
jan-vcapgemini Sep 2, 2024
df01ffe
Update cli/src/main/java/com/devonfw/tools/ide/io/IdeProgressBarConso…
jan-vcapgemini Sep 2, 2024
5a146b1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Sep 2, 2024
c13439d
#130: implemented requested changes
jan-vcapgemini Sep 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ public interface FileAccess {
* Downloads a file from an arbitrary location.
*
* @param url the location of the binary file to download. May also be a local or remote path to copy from.
* @param targetFile the {@link Path} to the target file to download to. Should not already exists. Missing parent
* directories will be created automatically.
* @param targetFile the {@link Path} to the target file to download to. Should not already exists. Missing parent directories will be created automatically.
* @param test the flag about calling this method from test classes.
*/
void download(String url, Path targetFile);
void download(String url, Path targetFile, boolean test);

/**
* Creates the entire {@link Path} as directories if not already existing.
Expand Down
56 changes: 45 additions & 11 deletions cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ public class FileAccessImpl implements FileAccess {
/** The {@link HttpClient} for HTTP requests. */
private final HttpClient client;

/** The default value for missing content length */
public static final long DEFAULT_CONTENT_LENGTH = 10000000L;

/**
* The constructor.
*
Expand All @@ -69,7 +72,7 @@ public FileAccessImpl(IdeContext context) {
}

@Override
public void download(String url, Path target) {
public void download(String url, Path target, boolean test) {

this.context.info("Trying to download {} from {}", target.getFileName(), url);
mkdirs(target.getParent());
Expand All @@ -88,7 +91,7 @@ public void download(String url, Path target) {
Path source = Path.of(url);
if (isFile(source)) {
// network drive
copyFileWithProgressBar(source, target);
copyFileWithProgressBar(source, target, test);
} else {
throw new IllegalArgumentException("Download path does not point to a downloadable file: " + url);
}
Expand All @@ -105,13 +108,10 @@ public void download(String url, Path target) {
* @param target Path of the target directory.
* @param response the {@link HttpResponse} to use.
*/
private void downloadFileWithProgressBar(String url, Path target, HttpResponse<InputStream> response) throws IOException {
private void downloadFileWithProgressBar(String url, Path target, HttpResponse<InputStream> response) {

long contentLength = response.headers().firstValueAsLong("content-length").orElse(0);
if (contentLength == 0) {
this.context.warning("Content-Length was not provided by download source : {} using fallback for the progress bar which will be inaccurate.", url);
contentLength = 10000000;
}
informAboutSettingDefaultContentLength(contentLength, url, null);

byte[] data = new byte[1024];
boolean fileComplete = false;
Expand All @@ -127,9 +127,12 @@ private void downloadFileWithProgressBar(String url, Path target, HttpResponse<I
fileComplete = true;
} else {
bufferedOut.write(data, 0, count);
pb.stepBy(count);
if (contentLength > 0) {
pb.stepBy(count);
}
}
}
callStepByWithDefaultContentLength(contentLength, pb);
} catch (Exception e) {
throw new RuntimeException(e);
}
Expand All @@ -140,26 +143,57 @@ private void downloadFileWithProgressBar(String url, Path target, HttpResponse<I
*
* @param source Path of file to copy
* @param target Path of target directory
* @param test the flag about calling this method from test classes.
*/
private void copyFileWithProgressBar(Path source, Path target) throws IOException {
private void copyFileWithProgressBar(Path source, Path target, boolean test) throws IOException {

try (InputStream in = new FileInputStream(source.toFile()); OutputStream out = new FileOutputStream(target.toFile())) {
long size;
if (test) {
size = 0L;
} else {
size = source.toFile().length();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need to test this somehow, extract to protected method long getFileSize(Path) create a subclass of FileAccessImpl for test that overrides and returns 0 instead. Then in your JUnit replace the real FileAccess with the test instance... Seems a little bit more complicated but adding this test flag to our official API seems odd to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented this as you've suggested, thanks.


long size = source.toFile().length();
informAboutSettingDefaultContentLength(size, null, source);
byte[] buf = new byte[1024];
int readBytes;

try (IdeProgressBar pb = this.context.prepareProgressBar("Copying", size)) {
while ((readBytes = in.read(buf)) > 0) {
out.write(buf, 0, readBytes);
pb.stepByOne();
if (size > 0) {
pb.stepByOne();
}
}
callStepByWithDefaultContentLength(size, pb);
} catch (Exception e) {
throw new RuntimeException(e);
}
}
}

private void callStepByWithDefaultContentLength(long contentLength, IdeProgressBar progressBar) {

if (contentLength == 0) {
progressBar.stepBy(DEFAULT_CONTENT_LENGTH);
}
}

private void informAboutSettingDefaultContentLength(long contentLength, String url, Path path) {

String source;
if (contentLength == 0) {
Copy link
Member

@hohwille hohwille Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you using 0 for undefined? IMHO -1 would make more sense also here...

Files can be 0 bytes large and then the user should not get this warning for every file that was empty but gets copied.
Also if the file size is know but smaller than e.g. 1k we should not give a progress bar.
What is good is that we have a dedicated method copyFileWithProgressBar for this use-case so we do not do this for every file. However, I assume this is used when we get a tool release package from a custom tool repository using a file URL and that can also be a network mount that is slower so already copying 1MB could take some seconds...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've set the default for undefined to -1 now.

if (path != null) {
source = path.toString();
} else {
source = url;
}
this.context.warning("Content-Length was not provided by download/copy source: {}. Using fallback: Content-Length for the progress bar is set to {}.",
source, DEFAULT_CONTENT_LENGTH);
}
}

@Override
public void mkdirs(Path directory) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ protected Path download(String url, Path target, String downloadFilename, Versio

Path tmpDownloadFile = createTempDownload(downloadFilename);
try {
this.context.getFileAccess().download(url, tmpDownloadFile);
this.context.getFileAccess().download(url, tmpDownloadFile, false);
if (resolvedVersion.toString().equals("latest")) {
// Some software vendors violate best-practices and provide the latest version only under a fixed URL.
// Therefore if a newer version of that file gets released, the same URL suddenly leads to a different
Expand Down
5 changes: 2 additions & 3 deletions cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@ public boolean install(boolean silent) {
@Override
public void installPlugin(PluginDescriptor plugin) {

Path mavenPlugin = this.context.getSoftwarePath().resolve(this.tool)
.resolve("lib/ext/" + plugin.getName() + ".jar");
this.context.getFileAccess().download(plugin.getUrl(), mavenPlugin);
Path mavenPlugin = this.context.getSoftwarePath().resolve(this.tool).resolve("lib/ext/" + plugin.getName() + ".jar");
this.context.getFileAccess().download(plugin.getUrl(), mavenPlugin, false);
if (Files.exists(mavenPlugin)) {
this.context.success("Successfully added {} to {}", plugin.getName(), mavenPlugin.toString());
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
package com.devonfw.tools.ide.context;

import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

import com.devonfw.tools.ide.io.FileAccess;
import com.devonfw.tools.ide.io.IdeProgressBar;
import com.devonfw.tools.ide.io.IdeProgressBarTestImpl;
Expand All @@ -14,6 +9,13 @@
import com.devonfw.tools.ide.repo.DefaultToolRepository;
import com.devonfw.tools.ide.repo.ToolRepository;

import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

import static com.devonfw.tools.ide.io.FileAccessImpl.DEFAULT_CONTENT_LENGTH;

/**
* Implementation of {@link IdeContext} for testing.
*/
Expand All @@ -34,12 +36,11 @@ public class AbstractIdeTestContext extends AbstractIdeContext {
*
* @param factory the {@link Function} to create {@link IdeSubLogger} per {@link IdeLogLevel}.
* @param userDir the optional {@link Path} to current working directory.
* @param toolRepository @param toolRepository the {@link ToolRepository} of the context. If it is set to {@code null}
* {@link DefaultToolRepository} will be used.
* @param toolRepository @param toolRepository the {@link ToolRepository} of the context. If it is set to {@code null} {@link DefaultToolRepository} will be
* used.
* @param answers the automatic answers simulating a user in test.
*/
public AbstractIdeTestContext(Function<IdeLogLevel, IdeSubLogger> factory, Path userDir,
ToolRepository toolRepository, String... answers) {
public AbstractIdeTestContext(Function<IdeLogLevel, IdeSubLogger> factory, Path userDir, ToolRepository toolRepository, String... answers) {

super(IdeLogLevel.TRACE, factory, userDir, toolRepository);
this.answers = answers;
Expand Down Expand Up @@ -73,6 +74,10 @@ public Map<String, IdeProgressBarTestImpl> getProgressBarMap() {
@Override
public IdeProgressBar prepareProgressBar(String taskName, long size) {

if (size == 0) {
size = DEFAULT_CONTENT_LENGTH;
}

IdeProgressBarTestImpl progressBar = new IdeProgressBarTestImpl(taskName, size);
IdeProgressBarTestImpl duplicate = this.progressBarMap.put(taskName, progressBar);
// If we have multiple downloads, we may have an existing "Downloading" key
Expand All @@ -97,11 +102,11 @@ public void setSystemInfo(SystemInfo systemInfo) {
this.systemInfo = systemInfo;
}


/**
* @param fileAccess the {@link FileAccess} to use for testing.
*/
public void setMockFileAccess(FileAccess fileAccess){
public void setMockFileAccess(FileAccess fileAccess) {

this.mockFileAccess = fileAccess;
}
}
141 changes: 99 additions & 42 deletions cli/src/test/java/com/devonfw/tools/ide/io/IdeProgressBarTest.java
Original file line number Diff line number Diff line change
@@ -1,42 +1,99 @@
package com.devonfw.tools.ide.io;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.any;
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;

import java.nio.file.Path;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import com.devonfw.tools.ide.context.AbstractIdeContextTest;
import com.devonfw.tools.ide.context.IdeContext;
import com.github.tomakehurst.wiremock.junit5.WireMockTest;

/**
* Test of {@link IdeProgressBar}.
*/
@WireMockTest(httpPort = 8080)
public class IdeProgressBarTest extends AbstractIdeContextTest {

/**
* Tests if a download of a file with a valid content length was displaying an {@link IdeProgressBar} properly.
*
* @param tempDir temporary directory to use.
*/
@Test
public void testProgressBarDownloadWithValidContentLength(@TempDir Path tempDir) {

int maxLength = 10_000;

stubFor(any(urlMatching("/os/.*")).willReturn(aResponse().withStatus(200).withBody(new byte[maxLength])
.withHeader("Content-Length", String.valueOf(maxLength))));

IdeContext context = newContext(tempDir);
FileAccess impl = context.getFileAccess();
impl.download("http://localhost:8080/os/windows_x64_url.tgz", tempDir.resolve("windows_x64_url.tgz"));
assertThat(tempDir.resolve("windows_x64_url.tgz")).exists();
assertProgressBar(context, "Downloading", maxLength);
}
}
package com.devonfw.tools.ide.io;

import com.devonfw.tools.ide.context.AbstractIdeContextTest;
import com.devonfw.tools.ide.context.IdeContext;
import com.devonfw.tools.ide.context.IdeTestContext;
import com.devonfw.tools.ide.log.IdeLogLevel;
import com.github.tomakehurst.wiremock.junit5.WireMockTest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.nio.file.Path;

import static com.devonfw.tools.ide.io.FileAccessImpl.DEFAULT_CONTENT_LENGTH;
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.any;
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;

/**
* Test of {@link IdeProgressBar}.
*/
@WireMockTest(httpPort = 8080)
public class IdeProgressBarTest extends AbstractIdeContextTest {

private static final int MAX_LENGTH = 10_000;

private static final String TEST_URL = "http://localhost:8080/os/windows_x64_url.tgz";

/**
* Tests if a download of a file with a valid content length was displaying an {@link IdeProgressBar} properly.
*
* @param tempDir temporary directory to use.
*/
@Test
public void testProgressBarDownloadWithValidContentLength(@TempDir Path tempDir) {

stubFor(any(urlMatching("/os/.*")).willReturn(
aResponse().withStatus(200).withBody(new byte[MAX_LENGTH]).withHeader("Content-Length", String.valueOf(MAX_LENGTH))));

IdeContext context = newContext(tempDir);
FileAccess impl = context.getFileAccess();
impl.download(TEST_URL, tempDir.resolve("windows_x64_url.tgz"), true);
assertThat(tempDir.resolve("windows_x64_url.tgz")).exists();
assertProgressBar(context, "Downloading", MAX_LENGTH);
}

/**
* Tests if {@link FileAccessImpl#download(String, Path, boolean)} with default value for missing content length is working properly.
*
* @param tempDir temporary directory to use.
*/
@Test
public void testProgressBarDownloadWithDefaultValueForMissingContentLength(@TempDir Path tempDir) {

//arrange
String taskName = "Downloading";
stubFor(any(urlMatching("/os/.*")).willReturn(aResponse().withStatus(200).withBody(new byte[MAX_LENGTH])));
IdeTestContext context = newContext(tempDir);
FileAccess impl = context.getFileAccess();
//act
impl.download(TEST_URL, tempDir.resolve("windows_x64_url.tgz"), true);

//assert
checkLogMessageForDefaultContentLength(context, TEST_URL);
assertThat(tempDir.resolve("windows_x64_url.tgz")).exists();
IdeProgressBarTestImpl progressBar = context.getProgressBarMap().get(taskName);
assertThat(progressBar.getMaxSize()).isEqualTo(DEFAULT_CONTENT_LENGTH);
}

private void checkLogMessageForDefaultContentLength(IdeTestContext context, String source) {

assertLogMessage(context, IdeLogLevel.WARNING,
"Content-Length was not provided by download/copy source: " + source + ". Using fallback: Content-Length for the progress bar is set to 10000000.");
}

/**
* Tests if {@link FileAccessImpl#download(String, Path, boolean)} with default value for missing content length is working properly.
*
* @param tempDir temporary directory to use.
*/
@Test
public void testProgressBarCopyWithDefaultValueForMissingContentLength(@TempDir Path tempDir) {

//arrange
String taskName = "Copying";
IdeTestContext context = newContext(tempDir);
FileAccess impl = context.getFileAccess();
String source = Path.of("src/test/resources/__files").resolve("testZip").toString();

//act
impl.download(source, tempDir.resolve("windows_x64_url.tgz"), true);

//assert
checkLogMessageForDefaultContentLength(context, source);
assertThat(tempDir.resolve("windows_x64_url.tgz")).exists();
IdeProgressBarTestImpl progressBar = context.getProgressBarMap().get(taskName);
assertThat(progressBar.getMaxSize()).isEqualTo(DEFAULT_CONTENT_LENGTH);
}
}
Binary file added cli/src/test/resources/__files/testZip
Binary file not shown.
Loading