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

Allow creation of Beans that cannot be autowired by type unless qualified #26528

Closed
hjohn opened this issue Feb 9, 2021 · 9 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@hjohn
Copy link

hjohn commented Feb 9, 2021

I'm seeing more and more problems with complex Spring / Spring Boot applications where beans created in some dependency interfere with the operation of either another dependency or our own code.

Dependencies like to declare Beans because it is convenient to inject them where needed in the dependency's own code. However, some of these beans really shouldn't be public and available for general use. By declaring a Bean, it becomes publicly available, even if given its own name or if it is declared with a Qualifier:

 @Bean("myName") @MyQualifer
 public MyBean myBean() { ... }

This bean can still be injected anywhere (when it is the only option) because Spring will match it by type:

 @Autowired private MyBean bean1;

This is highly undesirable for types that are used commonly by multiple dependencies. Some of the biggest offenders are ObjectMapper, ThreadPoolTaskScheduler, etc. Beans like this often get configured in a specific way for a specific dependency and making them public (by simple declaring them) means that these beans can end up in completely unrelated parts of an application.

Spring's injection capabilities are very useful, even in libraries, but just like a library should be able to limit what API is publicly available, it should also be able to decide which of its beans are safe to use in external code (published) and which are not (kept private).

Qualifiers and Bean names are however insufficient due to Spring's matching by type -- a Bean intended for internal use only can still leak out and be used in external code. I therefore suggest allowing Beans to be declared with restrictions on how it can be matched:

 @Bean(value = "myName", name-must-match = true) public MyBean myBean() { ... }

Or:

@Bean(must-be-qualified = true) @MyQualifier public MyBean myBean() { ... }

The first declaration would require the wiring site to specify the bean name:

@Autowired @Qualifer("myName") private MyBean bean1;

The second would require:

@Autowired @MyQualfier private MyBean bean1;

This would NOT match:

@Autowired private MyBean myBean;

In this fashion, library authors can keep certain beans private to the library by keeping the Qualifier private or by using an obscure bean name (use at your own risk in external code).

The suggested names and even the entire mechanism are just for illustration. I could imagine there being different annotations for this purpose: @NamedBean -- bean that must match by name to be considered for injection, and @QualifiedBean -- bean that must match all qualifiers to be considered.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 9, 2021
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Feb 9, 2021
@nightswimmings
Copy link

nightswimmings commented May 26, 2022

@hjohn I requested this long time ago, and I was responded with a very enlightning answer by (was it Andy Wilkinson?).
The best approach to fulfill this need is creating a custom domain object that acts as a wrapper.
For instance, my need was usually @Autoconfiguring something like a Rest/Jms/KafkaTemplate or ObjectMapper, to be customized and used exclusively within the boundary of my domain, but forbidding for qualifying as @primary in the rest of the Boot Autconfiguration ecosystem.

You can then expose a @bean like

@Getter
@RequiredArgsConstructor
DomainRest/Kafka/JmsTemplate.java

   private final DomainRest/Kafka/JmsTemplate domainTemplate;

You can even use Lombok's @DeleGate if you want a 1-to-1 functionally equivalent decorator

This is exactly the same as defining a theoretical @bean(strictQualifying = true) @qualifier("strict-qualif-implementation") + with a @Autowired @qualifier("strict-qualif-implementation") injection

@hjohn
Copy link
Author

hjohn commented May 26, 2022

Sorry, but I don't consider wrapping every bean that I don't want exposed a very good answer to this problem. A much better way to approach this is how CDI does this. Every bean without a qualifier silently gets a default qualifier added to it, and every injection point defined without a qualifier also gets this default qualifier. Now when you define a bean with your own qualifier, it will not match with an injection point that does not explicitly have that qualifier:

  @Bean
  A myBean();   // gets defined as "@Default A"

  @Bean @Red
  A myBean();  // gets defined as "@Red A"

  @Bean @Default @Red
  A myBean();  // gets defined as "@Default @Red A"

  @Autowired A a;  // accepts "@Default A" or "@Default @Red A" but not plain "@Red A"

  @Autowired @Red A a; // accepts "@Red A" or "@Default @Red A" but not plain "@Default A"

