Add recipe to configure the JPA Modelgen annotation processor

[gsmet]

@famod @yrodiere could you have a look at the tests and make sure that's what we want? I tried to cover as many cases as I could think of but I might have forgotten some.

[gsmet]

@timtebeek I was wondering if you would be interested in us contributing these two generic recipes? They are relatively specific but I suppose they could be useful?

• ChangeMavenCompilerAnnotationProcessorGroupIdAndArtifactId
• AddMavenCompilerAnnotationProcessor

(name says it all)

Please have a look and let me know if they make sense and if changes would be needed (except adding more tests to cover all the use cases of the recipes), the tests in our repo are solely intented to test our upgrades.

No problem if you think they are too specific.

Thanks.

[timtebeek]

Hi @gsmet would definitely be open to receiving those, and thanks for the offer! At first glance these look good, but I'll get behind my laptop for a full review once there's a PR to OpenRewrite/rewrite.

[yrodiere]

LGTM.

Since you asked, here are a few more cases you might want to test (sorry!):

1. When the version of hibernate-jpamodelgen is managed through the Quarkus BOM
2. When the version of hibernate-jpamodelgen is managed through a custom <dependencyManagement> section
3. When hibernate-jpamodelgen is added as a dependency of the plugin. I have no idea how legimate that is, but I know we've used that in another project with another annotation processor plugin and it at least had an effect...
4. When some (all?) of the configuration of the maven-compiler-plugin is defined through <pluginManagement>.
5. When maven-compiler-plugin has extra configuration, in particular an explicit <annotationProcessors>
6. When annotation processing configuration is more complex, e.g. with a custom execution, when using <proc>none</proc>/<proc>only</proc> in some executions, processing test classes, ...
7. When the application uses org.bsc.maven:maven-processor-plugin for annotation processing. Maybe we should not mess with that, considering the configuration of that plugin can get quite complex, but a test checking what happens in that case (especially if annotation processing is disabled in maven-compiler-plugin) wouldn't hurt?

I doubt you can test it all, but I thought I'd bring it to your attention so that you can decide what is worth the effort.

[gsmet]

1. it was more or less already covered (I had one test with the Quarkus BOM). I added one more. It doesn't really change anything as we don't add the version to the annotation processor and we don't try to actually run the projects. We just test the transformation is done.
2. I think this is covered
3. Let's wait if we have feedback from the field about that. I don't want to overcomplicate things
4. I think I will restrict it to plugins and avoid pluginManagement for now.
5. I added some tests for 5 and 6 but refrained from adjusting multiple executions. We will adjust the default config always. If people need specific tuning they will have to adjust by themselves
6. see above
7. yeah, I will refrain from handling that for now.

[famod]

3. Well, you do have to resort to a plugin dependency in certain cases, e.g. for ECJ prior to Fix ECJ not using annotation processor when defined via processorpath codehaus-plexus/plexus-compiler#350 (not released yet) or even with mentioned fix when using lombok with ECJ (which doesn't affect jpamodelgen though). It's not super common though (I think).

[gsmet]

@famod so if I understand correctly, I should drop hibernate-jpamodelgen from the Maven Compiler Plugin dependencies if present so that we only have the annotationProcessorPaths defined?

[famod]

@famod so if I understand correctly, I should drop hibernate-jpamodelgen from the Maven Compiler Plugin dependencies if present so that we only have the annotationProcessorPaths defined?

Well, that's where it gets complicated:
Basically yes, but ECJ users would run into codehaus-plexus/plexus-compiler#349, meaning that jpamodelgen would not run for them. So I would suggest to wait at least for plexus-compiler 2.14.3 or even better: wait for another maven-compiler-plugin version that depends on plexus-compiler 2.14.3 or above (otherwise users would need to fiddle around with plexus-compiler dependencies for m-c-plugin, which is ok-ish but not ideal).

But I have just found a more pressing issue: Eclipse IDE (or better to say m2e) does not seem take a annotationProcessorPaths version from project depMgmt and hence the project breaks.
Is this working for IntelliJ?

