eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
159 stars 129 forks source link

Duplicate Annotation Error for Records #1092

Closed vladoiliev02 closed 1 year ago

vladoiliev02 commented 1 year ago

The following code compiles successfully using the eclipse compiler and throws a java.lang.annotation.AnnotationFormatError: Duplicate annotation... exception on runtime. The same code compiled with a different compiler has no issues. This leads me to believe that this is an issue with the eclipse implementation of the java compiler. Using a decompiler on the '.class' file shows that really the @NotBlank annotation has been repeated inside the type parameter of the List post compilation.

@JsonInclude(JsonInclude.Include.NON_NULL)
public record Record (
        @Valid
        Record2 record2,

        @Valid
        Record3 record3,

        @Valid
        Record1 record1
) {
}
public record Record1(
        @NotEmpty(message = "Not empty Record1 strings")
        List<@NotBlank(message = "Not blank Record1 string") String> strings,

        @NotBlank(message = "Not blank Record1 str")
        String str
) {
}
public record Record2(
        @NotEmpty(message = "Not empty Record2 strings")
        List<@NotBlank(message = "Not blank Record2 string") String> strings,

        @NotBlank(message = "Not blank Record2 str")
        String str
) {
}
public record Record3(
        @NotEmpty(message = "Not empty Record3 strings")
        List<@NotBlank(message = "Not blank Record3 string") String> strings,

        @NotBlank(message = "Not blank Record3 str")
        String str
) {
}
@RestController
public class TestController {

    @PatchMapping("/request")
    public ResponseEntity<?> patch(@RequestBody @Valid Record dto) {
        return ResponseEntity.accepted().build();
    }
}

The problem occurs when calling the patch endpoint in the controller above. The exception occurs during validation of the dto.

Exception:

java.lang.annotation.AnnotationFormatError: Duplicate annotation for class: interface javax.validation.constraints.NotBlank: @javax.validation.constraints.NotBlank(message="Not blank Record2 string", payload={}, groups={})
    at java.base/sun.reflect.annotation.TypeAnnotationParser.mapTypeAnnotations(TypeAnnotationParser.java:387) ~[na:na]
    at java.base/sun.reflect.annotation.AnnotatedTypeFactory$AnnotatedTypeBaseImpl.<init>(AnnotatedTypeFactory.java:152) ~[na:na]
    at java.base/sun.reflect.annotation.AnnotatedTypeFactory.buildAnnotatedType(AnnotatedTypeFactory.java:67) ~[na:na]
    at java.base/sun.reflect.annotation.AnnotatedTypeFactory$AnnotatedParameterizedTypeImpl.getAnnotatedActualTypeArguments(AnnotatedTypeFactory.java:433) ~[na:na]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.findTypeArgumentsConstraints(AnnotationMetaDataProvider.java:766) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.findTypeAnnotationConstraints(AnnotationMetaDataProvider.java:619) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.findExecutableMetaData(AnnotationMetaDataProvider.java:336) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.getMetaData(AnnotationMetaDataProvider.java:292) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.getMethodMetaData(AnnotationMetaDataProvider.java:279) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.retrieveBeanConfiguration(AnnotationMetaDataProvider.java:130) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider.getBeanConfiguration(AnnotationMetaDataProvider.java:120) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.BeanMetaDataManagerImpl.getBeanConfigurationForHierarchy(BeanMetaDataManagerImpl.java:234) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.BeanMetaDataManagerImpl.createBeanMetaData(BeanMetaDataManagerImpl.java:201) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.metadata.BeanMetaDataManagerImpl.getBeanMetaData(BeanMetaDataManagerImpl.java:165) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.engine.ValidatorImpl.buildNewLocalExecutionContext(ValidatorImpl.java:772) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.engine.ValidatorImpl.validateCascadedAnnotatedObjectForCurrentGroup(ValidatorImpl.java:627) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.engine.ValidatorImpl.validateCascadedConstraints(ValidatorImpl.java:590) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.engine.ValidatorImpl.validateInContext(ValidatorImpl.java:409) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.hibernate.validator.internal.engine.ValidatorImpl.validate(ValidatorImpl.java:172) ~[hibernate-validator-6.2.5.Final.jar:6.2.5.Final]
    at org.springframework.validation.beanvalidation.SpringValidatorAdapter.validate(SpringValidatorAdapter.java:109) ~[spring-context-5.3.27.jar:5.3.27]
    at org.springframework.boot.autoconfigure.validation.ValidatorAdapter.validate(ValidatorAdapter.java:66) ~[spring-boot-autoconfigure-2.7.12.jar:2.7.12]
    at org.springframework.validation.DataBinder.validate(DataBinder.java:933) ~[spring-context-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodArgumentResolver.validateIfApplicable(AbstractMessageConverterMethodArgumentResolver.java:250) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor.resolveArgument(RequestResponseBodyMethodProcessor.java:139) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:122) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:179) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:146) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:117) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:895) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:808) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1072) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:965) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1006) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:880) ~[spring-webmvc-5.3.27.jar:5.3.27]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:623) ~[tomcat-embed-core-9.0.75.jar:4.0.FR]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:209) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:51) ~[tomcat-embed-websocket-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:117) ~[spring-web-5.3.27.jar:5.3.27]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:93) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:117) ~[spring-web-5.3.27.jar:5.3.27]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201) ~[spring-web-5.3.27.jar:5.3.27]
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:117) ~[spring-web-5.3.27.jar:5.3.27]
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:178) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:153) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:167) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:90) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:481) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:130) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:93) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:390) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:63) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:926) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1791) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) ~[tomcat-embed-core-9.0.75.jar:9.0.75]
    at java.base/java.lang.Thread.run(Thread.java:833) ~[na:na]
