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.

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

Handle plugins without groupId #4507

Conversation

DidierLoiseau
Copy link
Contributor

@DidierLoiseau DidierLoiseau commented Sep 20, 2024

What's changed?

Handle the default value of plugin groupId as per https://maven.apache.org/xsd/maven-4.0.0.xsd

See also is groupId required for plugins in Maven pom.xml? on Stack Overflow.

What's your motivation?

Someone declared a plugin without groupId…

Have you considered any alternatives or workarounds?

It would be possible to fix just RemoveRedundantDependencyVersions, but since this default value is defined in the xsd, I thought it would be better to fix the parsing/resolution.

Any additional context

It was causing an invisible NPE in RemoveRedundantDependencyVersions for the pluginManagement case. I’m not sure how such exceptions should be handled.

Theoretically the artifactId is optional as well, but I don’t see why anyone wouldn’t provide it. Moreover there isn’t a default for it.

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

plugins.add(new org.openrewrite.maven.tree.Plugin(
rawPlugin.getGroupId(),
pluginGroupId == null ? PLUGIN_DEFAULT_GROUPID : pluginGroupId,
Copy link
Contributor Author

@DidierLoiseau DidierLoiseau Sep 20, 2024

Choose a reason for hiding this comment

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

I don’t see a way to set it anywhere else because the field is final in the raw plugin, and the constructors are generated by Lombok.

Jackson does not seem to have default value handling either.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@timtebeek timtebeek merged commit 20c44d0 into openrewrite:main Sep 20, 2024
2 checks passed
@timtebeek timtebeek added bug Something isn't working enhancement New feature or request labels Sep 20, 2024
@timtebeek
Copy link
Contributor

Thanks for the many fixes! I've just put out a new patch release:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants