Skip to content

Introduce fluent API for search options in MergedAnnotations #28208

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

Closed
sbrannen opened this issue Mar 21, 2022 · 5 comments
Closed

Introduce fluent API for search options in MergedAnnotations #28208

sbrannen opened this issue Mar 21, 2022 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Mar 21, 2022

Overview

Inspired by the requirements for implementing #28207, we have decided to introduce a fluent API for search options in MergedAnnotations.

The following is an example of how one would supply search options using the existing API.

MergedAnnotations annotations = MergedAnnotations.from(myClass, SearchStrategy.TYPE_HIERARCHY,
		RepeatableContainers.of(MyRepeatable.class, MyRepeatableContainer.class),
		myCustomAnnotationFilter);

Proposal

For each strategy in SearchStrategy, we will introduce a corresponding find*() method that starts the fluent API. Methods such as usingRepeatableContainers() and withAnnotationFilter() will be optional. The fluent API culminates with an invocation of from(...) which performs the search and returns the MergedAnnotations instance.

With a fluent API, the above can be rewritten as follows.

MergedAnnotations annotations = MergedAnnotations
    .findAnnotationsInTypeHierarchy()
    .usingRepeatableContainers(RepeatableContainers.of(MyRepeatable.class, MyRepeatableContainer.class))
    .withAnnotationFilter(myCustomAnnotationFilter)
    .from(myClass);

For a less involved use case that relies on the defaults for repeatable containers and filtering, the code would reduce to the following.

MergedAnnotations annotations = MergedAnnotations.findAnnotationsInTypeHierarchy().from(myClass);
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Mar 21, 2022
@sbrannen sbrannen added this to the 6.0.0-M4 milestone Mar 21, 2022
@sbrannen sbrannen self-assigned this Mar 21, 2022
@sbrannen
Copy link
Member Author

sbrannen commented Mar 22, 2022

After putting more thought into this, I wonder if it's best to end the fluent API with from(...) instead of a verb or command like search(...), searchFrom(...), etc.

MergedAnnotations already has various from(...) and on(...) methods, so the new fluent API cannot start with either of those.

@philwebb recommended all new factory methods in MergedAnnotations start with the same prefix to make them easily discoverable, which of course makes a lot of sense. So we were thinking of find* and search* as a reasonable, meaningful prefix for these new methods.

However, if the final action in the fluent API is a method named search*(...), it seems a bit odd to have the first method called find*(...) or search*(...).

So, another idea I'm tinkering with is starting with a single static factory method for "search options" like this:

MergedAnnotations annotations = MergedAnnotations.searchOptions()
	.typeHierarchy()
	.repeatableContainers(myRepeatableContainers)
	.annotationFilter(myCustomAnnotationFilter)
	.search(myClass);

One additional (unplanned) benefit of that is that the SearchOptions "builder" instance could actually be saved and reused to perform .search(...) on different classes/methods. However, I'm not sure how useful that would be in practice.

@sbrannen
Copy link
Member Author

Current proposal, based on brainstorming sessions and taking #28207 into account:

MergedAnnotations
	.search(searchStrategy)
	.withEnclosingClasses(ClassUtils::isInnerClass)
	.withRepeatableContainers(repeatableContainers)
	.withAnnotationFilter(annotationFilter)
	.from(myClass);

@philwebb
Copy link
Member

philwebb commented Mar 22, 2022

Ha, I was about to suggest this :)

MergedAnnotations
	.searching(searchStrategy)
	.withEnclosingClasses(ClassUtils::isInnerClass)
	.withRepeatableContainers(repeatableContainers)
	.withAnnotationFilter(annotationFilter)
	.from(myClass);

@philwebb
Copy link
Member

philwebb commented Mar 22, 2022

I think convenience search methods are also worth considering. SearchStrategy tends to be one of the more common things to want to define.

MergedAnnotations.searchingTypeHierarchy().with...().from(myClass)

@sbrannen
Copy link
Member Author

sbrannen commented Mar 22, 2022

I think convenience search methods are also worth considering.

We definitely considered that approach, but the choice of meaningful (and concise) names becomes challenging for any SearchStrategy other than TYPE_HIERARCHY.

MergedAnnotations
	// .searchDirect()
	// .searchInheritedAnnotations()
	// .searchSuperclass()
	.searchTypeHierarchy()
	.withEnclosingClasses(ClassUtils::isInnerClass)
	.withRepeatableContainers(repeatableContainers)
	.withAnnotationFilter(annotationFilter)
	.from(myClass);

The above seems too vague, and the following seems too verbose.

MergedAnnotations
	// .findDirectlyDeclaredAnnotations()
	// .findInheritedAnnotations()
	// .findSuperclassAnnotations()
	.findAnnotationsInTypeHierarchy()
	.withEnclosingClasses(ClassUtils::isInnerClass)
	.withRepeatableContainers(repeatableContainers)
	.withAnnotationFilter(annotationFilter)
	.from(myClass);

In the end, @jhoeller and I decided that it's probably best to let the user supply a SearchStrategy and rely on the documentation for those enum constants to explain things, since people are accustomed to the increasing scope of the strategies in the context of enums; whereas, it becomes a bit more cumbersome to infer that increasing scope based solely on method names like the ones in the two preceding examples.

But... if you have better ideas for how to name all 4 convenience methods, by all means speak up. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants