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

Move jakarta.el-api related method to new BeanManager subclass? #613

Closed
starksm64 opened this issue May 18, 2022 · 13 comments · Fixed by #644
Closed

Move jakarta.el-api related method to new BeanManager subclass? #613

starksm64 opened this issue May 18, 2022 · 13 comments · Fixed by #644
Milestone

Comments

@starksm64
Copy link
Contributor

The core profile cannot cleanly separate a dependency on jakarta.el-api from the dependency on jakarta.enterprise.cdi-api because of the usage in BeanManager#{getELResolver, wrapExpressionFactory}. We need to think about how factor out this dependency.

@Ladicek
Copy link
Contributor

Ladicek commented May 19, 2022

The core profile seems to include EL, so that dependency shouldn't be an issue?

@manovotn
Copy link
Contributor

I am not sure it does...

The website and the issue for it both list different parts.
Which one is correct @starksm64?

@Ladicek
Copy link
Contributor

Ladicek commented May 19, 2022

Ah I forgot about that divergence. I only looked at the first link.

If the core profile doesn't include EL, exclusion should be possible, but I agree it's not nice.

@starksm64
Copy link
Contributor Author

The release plan is out of date as that was the plan before we knew what CDI Lite would support. The Core Profile specification does not list EL as a supported tech. It says it is not required, and the platform api project gives the core profile dependencies as:

https://github.com/eclipse-ee4j/jakartaee-api/blob/d4738affc87918cd5887acd3f0d638a81d95a337/pom.xml#L47

        <!-- Core Profile -->
        <!-- Theoretically these versions could be different from Web Profile -->
        <jakarta.json-api.cp.version>2.1.0</jakarta.json-api.cp.version>
        <jakarta.json.bind-api.cp.version>3.0.0</jakarta.json.bind-api.cp.version>
        <jakarta.annotation-api.cp.version>2.1.0</jakarta.annotation-api.cp.version>
        <jakarta.inject.cp.version>2.0.1</jakarta.inject.cp.version>
        <jakarta.interceptor-api.cp.version>2.1.0</jakarta.interceptor-api.cp.version>
        <jakarta.enterprise.cdi-api.cp.version>4.0.1</jakarta.enterprise.cdi-api.cp.version>
        <jakarta.inject.cp.version>2.0.1</jakarta.inject.cp.version>
        <jakarta.ws.rs-api.cp.version>3.1.0</jakarta.ws.rs-api.cp.version>

This is where the optional dependency on EL is introduced because the jakarta-core-api jar is rebuilt from the source dependencies, and this requires EL due to the BeanManager usage.

EL is excluded from the required compile scope dependency on CDI, but then a provided optional dependency has to be added to be able to compile the jakarta-core-api jar.

@manovotn
Copy link
Contributor

I see.

So ATM we seem to have two methods on BeanManager:

  • public ELResolver getELResolver();
  • public ExpressionFactory wrapExpressionFactory(ExpressionFactory expressionFactory);

Plus an entry in module-info (which, IIUIC, is a mandatory compile, optional runtime dependency):

  • requires static jakarta.el;

In an ideal world, we'd have CDI Lite JAR and then CDI Full JAR (which depends in Lite as well as EL API) but we've been to that rabbit hole before and there wasn't an agreement on how to do that without breaking everything.
Another way would be to extract the two BM methods into another "EL-aware" module but that's not very nice justification for a separate module. Besides, it again breaks any app or integrator using these two methods.

exclusion should be possible, but I agree it's not nice.

@Ladicek which approach did you mean?

@Ladicek
Copy link
Contributor

Ladicek commented May 23, 2022

I meant EL should be a compile-only dependency of CDI in the Core profile, and the Core profile "uberPOM" should add an exclusion of EL to the CDI dependency. Of course, Maven doesn't really have a concept of compile-only scope, but there are workarounds (optional, provided scope).

@manovotn
Copy link
Contributor

Ah ok, I thought you were talking about solution on CDI spec level.

@manovotn
Copy link
Contributor

We discussed this during CDI meeting with the following notes:

  • No immediate action is needed in CDI, this is workarounded by Jakarta core profile
  • In the future, we'd ideally want to get rid of this dependency somehow
  • We want to look into existing EE servers and how they use this API. If it is just integrators, we could change the way this is provided or make it impl dependent (i.e. via Weld's API/SPI) or other means to remove the EL dep from CDI.
    • @manovotn is going to look into WildFly usage
    • We could also use insight from Open Liberty, so @Emily-Jiang or someone from their side can take a look
  • @Emily-Jiang also mentioned that there was an end user use case for these two BeanManager methods, it would be nice to know what scenario is that. Otherwise, we can only be sure this is used by integrators ATM.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 4, 2022

We discussed this briefly on the CDI call today and there's a pretty straightforward way:

  1. Create a separate module that would depend on EL.
  2. Add an interface to that module, with the 2 EL-related methods. Demand that containers provide a bean for that interface. The bean can be injected, or looked up programmatically.
  3. Remove the EL-related methods from BeanManager (and the EL dependency from the core CDI API). This would be a hard breaking change, of course.

An open question is: should that new interface extend BeanManager? My personal opinion is no, but there are probably good reasons to make it so.

@rdebusscher
Copy link

Isn't it easier to just have separate artefacts for CDI lite and CDI full. CDI lite doesn't contain than any reference to EL classes and BeanManager can keep the EL one a-since as part of Web profile, EL is already available.

See also #640

@manovotn
Copy link
Contributor

EL API has very little usage in CDI - two methods altogether IIRC. Separating that would be fairly easy.

I've also added comment to #640 (comment)

@Ladicek
Copy link
Contributor

Ladicek commented Jan 5, 2023

I drafted a solution for this here: #644

@Emily-Jiang Emily-Jiang added this to the CDI 4.1/5.0 milestone Jan 31, 2023
@Ladicek Ladicek removed the CDI.next label Jan 31, 2023
@starksm64
Copy link
Contributor Author

So today's discussion brought up the idea of a 4.1 release with deprecation of the BM method, and removal in 5.0. The question that came up is whether there is some restriction on whether a platform release could skip over the 4.1 release, in which case the 11 core platform would never see a deprecation of the BM method.

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 a pull request may close this issue.

5 participants