ensuring connection timeouts get wrapped by MavenDownloadingException #4081

nmck257 · 2024-03-06T17:34:30Z

Closes #4080.

Might take a couple iterations in Actions; I wrote/tested this in a weird network environment. Fingers crossed.

Checklist

I've added unit tests to cover both positive and negative cases
I've read and applied the recipe conventions and best practices
I've used the IntelliJ IDEA auto-formatter on affected files

…sing tests to fail with latest changes

nmck257 · 2024-03-06T20:42:32Z

rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java

@@ -769,8 +771,6 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
                                            repository.getSnapshots(),
                                            repository.getUsername(),
                                            repository.getPassword());
-                                } else if (!e.isClientSideException()) {
-                                    return originalRepository;


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

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

rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java

timtebeek · 2024-03-07T22:50:41Z

Two new test failures with the new approach: https://ge.openrewrite.org/s/p33d2lkyakx4u/tests/overview?outcome=FAILED

listenerRecordsFailedRepositoryAccess()

java.lang.AssertionError: 	
Expecting throwable message:	
  "65b2442b-44ab-4f5c-a7c4-ec45d42f42a3.com"	
to contain:	
  "java.net.UnknownHostException"	
but did not.	
Throwable that failed the check:	
java.net.UnknownHostException: 65b2442b-44ab-4f5c-a7c4-ec45d42f42a3.com	
	at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:572)	
	at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327)

connectTimeout()

Expecting actual throwable to be an instance of:	
  org.openrewrite.maven.MavenDownloadingException	
but was:	
  java.lang.RuntimeException: java.net.SocketTimeoutException: Connect timed out	
	at org.openrewrite.maven.internal.MavenPomDownloader.requestAsAuthenticatedOrAnonymous(MavenPomDownloader.java:810)	
	at org.openrewrite.maven.internal.MavenPomDownloader.download(MavenPomDownloader.java:574)	
	at org.openrewrite.maven.internal.MavenPomDownloaderTest.lambda$connectTimeout$14(MavenPomDownloaderTest.java:752)	
	...(106 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)

…ownloadingException

nmck257 · 2024-03-08T19:29:09Z

pushed further changes, focused on 1) fixing those test failures and 2) using specific checked exception types instead of Throwable in method signatures

timtebeek

Thanks a lot! Moving away from throwable and using a checked IOException does really help take some of the magic out on what can and can't happen. Good to have this case caught & handled correctly, thanks!

…openrewrite#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]>

@b