srikanth-sankaran commented 1 year ago

Attempting to reduce to smaller test case shows some additional interesting incorrect behavior:

import java.util.List;

@interface NotBlank {
    String message();
}

record Record1(
        List<@NotBlank(message = "Not blank Record1 string") String> strings,
        @NotBlank(message = "Not blank Record1 str") String str) {
}

public class X {
    List<@NotBlank(message = "Not blank Record1 string") String> strings;
    public static void main(String[] args) {
        foo( new Record1(List.of(""), "Blah"));
    }
    static void foo(Record1 r) {}
}

This code compiled with eclipse master while javac correctly rejects it with:

X.java:8: error: annotation @NotBlank not applicable in this type context
        List<@NotBlank(message = "Not blank Record1 string") String> strings,
             ^
X.java:13: error: annotation @NotBlank not applicable in this type context
        List<@NotBlank(message = "Not blank Record1 string") String> strings;
             ^
2 errors
srikanth-sankaran commented 1 year ago

Hello @vladoiliev02 , can you provide a self contained complete example that shows the problem ? This is crucial - I don't want to fill in missing details (such as annotation targets etc)

Looks at first glance, ECJ went ahead and implemented some spec change that javac hasn't caught up with: see https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082 and https://bugs.openjdk.org/browse/JDK-8231435 and in particular the comment https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082#c6 which behavior difference I still see.

Given all this, it is best to work with a test case that shows the problem fully without my having to guess and fill in the blanks.

@stephan-herrmann and @jarthana - FYI.

That the code in https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082#c6 still doesn't compile in javac and does with ecj worries me - I'll ask through my channels what exactly is going on there in JDK/javac

srikanth-sankaran commented 1 year ago

I worry that we have gone ahead and implemented something overeagerly in https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082

JLS20 9.6.4.1 @Target ... still reads:

... If an annotation of type java.lang.annotation.Target is not present on the declaration of an annotation interface A, then A is applicable in all declaration contexts and in no type contexts.

Digging up a bit more, JLS and javac changed course via https://bugs.openjdk.org/browse/JDK-8261610 pulling back from https://bugs.openjdk.org/browse/JDK-8231435 with this text:

...
Per https://mail.openjdk.java.net/pipermail/compiler-dev/2021-February/016321.html the desired meaning of no-@Target is "all declaration contexts". This meaning includes:
- the type parameter declaration context added in Java SE 8 (and excluded from no-@Target by JSR 308, wrongly),
- the module declaration context added in Java SE 9, and
- the record component declaration context added in Java SE 16.
As further kinds of declaration are added to Java, it is intended that "all declaration contexts" will make annotations without an explicit @Target be applicable to those new kinds of declarations.

This meaning overrules JDK-8231435, which in 2019 expanded the meaning from:
  "all declaration contexts that were present in Java SE 7, and no type contexts"
to:
  "all declaration contexts and all type contexts".
The 2019 expansion was intended to put type annotations on an equal footing with declaration annotations, but was subsequently seen as a step too far given the disjoint roles typically played by type annotations and declaration annotations. A secondary concern arose in relation to the corner case where annotations in certain ambiguous locations are treated as both type annotations and declaration annotations -- this behavior is long standing and well specified, but bringing all the no-@Target annotations into its orbit was seen as undesirable from a JDK implementation/testing point of view.

Specifically, 9.6.4.1 should say: "If an annotation of type java.lang.annotation.Target is not present on the declaration of an annotation interface A, then A is applicable in all declaration contexts and in no type contexts." In effect, JSR 308 had the right idea to disallow type contexts, but went too far in trying to constrain the allowed declaration contexts.
...
srikanth-sankaran commented 1 year ago

I'll work on reversing course for ECJ - I think that has a strong influence on the current ticket.

@vladoiliev02 - to be 100% sure, I will need a standalone test case that compiles and runs (with failure) - TIA. You are in a better position to stub out portions of code to come up with a minimal test case than I am

srikanth-sankaran commented 1 year ago

