Make Plugin.getGroupId() NonNull #4530
Conversation
|
Thanks for the suggestion @rob-valor ! I think this indeed more closely matches the default groupId handling in Maven. Let me tag @knutwannheden for a final check on serialization since I'm reviewing from mobile. |
|
It’s strange that you would have to change It’s true that the null check in Are you sure you are using the latest version of Rewrite? I have noticed with my own recipe library that sometimes the Maven Plugin does not pick the Rewrite version I want… |
|
@DidierLoiseau, the issue is not in the (raw) pom of the project being rewritten. It's in the But since you mention the logic you added, maybe that should also just depend on the default getter logic I added. That way, it's possible to detect a missing |
I don’t understand what you mean by such defaults – the default value for
If you want to add the missing In fact, the |
|
As a quick note in between holidays: there is already a recipe to make implicit groupIds explicit: I am still chasing down a potentially related failure downstream in rewrite-jackson: Any quick insight there @DidierLoiseau ? You've been diving deeper there lately and have provided a lot of helpful context already. |
Didn’t know about that one. I have removed the example test from the other PR then.
Could it be that the |
As suggested on - #4530 (comment) To fix the downstream issues on rewrite-jackson - #4530 (comment)
Thanks a lot spotting that annotation and the linked context of the stackoverflow issue; I've pushed up a quick PR to remove it: If that passes I'll see it through (assuming no harm) and see if that fixes rewrite-jackson. |
As suggested on - #4530 (comment) To fix the downstream issues on rewrite-jackson - #4530 (comment)
Yes, I checked-out main branch of rewrite and rewrite-maven-plugin. In rewrite I added
Well, I can only say, it is Since Insufficient test setup?I used the entire pom of our project in an existing test private static @Nullable Plugin findPlugin(Xml.Tag tag, List<Plugin> plugins) {
System.out.println("plugins");
plugins.stream().forEach(p -> System.out.println(p.getGroupId() + ":"+p.getArtifactId()+ ":" + p.getVersion()));Actual rewrite: Output during test: |
null originI added a |
|
Now that I looked into that caching thing there, I eventually found the issue. The Please to consider putting the logic of the If you want I can remove all duplicate logic and keep it only in the |
Good catch! This means we know what bug we are fighting against, and it would potentially be possible to write a unit test for that. I’m not sure what’s best approach here though. This would only affect people who have run the old version of Rewrite and have such resolved poms in their cache already. Maybe the best would be to validate the values in the Also maybe there is a mechanism to clear the cache automatically when such a change is detected? (didn’t check) |
|
Well, I think, if it's all covered in There also won't be any such logic dangling around anymore (if I would clean it all up 😉 ). It's only in the upcoming Maven 4 that they fixed the |
|
Thanks for the detailed look here both! I'm hopping in and out, and will be away for the rest of the week before it's conference season starting next week. Let me know what the both of you settle on as the best way forward! 😅 I think handling a poisoned |
|
Aha! Maybe I’ll meet you on Monday then @timtebeek 😉 Enjoy the rest of your holidays! |
|
I think these changes look reasonable. Happy to continue to iterate as the discussion evolves. Thanks @rob-valor and @DidierLoiseau ! |
What's changed?
org.openrewrite.maven.tree.Plugin.getGroupId()is now non-null, defaulting toorg.openrewrite.maven.tree.Plugin.PLUGIN_DEFAULT_GROUPID.What's your motivation?
closes #4526
This ensures no more NullPointerExceptions can occur. Plugins can have a
nullgroupIdbut not all places of the code take that situation into account. By automatically defaulting toorg.apache.maven.pluginsin the getter itself, there is no more need in the code to duplicate the nullability branching, and potentially forgetting it.Anything in particular you'd like reviewers to focus on?
I was not able to provide a test for this case because the project being tested needs to use a parent pom that uses plugin management with plugins not explicitly setting
<groupId>org.apache.maven.plugins</groupId>. I could not find a public pom file that I could use as a parent in the tests.Locally there was 1, on first sight, non related test (
MavenPomDownloaderTest.mirrorsOverrideRepositoriesInPom()) failing. I verified the changes by running our failing recipes again and now the NPE is gone.Have you considered any alternatives or workarounds?
We bump into this issue because we use jgitver to manage out Maven versions. I tried to fix the issue in jgitver itself but it is using native maven to write the built pom file and it is in that native Maven code that the
groupIdfor all plugins of org.apache.maven.plugins is not written to the new pom.xml file