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

fix combination of lombok.Builder with Introspected without builder #11347

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

graemerocher
Copy link
Contributor

@graemerocher graemerocher commented Nov 15, 2024

Currently users have to manually defined @Introspected(builder=..) when using Lombok. This change adds native processing of Lombok builder. Not a huge fan of the direct references to Lombok annotation names but I don't see many other options.

Fixes #11344

@graemerocher graemerocher added the type: bug Something isn't working label Nov 15, 2024
@@ -722,7 +722,8 @@ private void writeIntrospectionClass(ClassWriterOutputVisitor classWriterOutputV
dispatchWriter.buildGetTargetMethodByIndex(classWriter);
buildFindIndexedProperty(classWriter);
buildGetIndexedProperties(classWriter);
boolean hasBuilder = annotationMetadata != null && annotationMetadata.isPresent(Introspected.class, "builder");
boolean hasBuilder = annotationMetadata != null &&
(annotationMetadata.isPresent(Introspected.class, "builder") || annotationMetadata.hasDeclaredAnnotation("lombok.Builder"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an annotation remapper to avoid referencing the Lombok here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but also are we sure we want to generate an instrospection for all Lombok annotated builders?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we can add a visitor that will only remap it if the builder annotation is present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I move the reference to Lombok from one place to another?

// unfortunately sometimes we don't see the Lombok transformations
// so assume if the class is annotated with Lombok builder we cannot
// access the constructor.
if (!ce.hasDeclaredAnnotation(ANN_LOMBOK_BUILDER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be added as an option to the introspected or builder annotation and use a remapper to avoid the lombok reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I understand how

.orElse(null);
if (methodElement == null) {
// Lombok processing not done yet, try again in the next round.
throw new ElementPostponedToNextRoundException(element);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should deprecate PostponedToNextRoundException and use only ElementPostponedToNextRoundException

Copy link

sonarcloud bot commented Nov 19, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
32.1% Coverage on New Code (required ≥ 70%)

See analysis details on SonarQube Cloud

@graemerocher graemerocher merged commit 9f7af32 into 4.7.x Nov 22, 2024
20 of 21 checks passed
@graemerocher graemerocher deleted the issue11344 branch November 22, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BeanInstantiationException caused by IllegalAccessError when running test since upgrading to 4.7.0
2 participants