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

FISH-10049 bugfix: Split CDI bean deployment when multiple WARs exist in an EAR #7032

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

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented Oct 18, 2024

Description

Bugfix. BeanManager isn't properly constructed when multiple CDI WARs exist in an EAR.
Also allows EAR-libraries to be scanned for CDI annotations

fixes #7031
also fixes #7078
Appears specifically when multiple WARs use OmniFaces

Testing

Tested with the failing reproducer
** Needs TCK run

@lprimak
Copy link
Contributor Author

lprimak commented Oct 28, 2024

@Pandrex247 Do you think you can run the TCK against this PR?
I'd like to make sure I didn't break anything.

Thank you!

@lprimak lprimak changed the title bugfix: passing BeanManager in EAR deployment's invocation state object bugfix: Split CDI bean deployment when multiple WARs exist in an EAR Oct 28, 2024
@Pandrex247
Copy link
Member

@lprimak I ran it through the CDI and EJB TCKs and all seems green 👍

@lprimak
Copy link
Contributor Author

lprimak commented Oct 28, 2024

Thanks, Fantastic!
I gotta do a lot more testing on my end, but this is promising!

@lprimak
Copy link
Contributor Author

lprimak commented Nov 10, 2024

I have found a few issues that I found with "real" applications.
I think I worked through most of them, so stay tuned

@lprimak
Copy link
Contributor Author

lprimak commented Nov 10, 2024

I may also roll a fix for #6405 if that's trivial as well

@lprimak lprimak force-pushed the cdi-ear-exception branch 2 times, most recently from 89ef769 to 4a79348 Compare November 19, 2024 22:49
@lprimak
Copy link
Contributor Author

lprimak commented Nov 21, 2024

Im getting close. "Real" application runs.

@lprimak lprimak marked this pull request as ready for review November 26, 2024 20:17
@lprimak
Copy link
Contributor Author

lprimak commented Nov 26, 2024

Ready to go. Will do #6405 in a separate PR

@lprimak
Copy link
Contributor Author

lprimak commented Nov 27, 2024

Now I gotta fix the conflict :)

@lprimak lprimak marked this pull request as draft November 27, 2024 22:21
@lprimak
Copy link
Contributor Author

lprimak commented Nov 27, 2024

Once the tests passes I am going to squash the commits and un-draft the PR again

@lprimak lprimak marked this pull request as ready for review November 28, 2024 00:08
@lprimak
Copy link
Contributor Author

lprimak commented Nov 28, 2024

Ready to go again

<configuration>
<parameter>
<excludes>
<exclude>org.glassfish.web.loader.CachingReflectionUtil</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will break semantic versioning, this will require reworking your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an internal class. It was moved into a different module. Nothing should be affected.

Copy link
Member

Choose a reason for hiding this comment

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

We'd strongly prefer that you keep changes in line with semantic versioning - no breaking changes on minor version updates.
Parts of the codebase are shared with Enterprise and extensions, the semantic versioning is in place to help ensure compilation and runtime errors don't unknowingly propagate.

We'll need to make sure this class is not used in any of the non-shared codebase (not just in this repo) before allowing this breaking change to go through.

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 suppose I could deprecate it. More changes to this API are coming from #7097 so stay tuned for that anyway

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 have made the changes to deprecate the class and remove the japicmp exclusion
Thank you

@Pandrex247 Pandrex247 changed the title bugfix: Split CDI bean deployment when multiple WARs exist in an EAR FISH-10049 bugfix: Split CDI bean deployment when multiple WARs exist in an EAR Dec 5, 2024
…I-enabled library JARs

- correctly copy BDA sets for each war in EAR
- Make WAR's CDI beans available in EAR-libs
- read web-fragment.xml from EAR-libs
- processing ear-lib manifest
- de-duplicate BDAs in CDI processing by using LinkedHashSet intead of ArrayList
- made some structures final (cleanup)
- fixed ear and concurrent classloader leaks, including refactored reflection caching
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants