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

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

Merged
merged 8 commits into from
Mar 8, 2024
Merged

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4d8208e
ensuring connection timeouts get wrapped by MavenDownloadingException
nmck257 Mar 6, 2024
246ea4a
polish
nmck257 Mar 6, 2024
e027e6c
removed a condition which I believe no longer made sense, and was cau…
nmck257 Mar 6, 2024
1ba13a7
Merge branch 'main' into bugfix/4080
timtebeek Mar 7, 2024
71e407d
Apply suggestions from code review
timtebeek Mar 7, 2024
de0fd19
adjusting so that IOExceptions can get caught and wrapped into MavenD…
nmck257 Mar 8, 2024
f60f905
clarifying checked exception types for `sendRequest` and related methods
nmck257 Mar 8, 2024
0a32e07
Merge branch 'main' into bugfix/4080
timtebeek Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ byte[] sendRequest(HttpSender.Request request) throws Throwable {
});
} catch (FailsafeException failsafeException) {
throw failsafeException.getCause() == null ? failsafeException : failsafeException.getCause();
} catch (Throwable e) {
throw new HttpSenderResponseException(e, null, "");
timtebeek marked this conversation as resolved.
Show resolved Hide resolved
} finally {
this.ctx.recordResolutionTime(Duration.ofNanos(System.nanoTime() - start));
}
Expand Down Expand Up @@ -769,8 +771,6 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
repository.getSnapshots(),
repository.getUsername(),
repository.getPassword());
} else if (!e.isClientSideException()) {
return originalRepository;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removing this feels a little cryptic, but I'm pretty sure it's no longer relevant?
eg, when it was written, the e.isServerReached call a few lines earlier did not exist:
3bcafba#diff-a39c7415cc78264a78509aaaba13d76b4eeea28cdc5723e15615b7a8add5b325

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that said, tests pass -- ready for review @timtebeek

}
} catch (Throwable e) {
// ok to fall through here and cache a null
Expand Down
17 changes: 15 additions & 2 deletions rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Expand Down Expand Up @@ -86,7 +85,7 @@ void ossSonatype() {
null, "true", false, null, null, null);
MavenRepository repo = new MavenPomDownloader(ctx).normalizeRepository(ossSonatype,
MavenExecutionContextView.view(ctx), null);
assertThat(requireNonNull(repo).getUri()).isEqualTo(ossSonatype.getUri());
assertThat(repo).isNotNull().extracting((MavenRepository::getUri)).isEqualTo(ossSonatype.getUri());
}

@Issue("https://github.com/openrewrite/rewrite/issues/3908")
Expand Down Expand Up @@ -742,6 +741,20 @@ void doNotRenameRepoForCustomMavenLocal(@TempDir Path tempDir) throws MavenDownl
assertThat(result.getRepository().getUri()).startsWith(tempDir.toUri().toString());
}

@Issue("https://github.com/openrewrite/rewrite/issues/4080")
@Test
void connectTimeout() {
var downloader = new MavenPomDownloader(ctx);
var gav = new GroupArtifactVersion("org.openrewrite", "rewrite-core", "7.0.0");
var repos = singletonList(MavenRepository.builder()
.id("non-routable").uri("http://10.0.0.0/maven").knownToExist(true).build());

assertThatThrownBy(() -> downloader.download(gav, null, null, repos))
.isInstanceOf(MavenDownloadingException.class)
.hasMessageContaining("rewrite-core")
.hasMessageContaining("10.0.0.0");
}

private static GroupArtifactVersion createArtifact(Path repository) throws IOException {
Path target = repository.resolve(Paths.get("org", "openrewrite", "rewrite", "1.0.0"));
Path pom = target.resolve("rewrite-1.0.0.pom");
Expand Down