[famod]

PS: Reported the Eclipse IDE/m2e issue here: eclipse-m2e/m2e-core#1644

[famod]

@gsmet @yrodiere just want to make sure my previous comments didn't fall through the cracks:
Currently, the way jpamodelgen is advertised to be configured in the migration guide and/or the way it's auto-migrated by quarkus update will break Eclipse project setup!

[yrodiere]

From experience, bugs in ECJ can take a very long time to get fixed (not sure why exactly, I'm not blaming anyone).

In the meantime... Basically, people using Eclipse need to specify the version of jpamodelgen in their annotationProcessorPaths, right?

We can amend the migration guide to mention this.

For quarkus update... I guess we could keep the explicit version if there is one to begin with? It will probably get managed by the Quarkus BOM anyway, so it shouldn't hurt non-Eclipse users?

EDIT: Though if people were using a <dependencyManagement> entry to manage the version of jpamodelgen in a provided dependency, the recipe will still break their build...

[famod]

From experience, bugs in ECJ can take a very long time to get fixed (not sure why exactly, I'm not blaming anyone).

I agree, but in this case m2e (the Maven integration plugin) has to be adjusted instead of EJC: eclipse-m2e/m2e-core#1644

In the meantime... Basically, people using Eclipse need to specify the version of jpamodelgen in their annotationProcessorPaths, right?

Correct!

We can amend the migration guide to mention this.

+1!

For quarkus update... I guess we could keep the explicit version if there is one to begin with? It will probably get managed by the Quarkus BOM anyway, so it shouldn't hurt non-Eclipse users?

Well, an explicit version in annotationProcessorPath will always take precedence over depMgmt (if any, direct or via BOM), no?
The main issue is that quarkus update is not aware of the users IDE (I guess?). So either never omit the version for a processor (until m2e has been adjusted) and hope quarkus update is used frequently or ignore that problem and rely on the migration guide (well, not so user-friendly for Eclipse users).

[yrodiere]

Well, an explicit version in annotationProcessorPath will always take precedence over depMgmt (if any, direct or via BOM), no?

I don't know for annotationProcessorPaths, but in "classic" dependencies, that's not what I remember. Dependency management would take precedence there, IIRC.

So either never omit the version for a processor (until m2e has been adjusted)

+1 I think that would work. @gsmet?

(well, not so user-friendly for Eclipse users).

But then if they're using Eclipse, they're used to that!

... okay that was too easy. Just a joke, Eclipse is great too :)

[yrodiere]

We can amend the migration guide to mention this.

+1!

Done

[famod]

Well, an explicit version in annotationProcessorPath will always take precedence over depMgmt (if any, direct or via BOM), no?

I don't know for annotationProcessorPaths, but in "classic" dependencies, that's not what I remember. Dependency management would take precedence there, IIRC.

True for regular depMgmt (actually, you should be able to override a depMgmt version via dependencies), but compiler-plugin takes a custom approach: https://github.com/apache/maven-compiler-plugin/pull/180/files#diff-d4bac42d8f4c68d397ddbaa05c1cbbed7984ef6dc0bb9ea60739df78997e99eeR1648-R1650

(well, not so user-friendly for Eclipse users).

But then if they're using Eclipse, they're used to that!

... okay that was too easy. Just a joke, Eclipse is great too :)

I guess I was asking for it. ;-)

[maxandersen]

just one thing - this not only affect Eclipse; it affects vscode and any web based IDE too.

[yrodiere]

just one thing - this not only affect Eclipse; it affects vscode and any web based IDE too.

Ok, I updated the migration guide.

[gsmet]

FWIW, I'm working on adjusting the recipes.

[gsmet]

See #118 for the necessary adjustments.

[famod]

Btw, Eclipse m2e has been adjusted, see eclipse-m2e/m2e-core#1659

2.5.1 is planned for end of February: https://github.com/eclipse-m2e/m2e-core/blob/master/RELEASE_NOTES.md#251