Weld/CDI XML backwards compat?

[joakime]

We have a backwards compat XML breakage for weld/cdi users starting in Jetty 10.0.x

The documented format ...
https://docs.jboss.org/weld/reference/latest/en-US/html/environments.html#_class_loading

   <Call name="prependServerClass">
      <Arg>-org.eclipse.jetty.util.Decorator</Arg>
   </Call>
   <Call name="prependServerClass">
      <Arg>-org.eclipse.jetty.util.DecoratedObjectFactory</Arg>
   </Call>
   <Call name="prependServerClass">
      <Arg>-org.eclipse.jetty.server.handler.ContextHandler.</Arg>
   </Call>
   <Call name="prependServerClass">
      <Arg>-org.eclipse.jetty.server.handler.ContextHandler</Arg>
   </Call>
   <Call name="prependServerClass">
      <Arg>-org.eclipse.jetty.servlet.ServletContextHandler</Arg>
   </Call>

The new format, starting in Jetty 10.0.x is ...

  <Get name="serverClassMatcher">
    <Call name="add">
      <Arg>-org.eclipse.jetty.util.Decorator</Arg>
    </Call>
    <Call name="add">
      <Arg>-org.eclipse.jetty.util.DecoratedObjectFactory</Arg>
    </Call>
    <Call name="add">
      <Arg>-org.eclipse.jetty.server.handler.ContextHandler</Arg>
    </Call>
    <Call name="add">
      <Arg>-org.eclipse.jetty.servlet.ServletContextHandler</Arg>
    </Call>
  </Get>

The changes are ...

• The WebAppContext.prependServerClass() method has been removed.
• The org.eclipse.jetty.server.handler.ContextHandler. (note the extra trailing ".") has been removed.

It should be noted that WebAppContext.prependServerClass() is a deprecated method in Jetty 9.4.x.

https://github.com/eclipse/jetty.project/blob/b9fc1f736241036162ecfed8ea5cde22337b0f0d/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java#L718-L725

The suggested alternative of using .getServerClasspathPattern() has been renamed in Jetty 10.0.x to getServerClassMatcher()

Do we want to rename this back to .getServerClasspathPattern() in Jetty 10.0.x?
Or do we want to bring .getServerClassMatcher() back to Jetty 9.4.x?

Either approach will minimize the version hassles that weld/cdi will have to deal with.

[gregw]

@joakime firstly, can't we do this kind of classpath modification in CdiConfiguration class? That way we hide the details from any XML, plus we can use the convenience methods that read better than adding a negative!

Updating the handling of server/system classes is exactly the kind of thing that should be looked at in a migration for 9.4 to 10, so I'm not keen on making things "just work", when they really should be reviewed.

Also, while this is not really the subject of this issue, do we REALLY have to expose org.eclipse.jetty.server.handler.ContextHandler and org.eclipse.jetty.servlet.ServletContextHandler ? It would be far better to expose just a CDI specific class that does whatever manipulations are necessary on the context rather than giving open access to the entire webapp to those important implementation APIs. Actually, let's make this the subject of this issue - CDI should have a Configuration that does all the class matcher manipulation and exposes a minimal set of classes to the webapp - I even don't like our decorators being exposed. Surely that can all be done in Configuration and/or SCI ?

Or is this because weld comes with a jetty specific class that needs to access these APIs and is beyond our control? Hmmmmm I still think we can do better. Perhaps we have a WeldSpecific Configuration that provides backwards compatible APIs that at least warn if an old jetty-web.xml is uses and that invokes the Weld mechanisms in a way that does need the general API exposure.... eg if they are initially invoked from a listener, then we can wrap their listener in something that turns on the expose server classes switch and then calls Weld. We are going to have to get the weld folks to change their integration, so let's do it in a way that will be more correct and portable in the future.

[gregw]

See also #3803

[gregw]

