Skip to content

Commit

Permalink
ensuring connection timeouts get wrapped by MavenDownloadingException (…
Browse files Browse the repository at this point in the history 
…#4081)

* ensuring connection timeouts get wrapped by MavenDownloadingException

* polish

* removed a condition which I believe no longer made sense, and was causing tests to fail with latest changes

* Apply suggestions from code review

* adjusting so that IOExceptions can get caught and wrapped into MavenDownloadingException

* clarifying checked exception types for `sendRequest` and related methods

---------

Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@nmck257 @timtebeek
nmck257 and timtebeek authored Mar 8, 2024
1 parent 2b32805 commit 1b4ec47
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 15 deletions.
25 changes: 13 additions & 12 deletions rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public MavenPomDownloader(Map<Path, Pom> projectPoms, HttpSender httpSender, Exe
this.mavenCache = this.ctx.getPomCache();
}

byte[] sendRequest(HttpSender.Request request) throws Throwable {
byte[] sendRequest(HttpSender.Request request) throws IOException, HttpSenderResponseException {
long start = System.nanoTime();
try {
return Failsafe.with(retryPolicy).get(() -> {
Expand All @@ -148,7 +148,12 @@ byte[] sendRequest(HttpSender.Request request) throws Throwable {
}
});
} catch (FailsafeException failsafeException) {
throw failsafeException.getCause() == null ? failsafeException : failsafeException.getCause();
if (failsafeException.getCause() instanceof HttpSenderResponseException) {
throw (HttpSenderResponseException) failsafeException.getCause();
}
throw failsafeException;
} catch (UncheckedIOException e) {
throw e.getCause();
} finally {
this.ctx.recordResolutionTime(Duration.ofNanos(System.nanoTime() - start));
}
Expand Down Expand Up @@ -282,7 +287,7 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv
.increment();
result = Optional.of(derivedMeta);
}
} catch (HttpSenderResponseException | MavenDownloadingException e) {
} catch (HttpSenderResponseException | MavenDownloadingException | IOException e) {
repositoryResponses.put(repo, e.getMessage());
}
}
Expand Down Expand Up @@ -328,7 +333,7 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv
* @return Metadata or null if the metadata cannot be derived.
*/
@Nullable
private MavenMetadata deriveMetadata(GroupArtifactVersion gav, MavenRepository repo) throws HttpSenderResponseException, MavenDownloadingException {
private MavenMetadata deriveMetadata(GroupArtifactVersion gav, MavenRepository repo) throws HttpSenderResponseException, IOException, MavenDownloadingException {
if ((repo.getDeriveMetadataIfMissing() != null && !repo.getDeriveMetadataIfMissing()) || gav.getVersion() != null) {
// Do not derive metadata if we cannot navigate/browse the artifacts.
// Do not derive metadata if a specific version has been defined.
Expand Down Expand Up @@ -590,6 +595,8 @@ public Pom download(GroupArtifactVersion gav,
//If the exception is a common, client-side exception, cache an empty result.
mavenCache.putPom(resolvedGav, null);
}
} catch (IOException e) {
repositoryResponses.put(repo, e.getMessage());
}
}
} else if (result.isPresent()) {
Expand Down Expand Up @@ -769,8 +776,6 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
repository.getSnapshots(),
repository.getUsername(),
repository.getPassword());
} else if (!e.isClientSideException()) {
return originalRepository;
}
} catch (Throwable e) {
// ok to fall through here and cache a null
Expand All @@ -797,7 +802,7 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
/**
* Replicates Apache Maven's behavior to attempt anonymous download if repository credentials prove invalid
*/
private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String uriString) throws HttpSenderResponseException {
private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String uriString) throws HttpSenderResponseException, IOException {
try {
return sendRequest(applyAuthenticationToRequest(repo, httpSender.get(uriString)).build());
} catch (HttpSenderResponseException e) {
Expand All @@ -806,12 +811,10 @@ private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String ur
} else {
throw e;
}
} catch (Throwable t) {
throw new RuntimeException(t); // unreachable
}
}

private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseException originalException) throws HttpSenderResponseException {
private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseException originalException) throws HttpSenderResponseException, IOException {
try {
return sendRequest(httpSender.get(uriString).build());
} catch (HttpSenderResponseException retryException) {
Expand All @@ -820,8 +823,6 @@ private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseExcep
} else {
throw retryException;
}
} catch (Throwable t) {
throw new RuntimeException(t); // unreachable
}
}

Expand Down
20 changes: 17 additions & 3 deletions rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.net.UnknownHostException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand All @@ -48,7 +49,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 +86,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 @@ -188,7 +188,7 @@ public void repositoryAccessFailed(String uri, Throwable e) {
// not expected to succeed
}
assertThat(attemptedUris).isNotEmpty();
assertThat(attemptedUris.get(httpUrl)).hasMessageContaining("java.net.UnknownHostException");
assertThat(attemptedUris.get(httpUrl)).isInstanceOf(UnknownHostException.class);
assertThat(discoveredRepositories).isEmpty();
}

Expand Down Expand Up @@ -742,6 +742,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

0 comments on commit 1b4ec47

Please sign in to comment.