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

introduce two CDI helper extension to add beans explicitly #2125

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

craig-day
Copy link

@craig-day craig-day commented Mar 29, 2023

As discussed in #2115, this introduces some CDI extensions to act as a bridge for environments where auto bean discovery is not utilized. A consumer can chose to explicitly add these extensions to their CDI container to get the same support that auto discovery would have provided.

Opening as a draft to start to see how we feel about the approach before propagating it to all other connector implementations and updating tests.

@manovotn
Copy link
Contributor

Few comments coming to my mind.

  1. From CDI perspective, this form of bean registration seems fine 👍
    However, I'd add a comment into each extension denoting that these CDI extensions are disabled by default as they intentionally have no META-INF entry in this repository and that they are meant to be used explicitly by integrators.

  2. As for the set of classes added, I have no idea which classes SR reactive messaging need so I'll trust you know that.
    How likely is this set going to change or expand? Since this way it would be hard to remember to add these beans into extensions as well.

  3. I think the tests using Weld SE should leverage these extensions - i.e. the base test class should register these extensions (whichever are needed) instead of manually registering the beans. That will also help cover the case of newly added CDI beans.

  4. Last but not least, we should double check that all modules that should have beans.xml (and are subject to having the extension added) do have one because adding an extension into them might otherwise trigger "no archive behavior" as per this spec part:

An archive which:

  • contains a beans.xml file with the bean-discovery-mode of none, or,
  • contains a portable extension or a build compatible extension and no beans.xml file

is not a bean archive.

@ozangunalp
Copy link
Collaborator

I am not against this change but we'll need to find some distinctive names for these "unexposed" CDI Extension classes.
ReactiveMessagingCDIExtension is too similar to ReactiveMessagingExtension.

As @manovotn mentioned we should register these extensions when running the connector tests. It'll also serve as a way to check that the Extension is not missing a critical bean.
For example change the following addBeanClass with addExtension in https://github.com/smallrye/smallrye-reactive-messaging/blob/main/smallrye-reactive-messaging-kafka/src/test/java/io/smallrye/reactive/messaging/kafka/base/WeldTestBase.java#L69

We would do the same for connectors other than Kafka too. I think we have beans.xml classes for every connector module.

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

Successfully merging this pull request may close these issues.

3 participants