Looking at the weld integration, ultimately it comes down to installing a Decorator that calls:

    public <T> T decorate(T o) {
        getInjector().inject(o);
        return o;
    }

    public void destroy(Object o) {
        getInjector().destroy(o);
    }

We must be able to come up with a way to do this without exposing key APIs to the webapp nor binding to specific jetty APIs... even if it is to optionally give special treatment to a nominated class and to use MethodHandles to match the inject/destroy signatures (why javax.inject doesn't have a standard API for this I don't know???).

So the challenge is, how to let a webapp add a decorator without container nor webapp having API dependencies on each other?

[gregw]

Oh and whatever we do, we should back port to 9.4, so the next release of Weld would work on both. Let's say that we came up with a jetty specific attribute name, so in order to install a decorator, the SCI just did:

getServletContext().setAttribute("org.eclipse.jetty.webapp.decorator", MyDecorator);

Jetty could then use method handles to look for an appropriate signatures for a decorate and destroy methods, it would then create a version appropriate decorator and add it in the version appropriate way. This will work for 9.4 and 10 and need no classpath opening!

[gregw]

I have created https://issues.jboss.org/browse/WELD-2587 to solicit input from Weld developers

[joakime]

In order for weld to add the decorator to the servlet context it needs access to both org.eclipse.jetty.server.handler.ContextHandler and org.eclipse.jetty.servlet.ServletContextHandler.

I like the servlet attribute approach.
One thought, Decorators are a list, multiple libraries can add them (not just weld).
How would we go about that with the attribute approach? perhaps a special key name?
org.eclipse.jetty.util.decorator.<lib> and that tells Jetty to add whatever decorator it finds to the list of decorators?

[joakime]

Also, perhaps org.eclipse.jetty.util.Decorator should just be exposed all the time for all WebApps.
If we combine that with the attribute approach, then there's no need to manually expose any more classes like we have in the past.

Never mind, the method discovery approach seems more reasonable.

[gregw]

I agree with your change of heart about exposing an API. Once you start down that road, it is long and twisty and never ends. We have mostly avoided exposing special API - at least not code APIs. We have used the context attribute trick a few times to avoid the hard dependencies that are at the root of the difficulties.

We can handle multiple decorators by making the set attribute have add semantics, so setting a single attribute just adds to the list. Doing a get returns null. Also this need only be active during the execution of a SCI.

[janbartel]

FWIW I really don't like the semantic of making "set" mean "add" - this is not the normal meaning of set and is confusing.

[gregw]

The other way to go is to have a DecoratorConfiguration that needs to be enabled to allow decorators to be set. It can add a list as an attribute and then applications can add their decorators to that list. At the appropriate time that list is processed and the items in it are converted to real decorators.

[gregw]

So here are some of the ways that I can think of to do the loose coupling between a webapp and decorators:

1. We define a special context attribute name that if set, will add a DynamicDecorator. All webapps have the ability to do this and we can't turn it off (they only get to decorate their own filters, servlets and listeners).
2. We define a special context attribute name prefix and setting any name under that prefix will act as a set (or remove if null value) of a decorator. Not sure we support removing decorators elsewhere, but perhaps works better with attribute semantics? All webapps have the ability to do this.
3. We create a DecorationConfiguration, that only if present will add an specially named context attribute with a list value. Any decorators added to that list will be added as dynamic decorators.
4. We create a DecorationConfiguration, that only if present will add a DynamicDecorator that when called will look at a specially named context attribute for a list value. Any decorators added to that list will be treated as decorators and the list can change after init.
5. We create a DecorationConfiguration that only if present will examine all the ServletContextListeners added to the context to see if they implement the Decorator signature and if so, then add a DynamicDecorator that calls the listener directly.

I actually like the last one, as the SCI is already adding a SCL, which can just be-a Decorator rather than add-a Decorator.

[joakime]

Looks like Weld 3.0.0 already updated to use the new non-deprecated Decorator.

