Config injection happens too soon

[martin-sladecek]

Environment Details

• Helidon Version: 1.0.3
• Helidon MP
• JDK version: 11.0.2
• OS: Linux
• Docker version (if applicable):

---

Problem Description

When using @Alternative bean (probably also applies to CDI bean specialization), the Config injection attempt in ConfigCdiExtension happens before @Alternative bean is evaluated, so the original bean is also instantiated and the Configs are injected into it. This is undesired as the original bean might use configuration that is not present in the current environment, leading to an exception.

Steps to reproduce

1. Create @Alternative CDI bean with higher priority to any existing bean
2. Inject non-existent config property into original bean

[ljnelson]

If I understand correctly, your issue, rephrased, is basically something like this: our portable extension evaluates which InjectionPoints to process and then blindly processes them during AfterBeanDiscovery, without taking into account whether their "containing" bean is enabled or not. The result is that we might be setting up producers for these "slots" without taking into consideration whether those slots belong to an enabled bean—they might belong to a bean that is superseded by an alternative.

That makes sense in general; while we create far too many producers of ConfigProperty values, doing so here doesn't really result in any harm.

But, it looks also like our portable extension validates injection points at AfterBeanValidation time not by calling BeanManager#validate(InjectionPoint) as should be the case but by somewhat oddly pre-instantiating beans and seeing if such instantiation fails. This is clearly wrong and is a bug.

Thanks for using Helidon MP and for your report.

[ljnelson]

Taking some notes here.

The MicroProfile Config specification often leaves some things up to interpretation.

In this case, the only validation guidance we get is kind of a double negative. The javadocs for ConfigProperty say, in part, regarding the injection of Optional configuration values: "Contrary to natively injecting, if the property is not specified, this will not lead to a DeploymentException." From this we can infer that the specification requires us to run some validation logic at AfterDeploymentValidation time. This makes sense. The TCK also looks for such DeploymentExceptions to be thrown if a requested configuration property value is not available; I'm not sure this is actually spelled out in the specification anywhere. Fine.

This means, though, that we must not simply gather injection points via the seemingly obvious ProcessInjectionPoint container lifecycle event (as we do incorrectly today), as these events happen before the container sorts through all the beans and considers alternatives, vetoes, and other whittling-down-of-the-overall-bean-population features.

So we need to instead track InjectionPoints at ProcessBean time, since ProcessBean events are fired once the container has determined the set of active and enabled beans (including selected alternatives).

However, that still doesn't get us all the way there as it is possible to, for example, have a @ConfigProperty-annotated injection point in an observer method. These injection points as well need to be validated—the specification has nothing to say one way or the other here, but does speak airily of "all injection points" throughout, and since any extra parameters to an observer method beyond its observed event parameter are called injection points by the CDI specification, it follows that we must validate these as well.

The problem here is that the ObserverMethod object does not have a getInjectionPoints() method, even though it probably should. So we need to comb through its parameters, weed out the observed event parameter, and then make sure that any other parameters we find that are qualified with @ConfigProperty get InjectionPoints created for them.

Then at AfterBeanDiscovery time, we can now trust that our collection of harvested injection points are correct for all active and enabled beans—including producer fields, producer methods, interceptors, decorators, managed beans—and observer methods. So we can set up producers properly for these "slots".

Finally, at AfterDeploymentValidation time, we should have enough information to validate that each appropriate slot will be filled without actually eagerly instantiating the bean hosting the configuration value.

I should be able to make progress here relatively quickly.

[spericas]

@ljnelson Would it make sense to create some utility classes so that we can reuse this logic in other CDI extensions in Helidon? The metrics extension, as an example, also observes ProcessInjectionPoint.

[ljnelson]

Yes, if using the utility class in a portable extension is not more work than just doing the work in a portable extension. One way or the other we need to ensure this kind of logic is used throughout Helidon wherever it touches CDI.

[ljnelson]

Taking more notes….

The current extension installs a lot of beans. But because @ConfigProperty-annotated injection points should receive Dependent-scoped objects, there's no need to do this, and they just sit around taking up memory.

Specifically, IMHO the extension should:

• gather injection points by observing ProcessBean and ProcessObserverMethod, not by observing ProcessInjectionPoint or anything else; otherwise we're needlessly processing what may turn out to be disabled effectively vetoed injection points
• keep track of the set of types to which all return types of all relevant injection points belong
• keep track of the sets of qualifiers—which include ConfigProperty instances—factoring in @NonBinding elements and such to which the sets of qualifiers of these injection points belong
• create a bean for a set of types and a set of qualifiers that produces configuration property values
• have that bean use the "get the current InjectionPoint" idiom detailed below
• eliminate all use of things like ConfigQLiteral and whatnot in the code

It would be far better to have fewer beans. Dependent producers can acquire a reference to the current InjectionPoint, yielding all the injection information they need. Thus fewer beans could produce all configuration information and this extension can be radically, radically simplified.