…ti module maven projects (#4074) * fix find maven dependencies for multi module maven projects * revert to see if build is succesfull * Defensive copy in addClasspathEntry so that it won't attempt to add an element to a read-only list * Make `JavaParser.Builder#addClasspathEntry()` public * Basic JSP support (#4075) * Improve `UsesField` * Improve `UsesField` a bit more * Fix RenameVariable for variables referenced through type casts (#4082) * Fix RenameVariable for variables referenced through type casts * Add unit test * Be explicit about null handling and types --------- Co-authored-by: Tim te Beek <[email protected]> * Java version equality should not be based on id. Plugins create a JavaVersion per source set, but if they have the same fields excluding id they should be considered equal. * `MethodMatcher`: Improve construction time performance Avoid expensive regular expressions for simple and common cases. * `MethodMatcher`: Optimize performance of common argument matching * `XPathMatcher`: Implement attribute value conditions like `/a[@b='c']` (#4084) Fixes: #4083 * Fix class cast exception in MarkIndividualDependency * Add table and version matcher to Maven FindProperties (#4085) The valuePattern allows use as DevCenter measurement, while the table enables custom visualizations. Target use is the `<jenkins.version>` used in Jenkins plugins. * ensuring connection timeouts get wrapped by MavenDownloadingException (#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]> * HasMiniumJavaVersion * Remove explicit `Properties.File#getContent()` method While indeed the LST should not be modified by modifying a `List` returned by a getter, it is also not right for a getter to return a new list every time (e.g. by wrapping an internal field using `Collections.unmodifiableList()`. This commit removes the explicit getter and wither and instead has these generated by Lombok. Further, the initial contents, as populated by the parser, are now wrapped using `Collections.unmodifiableList()` to avoid any situations, where the contents are directly modified. * Add dependency now works with empty maven projects (#4006) * Printable Recipe Datatables (#4087) * Added method to export datatable information as csv to RecipeRun class * Wrapped column value in quotes to escape commas in value * Refactored streams to iterative loops. Log exceptions through Execution Context error handler * Added formatCsv method. Added logic to ensure directories exist before creating files * Refactored export logic to be more testable. Added test for csv printing logic * Failing test cases for Quarkus version matching using `metadataPattern`s * Add another test case. * Get the new tests passing * License headers * Optimize `JavaParser.Builder#addClasspathEntry()` * Allow null argument to CommitMessage.message() * Fix precondition application in UpgradeDependencyVersion. I'm a bit confused as to how this recipe succeeded before I made this change. The whole visitor, not just the precondition, was wrapped in Preconditions.or(). * Refresh resolved dependencies if possible --------- Co-authored-by: U808771 <[email protected]> Co-authored-by: Sam Snyder <[email protected]> Co-authored-by: Knut Wannheden <[email protected]> Co-authored-by: Jonathan Schnéider <[email protected]> Co-authored-by: René Grob <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: Nick McKinney <[email protected]> Co-authored-by: Adriano Machado <[email protected]> Co-authored-by: Ryan Hudson <[email protected]>

@b

…ti module maven projects (#4074) * fix find maven dependencies for multi module maven projects * revert to see if build is succesfull * Defensive copy in addClasspathEntry so that it won't attempt to add an element to a read-only list * Make `JavaParser.Builder#addClasspathEntry()` public * Basic JSP support (#4075) * Improve `UsesField` * Improve `UsesField` a bit more * Fix RenameVariable for variables referenced through type casts (#4082) * Fix RenameVariable for variables referenced through type casts * Add unit test * Be explicit about null handling and types --------- Co-authored-by: Tim te Beek <[email protected]> * Java version equality should not be based on id. Plugins create a JavaVersion per source set, but if they have the same fields excluding id they should be considered equal. * `MethodMatcher`: Improve construction time performance Avoid expensive regular expressions for simple and common cases. * `MethodMatcher`: Optimize performance of common argument matching * `XPathMatcher`: Implement attribute value conditions like `/a[@b='c']` (#4084) Fixes: #4083 * Fix class cast exception in MarkIndividualDependency * Add table and version matcher to Maven FindProperties (#4085) The valuePattern allows use as DevCenter measurement, while the table enables custom visualizations. Target use is the `<jenkins.version>` used in Jenkins plugins. * ensuring connection timeouts get wrapped by MavenDownloadingException (#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]> * HasMiniumJavaVersion * Remove explicit `Properties.File#getContent()` method While indeed the LST should not be modified by modifying a `List` returned by a getter, it is also not right for a getter to return a new list every time (e.g. by wrapping an internal field using `Collections.unmodifiableList()`. This commit removes the explicit getter and wither and instead has these generated by Lombok. Further, the initial contents, as populated by the parser, are now wrapped using `Collections.unmodifiableList()` to avoid any situations, where the contents are directly modified. * Add dependency now works with empty maven projects (#4006) * Printable Recipe Datatables (#4087) * Added method to export datatable information as csv to RecipeRun class * Wrapped column value in quotes to escape commas in value * Refactored streams to iterative loops. Log exceptions through Execution Context error handler * Added formatCsv method. Added logic to ensure directories exist before creating files * Refactored export logic to be more testable. Added test for csv printing logic * Failing test cases for Quarkus version matching using `metadataPattern`s * Add another test case. * Get the new tests passing * License headers * Optimize `JavaParser.Builder#addClasspathEntry()` * Allow null argument to CommitMessage.message() * Fix precondition application in UpgradeDependencyVersion. I'm a bit confused as to how this recipe succeeded before I made this change. The whole visitor, not just the precondition, was wrapped in Preconditions.or(). * Refresh resolved dependencies if possible --------- Co-authored-by: U808771 <[email protected]> Co-authored-by: Sam Snyder <[email protected]> Co-authored-by: Knut Wannheden <[email protected]> Co-authored-by: Jonathan Schnéider <[email protected]> Co-authored-by: René Grob <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: Nick McKinney <[email protected]> Co-authored-by: Adriano Machado <[email protected]> Co-authored-by: Ryan Hudson <[email protected]>

nmck257 added 2 commits March 6, 2024 12:31

ensuring connection timeouts get wrapped by MavenDownloadingException

4d8208e

polish

246ea4a

timtebeek assigned nmck257 Mar 6, 2024

timtebeek self-requested a review March 6, 2024 17:48

removed a condition which I believe no longer made sense, and was cau…

e027e6c

…sing tests to fail with latest changes

nmck257 commented Mar 6, 2024

View reviewed changes

Merge branch 'main' into bugfix/4080

1ba13a7

timtebeek reviewed Mar 7, 2024

View reviewed changes

rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java Outdated Show resolved Hide resolved

Apply suggestions from code review

71e407d

timtebeek added bug Something isn't working enhancement New feature or request labels Mar 7, 2024

nmck257 added 2 commits March 8, 2024 13:57

adjusting so that IOExceptions can get caught and wrapped into MavenD…

de0fd19

…ownloadingException

clarifying checked exception types for sendRequest and related methods

f60f905

Merge branch 'main' into bugfix/4080

0a32e07

timtebeek approved these changes Mar 8, 2024

View reviewed changes

nmck257 merged commit 1b4ec47 into main Mar 8, 2024
1 check passed

nmck257 deleted the bugfix/4080 branch March 8, 2024 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

nmck257 commented Mar 6, 2024

nmck257 Mar 6, 2024

nmck257 Mar 6, 2024

timtebeek commented Mar 7, 2024

nmck257 commented Mar 8, 2024

timtebeek left a comment

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

Conversation

nmck257 commented Mar 6, 2024

Checklist

nmck257 Mar 6, 2024

Choose a reason for hiding this comment

nmck257 Mar 6, 2024

Choose a reason for hiding this comment

timtebeek commented Mar 7, 2024

nmck257 commented Mar 8, 2024

timtebeek left a comment

Choose a reason for hiding this comment