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.

Sign up for GitHub

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

Jump to bottom

Issue #5162 CDI embedded integration improvements #5177

Merged
merged 16 commits into from Sep 7, 2020
Merged

Issue #5162 CDI embedded integration improvements #5177

merged 16 commits into from Sep 7, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Aug 20, 2020

Clean up CDI integration and documentation to better support embedded usage.

  • made listener public
  • added utility class for SCIs

@gregw
Issue #5162 CDI embedded integration improvements
67c373c 
Clean up CDI integration and documentation to better support embedded usage.
 + made listener public
 + added utility class for SCIs
@gregw
Copy link
Contributor Author

gregw commented Aug 20, 2020

@janbartel @joakime can I get some feedback on this draft.

gregw
gregw commented Aug 20, 2020
View reviewed changes
...-webapps/test-weld-cdi-webapp/src/test/java/org/eclipse/jetty/cdi/weld/EmbeddedWeldTest.java Outdated
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;

public class EmbeddedWeldTest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this unit test to test similar features as the webapp, but from embedded

gregw
gregw commented Aug 20, 2020
View reviewed changes
...-webapps/test-weld-cdi-webapp/src/test/java/org/eclipse/jetty/cdi/weld/EmbeddedWeldTest.java Outdated
context.addFilter(ServerIDFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));

// Setup Jetty weld integration
switch (mode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think all these modes are recommended, but all variations are probably used somewhere so best to test to know if we break any usages.

gregw
gregw commented Aug 20, 2020
View reviewed changes
...-webapps/test-weld-cdi-webapp/src/test/java/org/eclipse/jetty/cdi/weld/EmbeddedWeldTest.java Outdated
Comment on lines 202 to 208
Configuration.ClassList.setServerDefault(server).addBefore(JettyWebXmlConfiguration.class.getName(),
AnnotationConfiguration.class.getName());

// Need to expose our SCI. This is ugly could be made better in jetty-10 with a CdiConfiguration
webapp.getServerClasspathPattern().add("-" + CdiServletContainerInitializer.class.getName());
webapp.getSystemClasspathPattern().add(CdiServletContainerInitializer.class.getName());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a CdiConfiguration to do this in 10.

@gregw gregw linked an issue Aug 24, 2020 that may be closed by this pull request
DecoratingListener raises a NullPointerException #5162
Closed
@gregw
Copy link
Contributor Author

gregw commented Aug 24, 2020

Hmm test harness approach doesn't work without IDE... refactoring...

@gregw gregw requested a review from WalkerWatch August 24, 2020 10:46
@gregw
Copy link
Contributor Author

gregw commented Aug 24, 2020

@WalkerWatch can you check the doco changes in this PR

@gregw
Issue #5162 CDI embedded integration improvements
ed0b344 
Clean up CDI integration and documentation to better support embedded usage.
 + moved EmbeddedWeldTest to jetty-embedded
@gregw
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-51…
b0f16c6 
…62-Improve-Decorating-Listener-Examples
@gregw
fix javadoc
b2fd5af 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
WalkerWatch
WalkerWatch approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@WalkerWatch WalkerWatch left a comment

Choose a reason for hiding this comment

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

Documentation is fine with one caveat. We try to make use of ventilated prose in asciidoc as it makes maintenance and upkeep easier.

@gregw
Issue #5162 CDI embedded integration improvements
e4d7712 
ventilated text
@gregw
Merge branch 'jetty-9.4.x-5162-Improve-Decorating-Listener-Examples' …
1052180 
…of github.com:eclipse/jetty.project into jetty-9.4.x-5162-Improve-Decorating-Listener-Examples
@gregw
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-51…
68cf3f5 
…62-Improve-Decorating-Listener-Examples
@gregw
fix test pom
f923d9b 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Fixed javadoc
d3c4bd8
@gregw
Merge branch 'jetty-9.4.x-5162-Improve-Decorating-Listener-Examples' …
7784be4 
…of github.com:eclipse/jetty.project into jetty-9.4.x-5162-Improve-Decorating-Listener-Examples
@gregw
Fixed javadoc
4008385
@gregw
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-51…
c9e1fa5 
…62-Improve-Decorating-Listener-Examples
@gregw
Issue #5162 CDI embedded integration improvements
9a6f48c 
Moved tests to jetty-cdi to avoid consequences to other tests in embedded
@gregw gregw marked this pull request as ready for review August 25, 2020 14:26
@gregw gregw requested a review from janbartel August 26, 2020 10:36
@gregw
trailing new line
16bae47 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor Author

gregw commented Aug 31, 2020

@janbartel What are the show stoppers for you in this PR?

@gregw
Copy link
Contributor Author

gregw commented Sep 2, 2020

@janbartel @joakime nudge!

janbartel
janbartel approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

So I don't have any showstoppers, but I would like you to take a look at my suggestion for the isKnownDecoratable() method.

jetty-cdi/src/main/java/org/eclipse/jetty/cdi/CdiSpiDecorator.java Outdated
{
if (Object.class == clazz)
return true;
if (isKnownUndecoratable(clazz.getName()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not at all fond of the name isKnownDecoratable(). How about instead you remove isKnownDecoratable() and add a field list of _undecoratables, a setter for them, and in isDecoratable() you just check the list? This might also be a good idea in case CDI adds some other classes that can't be decorated?

jetty-documentation/src/main/asciidoc/development/frameworks/cdi.adoc Outdated

However, depending on the exact configuration of the embedded server, either or both steps may not be required as Servlet Container Initializers may be discovered.

The details for embedding CDI will be explain in the Embedded Jetty with Weld section, but can be adapted to other CDI frameworks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a link here?

@gregw
updates from review
b665a6e 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
janbartel
janbartel requested changes Sep 2, 2020
View reviewed changes
jetty-cdi/src/main/java/org/eclipse/jetty/cdi/CdiSpiDecorator.java Outdated
* @param undecorated The set of class names that will not be decorated.
* @see #isDecoratable(Class)
*/
public void setUndecoratedClasses(Set<String> undecorated)
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant "setUndecoratableClasses". Undecorated makes it sound like they haven't been decorated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janb I saw that, but I think Undecorated also works, is less of a mouthful and eitherway you need to check with the javadoc to really know what it means.

janbartel
janbartel approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

LGTM

@gregw gregw merged commit 7ecf42e into jetty-9.4.x Sep 7, 2020
@joakime joakime mentioned this pull request Sep 16, 2020
Closed
@joakime joakime deleted the jetty-9.4.x-5162-Improve-Decorating-Listener-Examples branch September 17, 2020 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime left review comments

@janbartel janbartel janbartel approved these changes

@WalkerWatch WalkerWatch WalkerWatch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DecoratingListener raises a NullPointerException
4 participants
@gregw @joakime @janbartel @WalkerWatch