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

Draft Lite version of BeanManager. #471

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Conversation

manovotn
Copy link
Contributor

This is a first version of the Lite variant of BM - related to #469

It contains a subset of methods from original BM that would be guaranteed to run under CDI Lite restrictions all the while giving user an API that doesn't return UnsupportedException on any of its methods.

Note that the idea is that Lite users would be encouraged to use this Lite BM variant but they won't be blocked from using full blown BeanManager if they choose to (but some methods will throw exceptions). This is due to the fact that implementations can choose to implement more functionalities than Lite requires.

Any future changes that would want to add more methods from classic BM to its Lite counterpart are an easy change so I propose this first draft with smaller set of features that should make sense and be aligned with the functionalities that we discussed should be part of Lite.

Suggestions regarding what should/shouldn't be in Lite BM are welcome as well as alternative names for it (although it should IMO contain BeanManager in it).

@manovotn manovotn added the Lite Related to CDI Lite label Mar 22, 2021
@Ladicek
Copy link
Contributor

Ladicek commented Mar 22, 2021

The set of methods you chose looks good to me. I've briefly pondered getBeans(String name), but I guess we didn't want to restrict or change @Named, so it's probably fine.

@graemerocher
Copy link
Contributor

Looks reasonable to me, and reflects what I am able to implement right now for build time annotation processing

@manovotn
Copy link
Contributor Author

FTR I have verified (with japi-compliance-checker) that doing this kind of change is binary compatible.

@Ladicek
Copy link
Contributor

Ladicek commented Mar 26, 2021

I was wondering if we could come up with a name that would naturally be a "subset" of the BeanManager name and describe well the purpose (because LiteBeanManager or BeanManagerLite are not very nice).

I was thinking maybe BeanContainer would be good?

@manovotn
Copy link
Contributor Author

Hmm, I like the notion of Manager as that's what the beast essentially is. The name should probably also stay close to original BM as users might choose to use the classic version at times (if their impl supports something that is not in Lite BM for instance).

That being said, I agree that BeanManagerLite isn't exactly great name but I couldn't think of better one so far :-)

@graemerocher
Copy link
Contributor

I think BeanContainer is a good name either that or in Micronaut we call it BeanContext

@Emily-Jiang
Copy link
Contributor

Similar to @manovotn , I prefer the name shows the relationship with the other Bean Manager. How about LeanBeanManager or CoreBeanManager? I think BeanContainer is not quite related to BeanManager from the naming aspect.

@graemerocher
Copy link
Contributor

CoreBeanManager has a nice ring