@nightswimmings
Copy link

Yes, its my same need. In a way, I can understand what I was told, i.e, that if you need to strict qualify Red A everywhere, it is not an A anymore, it is a RedA and must be injected as such. Kinda Liskov.

@hjohn
Copy link
Author

hjohn commented Jun 8, 2022

I've been bitten by this problem again in a multi module project. When upgrading a library, a bean which was clearly not intended to be used for normal use was injected into an injection point that just expected a default version of that bean.

I've taken the time to construct a solution which is somewhat similar to what CDI offers, but in order to remain backwards compatible with how Spring selects candidates by default (given the many projects out there) I've flipped the logic around. With the AutowireCandidateResolver below it is possible to annotate a canditate with a NotDefault annotation. This prevents the candidate from being injected into injection points without any annotations (or if it has annotations, they must match what the candidate specifies as usual). This makes it possible to prevent certain candidates from being used as default. A bean defined as:

  @Bean @NotDefault @Red
  ARedBean myRedBean() { ... }

Will only get injected into an injection point that specifies the @Red annotation. Without the @NotDefault annotation it would get injected into any injection point that matches the type.

If anyone would like to toy with this, define a configuration to use an alternative AutowireCandidateResolver:

@Configuration
public static class Config {
    @Bean
    public CustomAutowireConfigurer autowireConfigurer(DefaultListableBeanFactory beanFactory) {
       CustomAutowireConfigurer configurer = new CustomAutowireConfigurer();
       beanFactory.setAutowireCandidateResolver(new NotDefaultSupportingQualifierAnnotationAutowireCandidateResolver());
       configurer.postProcessBeanFactory(beanFactory);
       return configurer;
    }
}

And then use this implementation:

/**
 * {@link AutowireCandidateResolver} implementation based on {@link ContextAnnotationAutowireCandidateResolver}
 * that allows candidates to be annotated with a {@link NotDefault} annotation to indicate they should not
 * be wired into injection points which have no annotations. If the injection points has at least one annotation
 * which also matches the candidate, injection is allowed as normal.
 */
public class NotDefaultSupportingQualifierAnnotationAutowireCandidateResolver extends ContextAnnotationAutowireCandidateResolver{
    private static final Annotation NOT_DEFAULT = AnnotationUtils.synthesizeAnnotation(NotDefault.class);

    /**
     * Match the given qualifier annotations against the candidate bean definition.
     */
    @Override
    protected boolean checkQualifiers(BeanDefinitionHolder bdHolder, Annotation[] annotationsToSearch) {
        if (super.checkQualifiers(bdHolder, annotationsToSearch)) {

            /*
             * The qualifiers matched according to standard rules. If there were any custom annotations
             * present on the injection point (aside from the Default annotation) then accept this result.
             */

            if (annotationsToSearch != null) {
                for (Annotation annotation : annotationsToSearch) {
                    if (annotation.annotationType() != Default.class && isQualifier(annotation.annotationType())) {
                        return true;
                    }
                }
            }

            /*
             * There were no custom annotations on the injection point, or there was only a Default annotation.
             * This means the injection point expects a default candidate. Any candidate is a default candidate
             * unless specifically annotated with NotDefault:
             */

            return !checkQualifier(bdHolder, NOT_DEFAULT, new SimpleTypeConverter());
        }

        return false;
    }
}

And these two annotations:

@Documented
@Qualifier
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, ElementType.ANNOTATION_TYPE})
public @interface NotDefault {
}

@Documented
@Qualifier
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, ElementType.ANNOTATION_TYPE})
public @interface Default {
}

@hjohn
Copy link
Author

hjohn commented Oct 4, 2023

@sbrannen @wilkinsona

I would be willing to contribute a PR for this as I have a working solution already, but due to how AutowireCandidateResolvers uses inheritance to contribute additional features I can't quite solve this 100% by just extending QualifierAnnotationAutowireCandidateResolver (it works correct for field injection only due to how qualifiers are checked twice for other forms of injection).