Well, JLS certainly reversed course - but looks like javac never implemented https://bugs.openjdk.org/browse/JDK-8231435 in the first place ? All of javac 14-19 reject this code:

@interface Simple {}
class Test { <@Simple T> void m(T arg1) {} }

while we compile this post https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082

srikanth-sankaran commented 1 year ago

Well, JLS certainly reversed course - but looks like javac never implemented https://bugs.openjdk.org/browse/JDK-8231435 in the first place ? All of javac 14-19 reject this code:

@interface Simple {}
class Test { <@Simple T> void m(T arg1) {} }

while we compile this post https://bugs.eclipse.org/bugs/show_bug.cgi?id=552082

Javac has fixed this for JDK21 here: https://bugs.openjdk.org/browse/JDK-8304169

vladoiliev02 commented 1 year ago

I have come up with a self contained example, although I am not sure why the exception is not thrown, but looking at the decompiled .class file, the error is still persisting. Here is the code:

@Target({ElementType.TYPE_USE})
@interface Annotation {
}

record Record(
        @Annotation
        List<@Annotation String> strings
) {
}

public class Main {
    public static void main(String[] args) throws Exception {
        Record record = new Record(List.of(""));
    }
}

And here is a picture of the decompiled file: image

The annotation is still being repeated which will eventually cause the same problem from the above example with Spring boot.

Here it also does not matter if the annotation on the list and inside on the parameter are the same or are different annotations. The result is the same - the one inside on the generic parameter gets repeated.

vladoiliev02 commented 1 year ago

" If an annotation of type java.lang.annotation.Target is not present on the declaration of an annotation interface A, then A is applicable in all declaration contexts and in no type contexts. " - from your previous reply. I do not think that this is causing the issue since the original @NotBlank annotation from the javax validation API has a @Target.

vladoiliev02 commented 1 year ago

Looking at the first exception I have sent the problem occurs when the validation library is iterating over the annotations on the different fields, it then sees the duplicate annotation we see in the picture above and throws the above provided exception. I don't really understand how to get the annotations and iterate over them, but if you are able to do that you will definitely see that the annotations in the generic parameter have doubled.

srikanth-sankaran commented 1 year ago

Thanks, I will follow up - yes with the shortened test case, it looks likely an orthogonal issue.

vladoiliev02 commented 1 year ago

I was able to come up with a test case that fails:

@Target(ElementType.TYPE_USE)
@Retention(RetentionPolicy.RUNTIME)
@interface Ann {

}

record Record(
    @Ann
    List<@Ann String> list
) {

}

class DuplicateAnnotationTest {

    @Test
    void testNotDuplicatingAnnotationDoesNotThrow() throws Exception {
        AnnotatedParameterizedType listMethodReturnType = (AnnotatedParameterizedType)
                    Record.class.getDeclaredMethod("list").getAnnotatedReturnType();

        assertDoesNotThrow(listMethodReturnType::getAnnotatedActualTypeArguments, 
                "Should not throw AnnotationFormatError.");
    }
}

This test case fails because of the duplicate annotation we see in the decompiled .class file. If it was a Map.class not a List.class, the annotations on both of the map's parameters are duplicated.

srikanth-sankaran commented 1 year ago

No, that test case doesn't demonstrate a crash due to duplicate annotation, It refuses to compile :-) This one does:

import java.lang.annotation.*;
import java.lang.annotation.Target;
import java.util.List;
import java.lang.reflect.AnnotatedParameterizedType;

@Target(ElementType.TYPE_USE)
@Retention(RetentionPolicy.RUNTIME)
@interface Ann {
}

record Record(
    @Ann
    List<@Ann String> list
) {
}

public class X {

    static void assertDoesNotThrow(Runnable exe, String message) {
        exe.run();
    }

    public static void main(String [] args) throws Exception {
        AnnotatedParameterizedType listField = (AnnotatedParameterizedType) Record.class.getDeclaredMethod("list").getAnnotatedReturnType();
        assertDoesNotThrow(listField::getAnnotatedActualTypeArguments, "Should not throw duplicate annotation exception.");
    }
}

Thanks a lot!

srikanth-sankaran commented 1 year ago

Looks like a bad side effect of https://bugs.eclipse.org/bugs/show_bug.cgi?id=571905

srikanth-sankaran commented 1 year ago

Fix under consideration : https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1101

vladoiliev02 commented 1 year ago

When will I be able to see the changes in my IDE?

srikanth-sankaran commented 1 year ago

I expect to integrate the fix early next week - for 4.29 M1.

vladoiliev02 commented 1 year ago

I expect to integrate the fix early next week - for 4.29 M1.

Wait, does this mean it will be GA in September?

iloveeclipse commented 1 year ago

Wait, does this mean it will be GA in September?

You could use nightly SDK build if you like. I do it every day since years, and in most cases it just works.