See: https://issues.jboss.org/browse/WELD-2032

[gregw]

@joakime That's good, but doesn't really help much. It is the APIs that they need to call to expose the API they need to register their Decorator instance that is a problem. Better to find an API-free way. After discussion with Jan, she suggested that we just look for a SCI that has the right signature and use that as a decorator.

[joakime]

We have to notify https://openwebbeans.apache.org/ too. (as they do the same thing with their Decorators and Jetty ContextHandler)

Have we considered a ServiceLoader approach for this?
The problem here is that we would still have a Decorator interface that needs to be exposed and another method on it to init(ServletContext) the decorator (a common need for both CDI2 implementations)

[gregw]

@joakime ServiceLoader doesn't help, as it still requires a webapp to have a hard dependency on a jetty class.
We don't need to expose the Decorator interface, but the draft PR I'm working on still allows us to do that if we want (or for backwards compatibility). The main problem is not the Decorator interface, but the APIs need to register the decorator and the APIs need to expose the APIs needed to register the decorator!

The approach my PR is working on is that the we need to externally to the webapp configure a context listener, then the webapp just needs to set it's decorator as an attribute. I'm pretty sure that will work.... the question is what is the best more portable way with least disruption to get the listener registered? Configuration? jetty-web.xml?

[gregw]

@joakime Have a look at the PR now. This is working for a manual test in jetty-test-webapp, where you see the steps needed are:

1. Add the org.eclipse.jetty.webapp.DecoratingListener to the context, this is currently done with jetty-web.xml, but could be done with a WeldConfiguration
2. The webapp needs to set it's decorator as an attribute, this is currently done in a test SCI as most frameworks will.
3. The Decorator API does not need to be exposed... but it could be to remove the need for MethodHandles... this could also be done in a WeldConfiguration

[gregw]

I think that the DecoratingListener approach is the correct way to go. However, I'm not so sure how we get there from the current state of affairs as the recent CDI releases will just break unless we continue to expose our implementation in the webapp classpath.

So I propose that we use the fact we have cdi and cdi2 modules to migrate. We will leave the cdi2 module doing the classpath manipulation and make the cdi module use the new listener technique.
We can then prepare a PR for weld, so that it can check to see if the new mechanism is available, in which case it is used... otherwise it can fall back to trying the classpath mechanism.

[gregw]

I have also created https://issues.apache.org/jira/browse/OWB-1293 for OpenWebBeans integration

[gregw]

@stephenc
With regards to mode 1 Embedded Jetty, that style of calling addBefore for configurations was very fragile and is one of the main changes in Jetty-10. Now Configurations will be discovered by the ServiceLoader, will be able to be default enabled or explicitly enabled and are self ordering with respect to other Configurations. So in jetty-10, if the OwbConfiguration is on by default, it will just be sufficient to have the jar on the system classpath for it to be enabled for all webapps. If you have it off by default, the it can be enabled for a particular webapp by calling:

        webapp.addConfiguration(new OwbConfiguration());

Note that Configuration has thus changed a bit, so you will not be able to have a single OwbConfiguration class that works for jetty9 and jetty10 - So a helper function may be good... or just good doco? Although if you follow the same pattern as mode 3, you could use just our CdiConfiguration and that will enable the DecoratingListener mechanism without the need for an OWB specific configuration.

With regards to mode 2, that is precisely what the updated Configuration mechanism is designed for, so you probably want to have a special OwbModuleConfiguration that can be discovered and will do all the class path conditioning and decorator registering for every webapp.

Mode 3 should move to the CDI module that uses the DecoratingListener mechanism. Note that will a bit of care, you can arrange for the one webapp to work with old jetty-web.xml approach or the new mechanism (see the Legacy integration in Weld).

I'm off for the rest of the week, but will read your doco in more detail and comment more next week.

[joakime]

Merged into jetty-9.4.x.