Catch non-semver compatible exception in case of version update #4656
Conversation
| @@ -262,7 +262,7 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) | |||
| acc.versionPropNameToGA.put(versionVariableName, ga); | |||
| acc.gaToNewVersion.put(ga, resolvedVersion); | |||
| } | |||
| } catch (MavenDownloadingException e) { | |||
| } catch (IllegalStateException | MavenDownloadingException e) { | |||
There was a problem hiding this comment.
This probably should instead be fixed here:
https://github.com/openrewrite/rewrite/blob/main/rewrite-gradle%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fgradle%2FDependencyVersionSelector.java#L168
The rationale is that the version is being compared and should really be skipped as exotic. The DependencyVersionSelector is used in several spots all of which could encounter this exact same issue. By moving this fix to the location in the link, we'll fix all of the recipes in one fell swoop.
There was a problem hiding this comment.
I did consider that also. However, I thought "If you use the versionSelector, you know that a version is selectable".
What would you return if no version could be selected? Null? I thought not all usages of that class might work with that.
But you know this part 10 times better so if you think it is safe, I am 100% agreeing!
There was a problem hiding this comment.
@shanman190 moved the check to the proposed position!
There was a problem hiding this comment.
Sweet!
Yeah, the version selector is simply trying to answer the question of: "is there a version to which we can upgrade to?" At which point, the version selector should return only the value to be upgraded to or null to indicate that there was no upgrade available or possible. In particular, with versions like the one from the issue, there's less of a natural order (outside of ASCII) in order to ensure that we are moving up the version strictly. This appears as well with the old Spring Cloud release trains where they were using the alpha ordering of train stations across Europe to order their releases (ie. Angel, Brixton, Camden, etc).
|
Fixes #4654 |
See the slack exception logs -> https://rewriteoss.slack.com/archives/C01A843MWG5/p1730974519581749