Please advise if you'd are interested in such a feature at all, and what the chances are for it to get accepted. A description of the feature follows:

Problem

Spring currently has no trivial mechanism to prevent autowiring of beans that happen to match the criteria of an unqualified injection point. This means that if some internal bean happens to be part of the context that happens to be of the same type as an injection point expects, it will be autowired even though the bean may be very specialized for a specific purpose.

This is a particular problem for large projects where multiple artifacts are combined that may be contributing (intended or not) beans to the overall context. Some of these artifacts may just want to use Spring injection for their own wiring purposes, but have no interest in having their beans also be used by other artifacts.

Solution

In CDI this is prevented by giving all injection points and beans a magic qualifier @Default if they do not have any other qualifier already. This means that a "naked" injection point has the qualifier @Default and a Bean with qualifier @Red will never match it. In Spring however the injection point has no qualifiers at all, and so a @Red Bean is allowed to be injected.

Solving this in the same manner as CDI does is not possible for Spring as it would break a lot of injections when they suddenly have a @Default qualifier added to them. As said, a @Red Bean would no longer be injected where before it would have been. However, we can introduce a mechanism where this logic is inverted without impacting any existing injections -- beans could be annotated with a special annotation @NotDefault -- when this annotation is present on the bean definition, it is not eligible for injection into injection points that have no qualifiers. In effect this means that there must be a qualifier present for this bean to be injected (and the qualifier must match the qualifiers specified on the bean as per normal).

Example

Consider these two beans:

 @Bean ObjectMapper objectMapperBean();
 @Bean @NotDefault @Encrypted ObjectMapper encryptedObjectMapperBean();

Given the injection points:

 1: @Autowired private ObjectMapper mapper1;
 2: @Autowired @Encrypted private ObjectMapper mapper2;
 3: @Autowired @Encrypted @Red private ObjectMapper mapper3;

... only objectMapperBean would qualify for mapper1. encryptedObjectMapperBean does not qualify as it doesn't allow unqualified use. For mapper2 only encryptedObjectMapperBean would qualify per normal rules as the qualifier @Encrypted must be present. For mapper3 nothing qualifies as per normal rules (all qualifiers must be present on the bean definitions considered).

Falling back to bean names

I'm aware that Spring can take the field or parameter name into account to differentiate between beans of the same type; however, I think this practice should be discouraged as it is all too easy to rename a field (for improved readability or some other reason) only to discover that this has an impact on which bean is selected by autowiring. This can all too easily lead to situations where everything seems to be in order (with a bean that works very similar) but may break when presented with edge cases or extra load (consider a synchronized vs unsynchronized bean, or an ObjectMapper that only handles date time fields differently than normal).

Alternative Syntax

@NotDefault could be named @Private or we could even introduce an annotation @PrivateBean that automatically has the @NotDefault qualifier.

Implementation

The implementation basically only needs to override the checkQualifiers method in AutowireCandidateResolver (but read on for a subtle problem):

private static final Annotation NOT_DEFAULT = AnnotationUtils.synthesizeAnnotation(NotDefault.class);

@Override
protected boolean checkQualifiers(BeanDefinitionHolder bdHolder, Annotation[] annotationsToSearch) {
    if (!super.checkQualifiers(bdHolder, annotationsToSearch)) {
        return false;
    }

    /*
     * The qualifiers (if any) on the injection point matched the candidate's qualifiers according
     * to the standard rules (a candidate always must have at least all qualifiers specified by the
     * injection point).
     */

    if (annotationsToSearch != null) {

        /*
         * If there was at least one qualifier on the injection point then proceed with injection 
         * (note: we only need to find if a qualifier was *present* here, as all were already matched 
         * by checkQualifiers at the start of this method).
         */

        for (Annotation annotation : annotationsToSearch) {
            Class<? extends Annotation> annotationType = annotation.annotationType();

            if (isQualifier(annotationType)) {
                return true;
            }
        }
    }

    /*
     * There were no qualifiers on the injection point at all. This means the injection point expects
     * a default candidate. Any candidate is a default candidate unless specifically annotated with NotDefault:
     */

    return !checkQualifier(bdHolder, NOT_DEFAULT, new SimpleTypeConverter());
}