* CDI Lite users should primarily use this version of bean manager but can choose to use standard {@link BeanManager}
* as well. However, some of it's methods will not work.
*/
public interface BeanManagerLite {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further to the discussion, I think both Graeme and I prefer CoreBeanManager.

@ljnelson
Copy link

Hmm; a BeanManager manages beans; a CoreBeanManager manages…?

@Emily-Jiang
Copy link
Contributor

Hmm; a BeanManager manages beans; a CoreBeanManager manages…?

CoreBeanManager also manages beans. Core means the subset of the BeanManager

@ljnelson
Copy link

Right; I'm aware of what is intended but BeanManagerCore would be better in that case (just from an English perspective): usually when you put a noun together with Manager as a suffix, the thing being managed is the noun. As currently proposed, CoreBeanManager would thus be an exception to that general rule (it suggests that the thing being managed is a "core bean", which is not the intent) and might therefore be confusing; users might start wondering what core beans are and how they're managed.

I think all these alternatives—the one proposed, CoreBeanManager, mine immediately above (BeanManagerCore; yuck), BeanContext (context for beans? then who has the beans whose context this is?), BeanContainer (maybe the best of the lot? not sure? but then what does a manager do above and beyond a container?)—are clunky, though.

What about something like ReferenceManager, or ContextualReferenceManager, etc. since the overall work so far appears to be removing functionality that is not directly or indirectly related to the acquisition and production of contextual references (and instances, to a lesser degree)? Then BeanManager can be explained as a superset: it would be all the contextual reference acquisition stuff plus the parts you're looking to prune that are relevant to portable extensions. At least there could be a rationale for the two names: one (ContextualReferenceManager or ReferenceManager) is for the business of doing what you need to to acquire and use contextual references and instances, and the other (BeanManager) is that plus more detailed work involving validation and customization of the beans involved in the former.

Regardless, I urge treading very carefully here: when names aren't crystal clear, sometimes that means the underlying concepts can end up not being crystal clear either.

@Ladicek
Copy link
Contributor

Ladicek commented Apr 14, 2021

I used similar reasoning when proposing BeanContainer (though it was a post-hoc reasoning really :-), which is why I didn't add it to my comment). BeanContainer is a passive thing, while BeanManager is a more active thing. Looking at the proposed split, honestly, this reasoning is very artificial; there's really not much difference in terms of "passive" vs. "active" between the "full" and the "lite" bean manager. The subset is picked based on what features are supposed to be part of CDI Lite, nothing else.

I very much agree that CoreBeanManager looks strange purely from English perspective, because "core" is more often a noun rather than an adjective. If we want to use the word "lite" in an API, it could easily be LiteBeanManager, because "lite" is an adjective for sure.

Another option -- the new extension API is currently dubbed BuildCompatibleExtension (compared to Portable Extension's Extension). If we stick to that name, we can as well use BuildCompatibleBeanManager (though is it a mouthful!).

@manovotn
Copy link
Contributor Author

With the above said, would CdiLiteManager or perhaps just LiteManager be better? Since it really just 'manages what CDI Lite has`.

Just FTR, the logic used in comments above is good. However, our currently existing BeanManager has outgrown its 'bean management' scope already because it allows you to check scopes, fire events, etc. Therefore, I am not sure we can use the same logic to deduce the name of its Lite counterpart. I was more looking for a name that will be akin to BeanManager which people are already familiar with.

That being said, I am not against either proposal so long as it contains reasonable degree of familiarity.

@ljnelson
Copy link

One quick opinion: don't use "lite" in the API, specification or documentation. It would create a world in which there would be heavy stuff and light stuff, and misspells "light" in a way that evokes mid-20th-century American advertising of cheap goods designed to fail quickly, and bakes it into the API signature itself. Almost any other option would be better.

@Emily-Jiang
Copy link
Contributor

The other options I have: BeanManagerBase or BeanManagerCore. I think the BeanManager in the name is very important.

@Ladicek
Copy link
Contributor

Ladicek commented May 4, 2021

Just noticed there's CDI.getBeanManager() -- we have to add a 2nd method to obtain the BeanManagerLite. (Where frankly, the more I think about it, the more I'm convinced we should call it BeanContainer, as the least bad of all bad options.)

@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2021

Yeah, good point. I'll add it to this PR (with the current naming, we'd need to change that on multiple places and we don't seem to have an agreement on the name yet).

* @throws IllegalStateException if called during application initialization, before the {@link AfterBeanDiscovery}
* event is fired.
*/
List<Interceptor<?>> resolveInterceptors(InterceptionType type, Annotation... interceptorBindings);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also wondering -- do we actually want these 2 methods (resolveObserverMethods and resolveInterceptors)? I know we implement them in Quarkus, but they sound a bit too runtime-oriented.

@Ladicek
Copy link
Contributor

Ladicek commented May 4, 2021

Thanks!

I added one comment about 2 methods about which I'm not sure.

One other thing is -- on a few places, we mention JNDI, which we probably want to remove from BeanManagerLite -- or at least mention that JNDI only applies to CDI Full (or even EE?).

Also, on a few places in BeanManagerLite, we say something like "this BeanManager" -- we should reword that to "this BeanManagerLite".

@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2021

Thanks!

I added one comment about 2 methods about which I'm not sure.

Not sure either. I added it because without portable extensions, this might be the only way to get your hands on interceptor/OMs.
Then again, I have no justification as to why you'd want to do that :-)

One other thing is -- on a few places, we mention JNDI, which we probably want to remove from BeanManagerLite -- or at least mention that JNDI only applies to CDI Full (or even EE?).

I see what you mean, but I am not sure it's worth the hassle. We'd probably need to copy the method into original BM as well with the original wording. Then again, something like BM.createInstance() even now talks about JNDI - and you can use this method in CDI SE where there is no JNDI.

Also, on a few places in BeanManagerLite, we say something like "this BeanManager" -- we should reword that to "this BeanManagerLite".

Will do. However, note that the very same method can then be used from standard BM which will make the wording awkward again. So I'd probably replace it by BeanManager/BeanManagerLite to cover both cases.

@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2021

@Ladicek, I've added some class javadoc and changed the occurrences of BeanManager to BeanManager/BeanManagerLite which should sound better.

EDIT: also rebased on master to fix conflict in BM API

I didn't touch the JNDI yet because I am not sure we need to tackle it if it was there even now for all CDI envs. Alternatively, we could change the sentence into something like:

... available for injection in the module or library containing the class into which the BeanManager/BeanManagerLite was injected or, in CDI Full environment, the Java EE component from whose JNDI environment namespace the BeanManager/BeanManagerLite was obtained, according to the rules of typesafe resolution.

That being said, the wording above (and the one currently in PR) would imply that you can obtain BeanManagerLite from JNDI which I am not sure we want. But technically speaking you can do that anyway because you can just grab the ordinary BM.

@graemerocher
Copy link
Contributor

My vote is still for BeanContainer as the name for this new interface.

@Ladicek
Copy link
Contributor

Ladicek commented May 4, 2021

@manovotn Given that BeanManager extends BeanManagerLite and you can say "BeanManager is BeanManagerLite", I believe you can just say "this BeanManagerLite" and it transitively applies to BeanManager as well :-)

As for JNDI, we could possibly restrict it to Jakarta EE. (In the spec split, I have currently put it into CDI Full, but we could easily move it to Jakarta EE part as well.) Something like

available for injection in the module or library containing the class into which the BeanManagerLite was injected or, in Jakarta EE environment, the Jakarta EE component from whose JNDI environment namespace the BeanManagerLite was obtained, according to the rules of typesafe resolution

But what you have, with CDI Full, seems fine as well.

@manovotn
Copy link
Contributor Author

manovotn commented May 4, 2021

Updated accordingly to the suggestion.
Also fixed the CI failure.

@manovotn
Copy link
Contributor Author

manovotn commented May 5, 2021

As more people lean towards BeanContainer (with me slowly joining it too), I have changed the the class name to BeanContainer.
I've also adapter the javadoc to try and avoid mentioning Lite as much as I could but I think we can't remove it altogether as we need to at least say that the usual BM in Lite is non-portable behavior.

@manovotn manovotn marked this pull request as ready for review June 1, 2021 08:35
@manovotn manovotn requested a review from Ladicek June 1, 2021 08:39
@manovotn
Copy link
Contributor Author

manovotn commented Jun 1, 2021

This should be ready for merge - soon as it gets a green stamp.
It is one of the two PRs (the other being build compatible extensions) that we want in the repo before cutting first Alpha.

@manovotn manovotn merged commit 0988d2c into jakartaee:master Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lite Related to CDI Lite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants