Refactoring around WebBundleDescriptor and EjbBundleDescriptor #24337

dmatej · 2023-03-10T16:25:47Z

Context:

CargoTracker touched several issues.
Some were already resolved and fixes were merged in Cargotracker discoveries part one #24335
Some I have "fixed" locally, but I did't believe the fix is alright. So I started writing unit tests but immediately collided with the fact that these two classes are not mockable and I can't even create some trivial fake implementations. So we have just integration tests, slow and covering just one of scenarios while I need a code coverage going through many possibilities. Even then there could be a mistake, but I still need to be able to just extend tests in 5 minutes.

So, here we go.

new javadocs (and comments for methods used by reflection)
new tests
removed obsoleted methods with alternatives + updated usages
renamed "hacks"
methods with just a single usage moved to where they are used, simplified.
some bugs were fixed by refactoring (not in between, but directly "by")

Fixed issues:

Validation of resource bundles in deployment - spammed logs without a reason (CargoTracker did it)
Several NullPointerExceptions when deployment failed (broken CargoTracker state)
Now it should be possible to write more unit tests for DOL, open door for future iterations of refactoring.

Tests:

TCK: https://ci.eclipse.org/jakartaee-tck/job/jakartaee-tck/job/10.0.x/50/ (all passed)
CargoTacker on JEE10 - in progress, there's probably yet one bug, but I will create another PR for it as it seems not related to descriptors.

Breaking changes:

Changed API of several classes in deployment, it may affect connectors despite I tried to keep some consistency.
However I doubt someone outside GF would be able to extend/implement those APIs to work well with GlassFish as the API is something horrible; too many dependencies between classes, circular relations, etc.

- Target: make unit tests possible Signed-off-by: David Matějček <[email protected]>

Signed-off-by: David Matějček <[email protected]>

- originally it required an exact match of the parameter's type - now it first tries the exact match, when no such method exists it tries to find any compatible method of the same name. Signed-off-by: David Matějček <[email protected]>

- instead of abstract methods moving their implementation as a default impl - It is quite complicated work affecting many places, so I will do rather smaller steps. - about generics - the coverage is not perfect because of cyclic dependencies Signed-off-by: David Matějček <[email protected]>

- all classes used the same method. Signed-off-by: David Matějček <[email protected]>

- broke connectors in tests; maybe it wouldn't be an issue for user, but I am not sure with that. I will find safer path. Signed-off-by: David Matějček <[email protected]>

- default implementations - EjbApplicationExceptionInfo Signed-off-by: David Matějček <[email protected]>

- Issue: I tried to use mocking, but I have found that it doesn't work well and even if, it would not cover too much of the implementation. - dol and ejb-container have too many relations, in fact the abstraction was unusable and worthless (and even undocumented). - I can create mocks for interfaces but I can't use them for validator. - I can create half-initialized mock of the abstract class, but it throws exceptions which take too much time to investigate and finally I find that the issue is the mock, not the code I wanted to test. - Same would apply for custom implementations (like is the only one, ejb-container) - too much details matter. - In practice: dol should have much better API, OR all descriptors and nodes whould move to DOL. They can be extended and used in ejb-container, but must have some basic consistent meaningful implementation. - So I do some basic steps forward now. - Final result: test covering bug we have noticed in logs + the fix not breaking anything else. Side effect result: easier future changes. Signed-off-by: David Matějček <[email protected]>

- it was already checked for null Signed-off-by: David Matějček <[email protected]>

- This is safe as there is only one child of this abstract class Signed-off-by: David Matějček <[email protected]>

- Unused methods removed - Used methods documented (at least minimal notes) - Deprecated XML nodes from GF 3.1 marked as deprecated and removed code which used them but was worthless. Now they should be just tolerated. Signed-off-by: David Matějček <[email protected]>

Signed-off-by: David Matějček <[email protected]>

…tion - one of intermediate steps - moving methods Signed-off-by: David Matějček <[email protected]>

- validators cover possible issues just partially - better break deploy asap with more helpful info, especially when it is about glassfish issues. Signed-off-by: David Matějček <[email protected]>

- broken in recent commit of the same PR - also fixed some logs and formatting Signed-off-by: David Matějček <[email protected]>

