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.

Sign up for GitHub

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

Jump to bottom

ArC - improve error message when annotation default value cannot be read #1838

Merged
merged 3 commits into from
Apr 4, 2019
Merged

ArC - improve error message when annotation default value cannot be read #1838

merged 3 commits into from
Apr 4, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Apr 3, 2019

@mkouba mkouba mentioned this pull request Apr 3, 2019
Closed
@mkouba
Copy link
Contributor Author

mkouba commented Apr 3, 2019

@sarxos I hope the updated error message is a little bit more clear ;-)

gsmet
gsmet reviewed Apr 3, 2019
View reviewed changes
...rojects/arc/processor/src/main/java/io/quarkus/arc/processor/AnnotationLiteralGenerator.java
retValue = loadValue(valueMethod, value, annotationClass, method);
throw new NullPointerException(String.format(
"Value is not set for %s.%s(). Most probably an older version of Jandex was used to index an application dependency. Make sure that Jandex 2.1+ is used.",
method.declaringClass().name(), method.name()));
Copy link
Member

Choose a reason for hiding this comment

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

So we don't need to handle the primitive case anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do handle primitives in AnnotationLiteralGenerator.loadValue(). I've just removed the part which sets the values to the default JLS values - this wouldn't help much and the produced literal would be incorrect.

@sarxos
Copy link
Contributor

sarxos commented Apr 3, 2019

@mkouba It's ok, however @dmlloyd suggested to change NullPointerException to some other exception designed specifically for processing failures. See our discussion in annotations topic on Zulip.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 3, 2019

@sarxos Could pls you verify the re-indexing fallback works?

@sarxos
Copy link
Contributor

sarxos commented Apr 3, 2019

Sure, let me checkout this PR.

@sarxos
Copy link
Contributor

sarxos commented Apr 3, 2019

@mkouba

I confirm. Re-indexing fallback works as expected. Before your patchset has been applied all my tests were failing. After patchset was applied all my tests passed. I also observed warning you included in a handleFilePath(Path) method of ApplicationArchiveBuildStep class:

17:52:20.415 [build-4] DEBUG i.q.d.i.ApplicationArchiveBuildStep - Indexing dependency: /home/sarxos/.m2/repository/io/smallrye/smallrye-config/1.3.5/smallrye-config-1.3.5.jar
17:52:20.429 [build-4] DEBUG i.q.d.i.ApplicationArchiveBuildStep - Indexing dependency: /home/sarxos/workspace-2019/abberwoult/abberwoult-core/target/test-classes
17:52:20.439 [build-4] DEBUG i.q.d.i.ApplicationArchiveBuildStep - Indexing dependency: /home/sarxos/workspace-2019/abberwoult/abberwoult-annotations/target/abberwoult-annotations-0.0.1-SNAPSHOT.jar
17:52:20.493 [build-4] WARN  i.q.d.i.ApplicationArchiveBuildStep - Re-indexing /home/sarxos/workspace-2019/abberwoult/abberwoult-annotations/target/abberwoult-annotations-0.0.1-SNAPSHOT.jar - at least Jandex 2.1 must be used to index an application dependency
17:52:20.509 [build-8] DEBUG io.quarkus.deployment.index - Index bean class: io.smallrye.config.inject.ConfigProducer
17:52:20.511 [build-8] DEBUG io.quarkus.deployment.index - Index annotation: javax.enterprise.inject.Produces
17:52:20.511 [build-8] DEBUG io.quarkus.deployment.index - Index annotation: javax.enterprise.context.Dependent

👍

sarxos
sarxos approved these changes Apr 3, 2019
View reviewed changes
Copy link
Contributor

@sarxos sarxos left a comment

Choose a reason for hiding this comment

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

Fixes indexing issue. 👍

@mkouba mkouba requested a review from dmlloyd April 3, 2019 17:18
@mkouba mkouba added this to the 0.13.0 milestone Apr 3, 2019
@mkouba
Copy link
Contributor Author

mkouba commented Apr 3, 2019

@sarxos Thanks for verification ;-)

@gsmet gsmet merged commit bb2f499 into quarkusio:master Apr 4, 2019
@gsmet
Copy link
Member

gsmet commented Apr 4, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarxos sarxos sarxos approved these changes

@gsmet gsmet gsmet approved these changes

@dmlloyd dmlloyd Awaiting requested review from dmlloyd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.

NPE from ArcAnnotationProcessor when default value() is used
3 participants
@mkouba @sarxos @gsmet