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

MicroProfileConfigASTValidator handles @ConfigProperties in a wrong way #304

Closed
jeffmaury opened this issue Oct 24, 2022 · 6 comments · Fixed by #308
Closed

MicroProfileConfigASTValidator handles @ConfigProperties in a wrong way #304

jeffmaury opened this issue Oct 24, 2022 · 6 comments · Fixed by #308
Assignees
Labels
bug Something isn't working validation
Milestone

Comments

@jeffmaury
Copy link
Contributor

MicroProfileConfigASTValidator tries to locate @ConfigProperties but it uses only the simple class name and not the fully qualified class name.
As a result, it also works for Quarkus @ConfigProperties but it should located into the Quarkus specific part and it may wrongly process @ConfigProperties annotations from other packages.

@rgrunber
Copy link
Contributor

When I look at

&& AnnotationUtils.isMatchAnnotation((NormalAnnotation) modifier, CONFIG_PROPERTIES_ANNOTATION)) {
it's using
public static final String CONFIG_PROPERTIES_ANNOTATION = "org.eclipse.microprofile.config.inject.ConfigProperties";
, which is fully qualified.

I think quarkus-ls has a separate properties provider to handle ConfigProperties from that end also.

@jeffmaury
Copy link
Contributor Author

Yes agree but if you go into isMatchAnnotation you will find that the annotation type name is ConfigProperties and not the fully qualified name
See my Eclipse debugging session:

image

@rgrunber
Copy link
Contributor

Interesting, so immediately above that is line 186, which is AnnotationUtils.isMatchAnnotation(..)

return annotationName.endsWith(annotation.getTypeName().getFullyQualifiedName());

So annotation.getTypeName().getFullyQualifiedName() is not returning the fully qualified name. Is there any chance the annotation element is not resolved ? Definitely a bug. Would need to investigate to figure out what's going wrong with the annotation.

@rgrunber rgrunber added bug Something isn't working validation labels Oct 25, 2022
@jeffmaury
Copy link
Contributor Author

Interesting, so immediately above that is line 186, which is AnnotationUtils.isMatchAnnotation(..)

return annotationName.endsWith(annotation.getTypeName().getFullyQualifiedName());

So annotation.getTypeName().getFullyQualifiedName() is not returning the fully qualified name. Is there any chance the annotation element is not resolved ? Definitely a bug. Would need to investigate to figure out what's going wrong with the annotation.

Yes because getTypeName() returns a SimpleName probably because the annotation is referenced through @ConfigProperties and not the FQCN

@datho7561 datho7561 self-assigned this Nov 2, 2022
datho7561 added a commit to datho7561/lsp4mp that referenced this issue Nov 8, 2022
Fixes eclipse#304

Signed-off-by: David Thompson <davthomp@redhat.com>
datho7561 added a commit to datho7561/lsp4mp that referenced this issue Nov 8, 2022
Fixes eclipse#304

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 added this to the 0.6.0 milestone Nov 8, 2022
datho7561 added a commit that referenced this issue Nov 8, 2022
Fixes #304

Signed-off-by: David Thompson <davthomp@redhat.com>
@rgrunber
Copy link
Contributor

rgrunber commented Nov 8, 2022

To me it looks like we don't really support validation/completion for the deprecated ConfigProperties annotation from quarkus. Not sure if we need to though given that it should be done via the MP property. Either way, I think David's change should improve things.

@datho7561
Copy link
Contributor

I created redhat-developer/quarkus-ls#759 to talk/fix this. Maybe the right solution is not validating the Quarkus @ConfigProperties at all, but either way, we should remove the tests in lsp4mp that reference it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants