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

Clarification: Annotations vs Qualifiers #320

Open
ghunteranderson opened this issue Feb 1, 2022 · 8 comments
Open

Clarification: Annotations vs Qualifiers #320

ghunteranderson opened this issue Feb 1, 2022 · 8 comments

Comments

@ghunteranderson
Copy link

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
Copy link

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
Copy link
Author

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

  • Provide health checks that can be enabled/disabled with MicroProfile Config
  • Dynamically register health checks for things like discovered JDBC connections

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

@arjantijms
Copy link

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
Copy link
Member

@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
Copy link
Member

xstefank commented Feb 9, 2022

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
Copy link
Member

I was pointed to eclipse/microprofile-fault-tolerance#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
Copy link

Ladicek commented Feb 10, 2022

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
Copy link

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.

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

No branches or pull requests

5 participants