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.

Sign up for GitHub

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

Jump to bottom

Check that we have downloaded the file successfully #4627

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Check that we have downloaded the file successfully #4627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
052d45e
Check that we have downloaded the file successfully
lkerford Oct 29, 2024
eaa2dd1
Updating tests
lkerford Oct 29, 2024
69bc8f7
Updating tests
lkerford Oct 29, 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
1 change: 1 addition & 0 deletions rewrite-core/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ dependencies {
implementation("io.github.classgraph:classgraph:latest.release")
implementation("org.yaml:snakeyaml:latest.release")

testImplementation("org.assertj:assertj-core:latest.release")
testImplementation(project(":rewrite-test"))
}
6 changes: 5 additions & 1 deletion rewrite-core/src/main/java/org/openrewrite/remote/RemoteArchive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ public InputStream getInputStream(ExecutionContext ctx) {
Path localArchive = cache.compute(uri, () -> {
//noinspection resource
HttpSender.Response response = httpSender.send(httpSender.get(uri.toString()).build());
return response.getBody();
if (response.isSuccessful()) {
Copy link
Member

Choose a reason for hiding this comment

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

question to team:
should we be doing some type of retry here to improve reliability with transient errors like network glitch or delays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If want to implement retires, I think we might want to move this change to the HttpSender, I suspect other endpoints could benefit from it

return response.getBody();
} else {
throw new IllegalStateException("Failed to download " + uri + " to artifact cache got an " + response.getCode());
}
}, ctx.getOnError());

if (localArchive == null) {
Expand Down
6 changes: 5 additions & 1 deletion rewrite-core/src/main/java/org/openrewrite/remote/RemoteFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ public InputStream getInputStream(ExecutionContext ctx) {
Path localFile = cache.compute(uri, () -> {
//noinspection resource
HttpSender.Response response = httpSender.get(uri.toString()).send();
return response.getBody();
if (response.isSuccessful()) {
return response.getBody();
} else {
throw new IllegalStateException("Failed to download " + uri + " to artifact cache got an " + response.getCode());
}
}, ctx.getOnError());

if (localFile == null) {
Expand Down
28 changes: 28 additions & 0 deletions rewrite-core/src/test/java/org/openrewrite/remote/RemoteArchiveTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.openrewrite.remote;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.openrewrite.ExecutionContext;
Expand All @@ -30,6 +31,7 @@

import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class RemoteArchiveTest {

Expand All @@ -38,6 +40,8 @@ class RemoteArchiveTest {
void gradleWrapper(String version) throws Exception {
URL distributionUrl = requireNonNull(RemoteArchiveTest.class.getClassLoader().getResource("gradle-" + version + "-bin.zip"));
ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapper")));
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(distributionUrl::openStream));

Expand All @@ -52,19 +56,43 @@ void gradleWrapper(String version) throws Exception {
assertThat(actual).isGreaterThan(50_000);
}

@Test
void gradleWrapperDownloadFails() throws Exception {
URL distributionUrl = requireNonNull(RemoteArchiveTest.class.getClassLoader().getResource("gradle-7.4.2-bin.zip"));
ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapperDownloadFails")));
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(408));

RemoteArchive remoteArchive = Remote
.builder(
Paths.get("gradle/wrapper/gradle-wrapper.jar"),
distributionUrl.toURI()
)
.build("gradle-[^\\/]+\\/(?:.*\\/)+gradle-wrapper-(?!shared).*\\.jar");

assertThatThrownBy(() -> getInputStreamSize(remoteArchive.getInputStream(ctx)))
.isInstanceOf(IllegalStateException.class)
.hasMessage("Failed to download " + distributionUrl.toURI() + " to artifact cache");
}

@ParameterizedTest
@ValueSource(strings = {"7.4.2", "7.5-rc-1", "7.6"})
void gradleWrapperConcurrent(String version) throws Exception {
int executionCount = 5;
ExecutorService executorService = Executors.newFixedThreadPool(executionCount);
CompletionService<Long> completionService = new ExecutorCompletionService<>(executorService);
LocalRemoteArtifactCache localRemoteArtifactCache = new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapperConcurrent"));

for (int i = 0; i < executionCount; i++) {
completionService.submit(() -> {
URL distributionUrl = requireNonNull(RemoteArchiveTest.class.getClassLoader()
.getResource("gradle-" + version + "-bin.zip"));

ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(localRemoteArtifactCache);
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(distributionUrl::openStream));

Expand Down
28 changes: 28 additions & 0 deletions rewrite-core/src/test/java/org/openrewrite/remote/RemoteFileTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,16 @@

import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

class RemoteFileTest {

@Test
void gradleWrapperProperties() throws Exception {
URL distributionUrl = requireNonNull(RemoteFileTest.class.getClassLoader().getResource("gradle-wrapper.properties"));
ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapperProperties")));
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(distributionUrl::openStream));

Expand All @@ -50,16 +53,41 @@ void gradleWrapperProperties() throws Exception {
assertThat(actual).isGreaterThan(800);
}

@Test
void gradleWrapperDownloadFails() throws Exception {
URL distributionUrl = requireNonNull(RemoteFileTest.class.getClassLoader().getResource("gradle-wrapper.properties"));
ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapperDownloadFails")));
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(408));

RemoteArchive remoteFile = Remote
.builder(
Paths.get("gradle/wrapper/gradle-wrapper.properties"),
distributionUrl.toURI()
)
.build("gradle-[^\\/]+\\/(?:.*\\/)+gradle-wrapper-(?!shared).*\\.jar");


assertThatThrownBy(() -> getInputStreamSize(remoteFile.getInputStream(ctx)))
.isInstanceOf(IllegalStateException.class)
.hasMessage("Failed to download " + distributionUrl.toURI() + " to artifact cache");
}

@Test
void gradleWrapperPropertiesConcurrent() throws Exception {
int executionCount = 5;
ExecutorService executorService = Executors.newFixedThreadPool(executionCount);
CompletionService<Long> completionService = new ExecutorCompletionService<>(executorService);
LocalRemoteArtifactCache localRemoteArtifactCache = new LocalRemoteArtifactCache(
Paths.get(System.getProperty("user.home") + "/.rewrite/remote/gradleWrapperPropertiesConcurrent"));
for (int i = 0; i < executionCount; i++) {
completionService.submit(() -> {
URL distributionUrl = requireNonNull(RemoteFileTest.class.getClassLoader().getResource("gradle-wrapper.properties"));

ExecutionContext ctx = new InMemoryExecutionContext();
RemoteExecutionContextView.view(ctx).setArtifactCache(localRemoteArtifactCache);
HttpSenderExecutionContextView.view(ctx)
.setLargeFileHttpSender(new MockHttpSender(distributionUrl::openStream));

Expand Down
14 changes: 13 additions & 1 deletion rewrite-test/src/main/java/org/openrewrite/test/MockHttpSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,25 @@
*/
public class MockHttpSender implements HttpSender {
final UncheckedSupplier<InputStream> is;
int responseCode = 200;

public MockHttpSender(UncheckedSupplier<InputStream> is) {
this.is = is;
}

public MockHttpSender(int responseCode) {
this.is = null;
this.responseCode = responseCode;
}

@Override
public Response send(Request request) {
return new Response(200, is.get(), () -> {});
if (responseCode != 200) {
return new Response(responseCode, null, () -> {
});
} else {
return new Response(responseCode, is.get(), () -> {
});
}
}
}