From within a custom Bean implementation whose scope is Dependent, the idiom is to ask the BeanManager for an injectable reference of type InjectionPoint:

final BeanManager beanManager = CDI.current().getBeanManager();
assert beanManager != null;
final InjectionPoint currentInjectionPoint = (InjectionPoint)beanManager.getInjectableReference(new CurrentInjectionPoint(InjectionPoint.class), context);
assert currentInjectionPoint != null;

…where CurrentInjectionPoint is a hypothetical InjectionPoint implementation that returns the result of Collections.singleton(InjectionPoint.class) from its getTypes() method, and Collections.singleton(Default.class) from its getQualifiers() method (well, or the AnnotationLiteral equivalent).

That InjectionPoint reference will then provide access to the Type of the field or parameter that is receiving the configuration property value.

The net effect is that there should be one bean per set of qualifiers (taking into account @NonBinding elements) and types instead of an explosion of such beans.

It is also easy enough while we're doing this to "get out of the way" of any user-supplied beans that indicate they want to produce this information; we should function as defaults.

[ljnelson]

This gets more complicated every day. CC:ing @martin-sladecek.

I've refactored the internals of our CDI portable extension to ensure first of all that only non-vetoed-beans' injection points are processed. And I've changed our validation routine to make use of CDI's internal injection point resolution mechanics instead of doing it ourselves. These are both changes that needed to be made anyway.

Now I'm at a place where I can write a unit test. In my first pass, I put together a test exactly as described, viz. an @Alternative selected for the entire application (i.e. with a higher @Priority than the non-alternative bean) is set up, and…the "shadowed" non-alternative bean's injection points are still validated (and following the issue report one of them is deliberately set up to name a configuration property for which a value is not available, so deployment fails). This is indirectly due to the fact that—initially surprisingly to me—the "shadowed" bean shows up at ProcessBean time (the specification indicates that only enabled beans should "get through" to this lifecycle event).

It turns out, after a conversation with Matěj Novotný that there is no way to determine whether an alternative is selected until actual runtime, i.e. long after the AfterDeploymentValidation event where Microprofile Config's specification mandates we do our validation. So sadly validation works as it is required to, i.e. this is an issue of sorts with the MicroProfile specification.

Workarounds obviously include supplying default values in the @ConfigProperty annotations or in META-INF/microprofile-config.properties resources, but I understand this simply punts your problem down a level.

To help, I went a step or two further: suppose your alternative is actually a specializer, i.e. you have two beans, A and B, and instead of making B an alternative for A, you decide to have B extend A and you also mark B with @Specializes. This mechanism should ensure that A is never used throughout your application, and B is, and IMHO in such a case we shouldn't be setting up producers for A's injection points. I'm currently addressing this case because it needs to be addressed anyway and might help @martin-sladecek make headway here.

[ljnelson]

It now occurs to me that specialization won't help either for at least two reasons:

1. To specialize a bean, you have to extend it. If you extend it, the extended bean might very well have injection points that have to be satisfied during a super() call. So they cannot be ignored.
2. ProcessBean events do not, in fact, fire for only enabled beans.

It is looking more and more like there is no solution to this problem, provided that the Microprofile Config specification actually mandates validation during the AfterDeploymentValidation event. If instead like many other things in the specification it is merely a hazy sketch that permits all sorts of different interpretations, we may be able to get away with not validating at all. I'll test this. If that's true (i.e. if we let mandatory injection points fail at runtime during normal application runtime instead of pre-validating them but the TCK still passes), then perhaps I'll add a switch to enable or disable this behavior.

However, even if we are able to get away with not doing AfterDeploymentValidation-time validation, the mere fact that the specification requires a DeploymentException to be thrown in the case of missing configuration property values, together with the javadoc of DeploymentException itself, would seem to require that such validation must be done at AfterDeploymentValidation time, or at least at some other unspecified time that CDI defines to be "initialization time". @martin-sladecek things are not looking good here.

[martin-sladecek]

Thanks @ljnelson for detailed examination. What about Specialization with constructor injection - wouldn't that skip validation of the injected objects in the superclass?
Anyway, since it's basically a specification issue, is there a way to report this to microprofile.io project?

[ljnelson]

Hi, @martin-sladecek; there may be some edge cases, but the general problem goes like this:

Consider A, specialized by B. No matter what you do, if A has any @ConfigProperty-annotated injection points, they must be validated (thus spaketh the spec). This is true whether A's injection points in question are field injection points, method parameter injection points or constructor parameter injection points.

Obviously the easiest workaround to this specification issue is to either (a) use @Inject @ConfigProperty private Optional<String> someOptionalConfigValue, i.e. always inject Optionals or (b) just supply a sane default value everywhere.

The MicroProfile Config project (and its issue board) can be found here: https://github.com/eclipse/microprofile-config/

[ljnelson]

@martin-sladecek I'm going to close this as there's not anything more to be done on this end. Feel free to file a new issue if you discover any other problem here.