search

eclipse / microprofile-health

microprofile-health
Apache License 2.0
106 stars 61 forks source link

Clarification: Annotations vs Qualifiers #320

Open ghunteranderson opened 2 years ago

ghunteranderson commented 2 years ago

Description

In most use cases, CDI qualifiers are applied using annotations. However, it's possible in CDI extensions to apply qualifiers to beans without adding annotations. In the example below, there are no annotations on the bean but it is qualified with Readiness.

// @Observes AfterBeanDiscovery event
event.addBean()
  .types(HealthCheck.class)
  .qualifiers(new ReadinessAnnotation())
  .scope(ApplicationScoped.class)
  .produceWith(obj -> {
    return new CustomHealthCheck();
  });

If an implementation were using the ProcessBean event to detect this bean, then processBean.getAnnotated() would not contain annotations but processBean.getBean().getQualifiers() would include the readiness qualifier.

When reading the spec, I'm not certain if beans defined in this way must/may be included in the health check endpoints. This statement from the spec leads me to think it's optional.

Any enabled bean with a bean of type org.eclipse.microprofile.health.HealthCheck and @Liveness, @Readiness, or @Startup qualifier can be used as health check procedure.

However, other areas of the spec speak very directly about requiring one of the annotations.

A HealthCheck procedure with none of the above annotations is not an active procedure and should be ignored.

I'm not really sure where beans like the one above fit into the specification. Does the spec intend for these to be used or are they outside the scope of the specification? I appreciate any guidance the community can provide.

arjantijms commented 2 years ago

Technically, CDI internally stores Beans, and these can be created from a managed bean (class with or without annotations), from a producer (@Produces annotated method) or by directly adding those Bean instances.

There should not be any difference, although a couple of imperfections in the CDI prevent code such as libraries to easily support everything.

Just a few examples:

In this case I guess the TCK should contain Beans added directly (or using the configurator, as shown in your example), so implementations can take this into account.

ghunteranderson commented 2 years ago

Thanks for the input @arjantijms! Personally, I would love to see a TCK added for this. It allows developers to do things like

I'd be happy to open a PR if we want a TCK for this scenario.

arjantijms commented 2 years ago

If the project committers (which I'm not) agree that would certainly be a good idea. If it's not too much work I would just do the PR. I certainly think it's a good idea. Other specs could use this too in their TCKs.

Emily-Jiang commented 2 years ago

@ghunteranderson for the readiness, liveness or stantup to be picked up, the class should be a CDI bean. The annoations are just qualifier not stereotype. Maybe in the future releases, these annotations should be updated to be stereotype with ApplicationScoped as the scope. @xstefank @pgunapal what do you think?

xstefank commented 2 years ago

I am personally not against such a test but I must mention that some implementations, e.g. Quarkus, don't support CDI portable extensions. So such TCK test will be explicitly excluded from our certifications. Personally, I would prefer to wait for the build-compatible extensions API that will come with CDI Lite and Jakarta 10 to avoid such problems.

xstefank commented 2 years ago

I was pointed to https://github.com/eclipse/microprofile-fault-tolerance/pull/596 for what I now agree that such test will be testing a CDI behavior, not MP Health. I also found out that the CDI Lite will no longer contain Portable Extensions, only Build compatible extensions so if we move to allow MP implementation to implement Jakarta CDI Lite, which I suppose is planned for the June release, this test will need to be removed.

Ladicek commented 2 years ago

In my opinion, if the specification contains unclear text, such as text that refers to annotations when it should refer to qualifiers (or qualifier annotations), that text should be fixed. That said, text such as

Any enabled bean with a bean of type org.eclipse.microprofile.health.HealthCheck and @Liveness, @Readiness, or @Startup qualifier can be used as health check procedure.

is pretty clear to me in that a synthetic bean that has the correct bean type and a qualifier is included.

arjantijms commented 2 years ago

for what I now agree that such test will be testing a CDI behavior, not MP Health.

It's not that simple. In a few cases implementations need to be aware of what they're doing to be able to support CDI Beans generated from several places (Managed Bean, Producer, Bean, etc). For instance, reading the annotations directly off the class instance in Interceptors is a no-go, as it wont work in general. You need to get the annotations via the API.

For instance, the @RememberMe interceptor needs to get its annotation to read attributes from, as done here in Soteria:

https://github.com/eclipse-ee4j/soteria/blob/master/impl/src/main/java/org/glassfish/soteria/cdi/RememberMeInterceptor.java#L158

In the naive situation, I would just get the class type that was intercepted and read the annotation from it. But that will only work if the intercepted bean was added via a managed bean.

A test for this makes sure the implementation is calling the right APIs, and it most definitely does not just test CDI behaviour.

Throughout the years, working on application servers, component implementations and various TCKs, I noticed this is easily missed. You really need to be elbow deep in this to have this aha! moment, so it would be good for TCKs in general to test this.