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

Use AnnotatedElementUtils instead of AnnotationUtils wherever feasible [SPR-13440] #18020

Closed
spring-issuemaster opened this issue Sep 7, 2015 · 6 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Sep 7, 2015

Sam Brannen opened SPR-13440 and commented

Status Quo

Spring Framework 4.2 introduced support for explicit annotation attribute overrides via @AliasFor; however, due to historical reasons, the Spring code base typically uses the lookup methods in AnnotationUtils which do not support attribute overrides.

For example, although Spring MVC supports @RequestMapping as a merged annotation, the same is not true for @ResponseStatus. @ResponseStatus is in fact supported as a meta-annotation, just not as a merged annotation. This means that the responseStatus attribute override in the following custom composed @Post annotation is currently unsupported even though it is declared syntactically correct. Furthermore, use of the responseStatus will fail silently: it will simply be ignored (see #18021).

@RequestMapping(method = POST)
@ResponseStatus
@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface Post {

  @AliasFor(annotation = RequestMapping.class, attribute = "path")
  String[] value() default {};

  @AliasFor(annotation = RequestMapping.class, attribute = "path")
  String[] path() default {};

  @AliasFor(annotation = ResponseStatus.class, attribute = "code")
  HttpStatus responseStatus() default HttpStatus.CREATED;
}

Deliverables

  1. Determine which annotations in the Spring Framework should be supported as merged annotations (i.e., support attribute overrides).
  2. Migrate from lookups in AnnotationUtils to methods such as findAllMergedAnnotations(), findMergedAnnotation(), or getMergedAnnotation in AnnotatedElementUtils wherever feasible.

Affects: 4.2 GA

Issue Links:

  • #18028 Provide a mechanism for composed annotations to signal that they want to override attributes
  • #18022 Introduce predefined composed annotations in core Spring
  • #16140 Document Spring Annotation Programming Model in the Wiki
  • #18630 Allow @Qualifier to be used in composed annotations with attribute overrides
  • #19208 Optimize ordered event listener performance
  • #18021 Support @ResponseStatus as a merged composed annotation
  • #18047 Support @CrossOrigin as a merged composed annotation
  • #18054 Support @Cache* as merged composed annotations
  • #18376 Support @AliasFor for @JmsListener attributes

Referenced from: commits 63115ed, c5b318a, 5025c61

0 votes, 5 watchers

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Sep 21, 2015

Sam Brannen commented

Note that piecemeal work in this direction has already been performed in conjunction with #18047, #18054, and #18021.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 23, 2016

Juergen Hoeller commented

After a full pass through the codebase for use of AnnotationUtils.findAnnotation, I've migrated most of the remaining calls to AnnotatedElementUtils.findMergedAnnotation, as far as feasible. I've also optimized the latter a bit for the common scenario of an immediately present annotation and introduced a hasAnnotation version (with find semantics) as a more efficient replacement for findMergedAnnotation != null. There is no direct cache yet as for AnnotationUtils.findAnnotation; we'll see whether we need that here as well.

Juergen

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Mar 23, 2016

Sam Brannen commented

Thanks for doing the grunt work here, Juergen. Much appreciated!

Now I just have to see if I can merge with your changes. ;)

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 11, 2016

Gareth Chapman commented

Hi there. I notice the calls in org.springframework.integration.aop.MethodAnnotationPublisherMetadataSource were not migrated from AnnotationUtils.findAnnotation to AnnotatedElementUtils.findMergedAnnotation. Is there a particular reason for this or were they overlooked? If the latter I'd be happy to raise an issue and a PR. If the former I'd be interested to know the reason, as I'd find support for annotation explicit overrides useful. Many thanks.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 11, 2016

Sam Brannen commented

Hi Gareth Chapman,

This is the Spring Framework issue tracker. Thus, this issue was only related to code in Core Spring.

So, yes, please open an issue against the "Spring Integration" project to address your concerns regarding the MethodAnnotationPublisherMetadataSource there.

Thanks!

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 11, 2016

Gareth Chapman commented

Hi Sam, many thanks for your quick response. I'll go ahead and open that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.