- most of issues were more visible when I caused some inconsistency - the commit is unfortunately huge, but necessary - the architecture of the DOL is broken, always was - depends on reflection across modules and packages Signed-off-by: David Matějček <[email protected]>

- Fixes the issue when DOL used nulls because singletons weren't found in HK2 - Affected appclient, tests, validations, processing descriptors, etc. Signed-off-by: David Matějček <[email protected]>

Signed-off-by: David Matějček <[email protected]>

- seen when deploying broken CargoTracker Signed-off-by: David Matějček <[email protected]>

Signed-off-by: David Matějček <[email protected]>

- foreach, avoiding for-keys-find-value, Vector replaced by ArrayList - formatting, generics, javadocs Signed-off-by: David Matějček <[email protected]>

smillidge · 2023-03-21T15:00:31Z

This is a large broad set of changes touching a number of subsystems and 212 files. Not sure I can review it tbh. I'd have to assume it is correct.

arjantijms · 2023-03-21T15:01:27Z

This will take me a little time to work through, but I agree with the general direction.

arjantijms · 2023-03-21T15:28:17Z

...enterprise/connectors/deployment/annotation/handlers/ConnectionFactoryDefinitionHandler.java

@@ -268,7 +160,7 @@ private ConnectionFactoryDefinitionDescriptor createDescriptor(ConnectionFactory
        desc.setMaxPoolSize(defn.maxPoolSize());
        desc.setMinPoolSize(defn.minPoolSize());

-        if (defn.description() != null && !defn.description().equals("")) {
+        if (defn.description() != null && !defn.description().isEmpty()) {


Could also use the GF utility method for this;

com.sun.enterprise.util.Utility.isEmpty(String)

dmatej · 2023-03-21T16:42:56Z

This is a large broad set of changes touching a number of subsystems and 212 files. Not sure I can review it tbh. I'd have to assume it is correct.

I completely agree that it is horrible, but I have no idea how to move from "swamps of DOL" by a different way. I could make a PR after every safe point, but even then it would not help too much. Everything is fragile, in previous iterations I learned some "rules" how to avoid breaking things. Also in the first iteration it took a whole day to find out where I did the bad step, this time it was much faster.

There are overlapping interfaces and methods, removing one can break deployment or related things despite it compiles. Many reflections used to set attributes or just call methods; when they are just setters or "adders", it is easy, but sometimes they depend on the order of execution, which is fragile, fragile, fragile ...

That leads to some "stack" and "routine" in refactoring. And also thinking about next iterations.

Reviewing it by reading code is nearly worthless (except well visible mistakes I really could make), that's why I always run the TCK and why we were helping the TCK (so we can run it against GF PR), in last two years we also (partially) fixed the "Zombie Army" (Ant tests, I'm often checking if the count of passing tests did not decrease), now I'm trying to make friends of GF and CargoTracker (the last issue: derby pool).

Good thing is that in the future it should converge to some more ideal state and those PRs should be smaller and smaller. I believe that creating them is still more painful than reading them.

hs536

it passed the TCK, so we could say it meets the minimum quality requirements. although incompatibilities should be carefully checked afterwards, I'd welcome modifications that would make GlassFish more testable.

dmatej force-pushed the cargotracker-discoveries branch 6 times, most recently from 36579eb to 0b1245f Compare March 17, 2023 13:48

dmatej force-pushed the cargotracker-discoveries branch from 360cda6 to 39c7e7b Compare March 19, 2023 10:10

dmatej added 21 commits March 19, 2023 22:13

Refactored WebBundleDescriptors

aa2137e

- Target: make unit tests possible Signed-off-by: David Matějček <[email protected]>

Further refactoring - replacing unused/obsoleted methods, added javadocs

466b608

Signed-off-by: David Matějček <[email protected]>

ResourceAnnotationControl - moved copy pasted code

e6bef17

- all classes used the same method. Signed-off-by: David Matějček <[email protected]>

Partially reverted incompatible changes in EJB descriptors

c9343b0

- broke connectors in tests; maybe it wouldn't be an issue for user, but I am not sure with that. I will find safer path. Signed-off-by: David Matějček <[email protected]>

Moving another methods to EjbBundleDescriptor + javadoc

fb89c21

- default implementations - EjbApplicationExceptionInfo Signed-off-by: David Matějček <[email protected]>

Fixed HK2 dependency - securityRoleMapperFactoryis optional

1f5ba67

- it was already checked for null Signed-off-by: David Matějček <[email protected]>

EjbBundleDescriptor - moving more fields to the parent

e36eec8

- This is safe as there is only one child of this abstract class Signed-off-by: David Matějček <[email protected]>

ComponentValidator formatting + fixed possible NPE

bb5199f

Signed-off-by: David Matějček <[email protected]>

Long process of resolving DOL cyclic dependencies resolved via reflec…

f7a9771

…tion - one of intermediate steps - moving methods Signed-off-by: David Matějček <[email protected]>

More strict DeploymentDescriptorNode - throwing exceptions

5ab479d

- validators cover possible issues just partially - better break deploy asap with more helpful info, especially when it is about glassfish issues. Signed-off-by: David Matějček <[email protected]>

Fixed comparison (carType() can return null!)

11bff77

- broken in recent commit of the same PR - also fixed some logs and formatting Signed-off-by: David Matějček <[email protected]>

Archive types moved to DOL

96e9429

- Fixes the issue when DOL used nulls because singletons weren't found in HK2 - Affected appclient, tests, validations, processing descriptors, etc. Signed-off-by: David Matějček <[email protected]>

Fixed resource validation + partial test coverage

56a6b13

Signed-off-by: David Matějček <[email protected]>

Imports in Globals

a5e7374

Signed-off-by: David Matějček <[email protected]>

Fixed nullpointers in connectors-runtime

ea3d9f1

- seen when deploying broken CargoTracker Signed-off-by: David Matějček <[email protected]>

Removed unused import (Disabled) from test

224fb08

Signed-off-by: David Matějček <[email protected]>

dmatej force-pushed the cargotracker-discoveries branch from 39c7e7b to 224fb08 Compare March 19, 2023 21:14

Removed unused methods and dead ends, smaller optimizations

14602ed

- foreach, avoiding for-keys-find-value, Vector replaced by ArrayList - formatting, generics, javadocs Signed-off-by: David Matějček <[email protected]>

dmatej marked this pull request as ready for review March 21, 2023 10:55

dmatej requested review from arjantijms, pzygielo, smillidge, anajosep, breakponchito, gerdogdu, gurunrao, hs536 and jGauravGupta and removed request for pzygielo March 21, 2023 10:55

dmatej self-assigned this Mar 21, 2023

dmatej added bug Something isn't working build and test improvement breaking change Changes something users / app devs labels Mar 21, 2023

dmatej added this to the 7.0.3 milestone Mar 21, 2023

arjantijms reviewed Mar 21, 2023

View reviewed changes

arjantijms approved these changes Mar 21, 2023

View reviewed changes

hs536 approved these changes Mar 22, 2023

View reviewed changes

arjantijms merged commit b2ff8f8 into eclipse-ee4j:master Mar 23, 2023

dmatej deleted the cargotracker-discoveries branch March 23, 2023 15:32

dmatej mentioned this pull request Mar 28, 2023

Cleanups in connectors logging #24361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring around WebBundleDescriptor and EjbBundleDescriptor #24337

Refactoring around WebBundleDescriptor and EjbBundleDescriptor #24337

dmatej commented Mar 10, 2023 •

edited

Loading

smillidge commented Mar 21, 2023

arjantijms commented Mar 21, 2023

arjantijms Mar 21, 2023

dmatej commented Mar 21, 2023 •

edited

Loading

hs536 left a comment

Refactoring around WebBundleDescriptor and EjbBundleDescriptor #24337

Refactoring around WebBundleDescriptor and EjbBundleDescriptor #24337

Conversation

dmatej commented Mar 10, 2023 • edited Loading

smillidge commented Mar 21, 2023

arjantijms commented Mar 21, 2023

arjantijms Mar 21, 2023

Choose a reason for hiding this comment

dmatej commented Mar 21, 2023 • edited Loading

hs536 left a comment

Choose a reason for hiding this comment

dmatej commented Mar 10, 2023 •

edited

Loading

dmatej commented Mar 21, 2023 •

edited

Loading