The above code would be the extent of the changes, if it weren't for one small issue; checkQualifiers is sometimes called twice, once with the qualifiers on an argument, and once with the qualifiers on the method that contains it. For fields this is not an issue, as it is only called once, but in all other cases the second call (with the qualifiers on the method) often doesn't have any qualifiers and so the @NotDefault logic kicks in and doesn't allow the match even though the argument may have had the correct qualifiers.

This is because currently in Spring, the two checkQualifiers calls function like an AND, both must be true for the bean to be a valid candidate. To solve this, a small change would be needed in QualifierAnnotationAutowireCandidateResolver so that it calls checkQualifiers only once with the combined set of qualifiers (on the argument and on the method). This should not impact any existing injections, but will make it possible to support the @NotDefault annotation.

Introducing an @Any annotation

With a small additional effort, an @Any annotation can also be supported that can be used to mark an injection point to accept anything, even @NotDefault beans. This can be useful when a list of all beans of a specific type is needed (for debugging perhaps), even beans considered private. Using the above bean definitions again, the following injection points:

 @Autowired List<ObjectMapper> mappers1;
 @Autowired @Any List<ObjectMapper> mappers2;

... would have 1 bean in the list for mappers1, and all ObjectMapper beans in mappers2.

Work Arounds

Work arounds are:

  1. Ensure field/argument names match the bean name; this is possible to a certain extend but not always (it gets annoying when using the FullyQualifiedAnnotationBeanNameGenerator for example).
  2. Ensure all beans have some kind of qualifier, just in case another project or artifact has a bean of the same type.
  3. Use delegates or wrappers around often duplicated beans so they are of a different type and therefore can't be injected in injection points that use the unwrapped type; note that it is insufficient to just subclass the bean, it must be a delegate or wrapper

None of these strike me as particularly good, and so I think having a @NotDefault annotation would a great addition to Spring's autowiring.

@wilkinsona
Copy link
Member

Please advise if you'd are interested in such a feature at all, and what the chances are for it to get accepted

I am not a member of the Spring Framework team so it is not for me to say.

@jhoeller jhoeller added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 21, 2023
@jhoeller jhoeller added this to the 6.2.x milestone Dec 21, 2023
@jhoeller jhoeller self-assigned this Dec 21, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Feb 5, 2024

I'm considering a variation of this in the form of declaring specific qualifier types as mandatory, e.g. through @Qualifier(mandatory = true), in which case injection points would be forced to declare that qualifier in order to match a bean annotated with that qualifier. So instead of backing out in case of @NotDefault on a bean definition, we'd check for any qualifier annotation that is mandatory on that bean, enforcing a match for those on the injection point.

In 6.2, we got the opportunity to introduce this as part of a deep revisit of the autowiring algorithm: #28122

@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.0-M1 Feb 5, 2024
@jhoeller
Copy link
Contributor

I eventually went with an approach closer to the original proposal, aligned with the existing autowireCandidate flag: A bean definition comes with a defaultCandidate flag now, as a variation of autowireCandidate which does not disable injection in general, just enforces an additional indication such as a qualifier. This is similar to the @NotDefault proposal, just as a flag instead of a pseudo-qualifier annotation for that purpose... which is semantically better aligned with Spring's qualifier handling.

As for an @Any indication at an injection point, this does not seem like a common enough need in Spring scenarios to require dedicated support for @Autowired injection. If all type matches are needed for some reason, there is always ListableBeanFactory.getBeansOfType which completely bypasses the candidate settings (autowireCandidate and now also defaultCandidate) , always retrieving all beans of the requested type.

@hjohn
Copy link
Author

hjohn commented Feb 20, 2024

Thanks a lot, very happy with the new feature, in whatever form fits best with Spring's model, and I'm looking forward to playing with it. The @Any is indeed really not that useful, only included it as @Default and @Any go together a bit in CDI.

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

6 participants