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

Enable CDI : Timebox - Check if existing work can be easily merged now #29552

Closed
spbolton opened this issue Aug 13, 2024 · 3 comments · Fixed by #29678, #29800 or #29822
Closed

Enable CDI : Timebox - Check if existing work can be easily merged now #29552

spbolton opened this issue Aug 13, 2024 · 3 comments · Fixed by #29678, #29800 or #29822

Comments

@spbolton
Copy link
Contributor

spbolton commented Aug 13, 2024

Create branch based upon https://github.com/dotCMS/core/tree/steve-poc-add-cdi

Thorough testing of existing code and endpoints

@spbolton spbolton self-assigned this Aug 13, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 15, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 15, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 18, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 19, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 19, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 20, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 20, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 20, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 21, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 21, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 21, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 21, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 21, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 23, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 23, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 23, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 28, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 28, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 28, 2024
@fabrizzio-dotCMS fabrizzio-dotCMS linked a pull request Aug 28, 2024 that will close this issue
fabrizzio-dotCMS added a commit that referenced this issue Aug 29, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 29, 2024
### Proposed Changes
* In this PR we are upgrading to jersey  2.28
* We are changing DotRestApplication to use package discovery to load
Resources and Providers
* We're making some adjustments on some Mappers to preserve the present
behavior
* We're introducing CDI through Weld container

---------

Co-authored-by: spbolton <steve.bolton@dotcms.com>
@fabrizzio-dotCMS
Copy link
Contributor

fabrizzio-dotCMS commented Aug 29, 2024

Note to QA: We upgraded to a more recent version of Jersey for this task, specifically 2.28. This update introduced some issues that were subsequently resolved. For instance, form validation in the /api/environment endpoint stopped functioning correctly; the response lost its JSON format and started being returned as plain text. This issue was successfully addressed.

However, it would be a good idea to test other endpoints that perform similar validation, such as:

The secret creation endpoints. Test by sending both valid and invalid values, and verify against previous versions of the endpoints:
POST: /v1/apps/{key}/{siteId}
POST: /v2/languages/
Additionally, we need to test loading dynamic plugins that deploy a new REST endpoint.

We now have a JerseyApplicationEventListener class that allows us to track the loaded resources. It can be used alongside the previous code to generate a list comparing the current resources with the expected ones, helping to detect if something is missing or if there are any extra resources.

Regarding CDI:
to test CDI integration
Here's a starting point in the class:
com/dotcms/cdi/CDIUtils.java
There is an example in
src/test/java/com/dotcms/cdi/SimpleInjectionIT.java
There's another example of the actual working code in the DotRestApplication
Where there is a Singleton that used to be created manually and got replaced by injection using the Annotation @ApplicationScope
Please note that @Singleton isn't going to work
Here's why https://stackoverflow.com/questions/44097337/how-to-make-a-singleton-in-cdi-1-2
I decided to leave it out since we pretty much can accomplish the same with @ApplicationScope

So we should limit to https://docs.jboss.org/cdi/spec/1.2/cdi-spec.html#bean_defining_annotations

Here's an interesting pattern that uses Weld on a StandAlone App

public class Main {

    public static void main(String[] args) {
        Weld weld = new Weld();
        try (WeldContainer container = weld.initialize()) {
            // Doing this 
            MyApplication app = container.select(MyApplication.class).get();
            app.run();
         }
         
           //Is Equivalent to doing this :
            final MyApplication myApplication = CDI.current().select(MyApplication.class).get();
            myApplication.run();

           //So on a Web Application where we aren't supposed to have direct access to the Weld Container 
           //We should do the later 

           // This could easily be our entry Point class that Injects all the services etc..  
           // At some point, we could do this to our APILocator so all new services will get CDI            
           //for now I would recommend we use a Class to serve as the locator of our new API and do that initialization there 

    }
}

fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 30, 2024
This reverts commit 11358b0.

This change breaks the API Playground 
as it adds an extra API prefix to the URLs like this:


http://localhost:8080/api/api/v1/contenttype?orderby=upper%28name%29&direction=ASC
fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
fabrizzio-dotCMS added a commit that referenced this issue Aug 30, 2024
fabrizzio-dotCMS added a commit that referenced this issue Sep 3, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 3, 2024
…s:29552 (#29822)

Proposed Changes
In this PR we are upgrading to jersey 2.28
We are changing DotRestApplication to use package discovery to load
Resources and Providers
We're making some adjustments on some Mappers to preserve the present
behavior
We're introducing CDI through Weld container
This PR fixes: #29552

---------

Co-authored-by: spbolton <steve.bolton@dotcms.com>
@nollymar nollymar reopened this Sep 4, 2024
@jgambarios
Copy link
Contributor

While working on the new job queue system I was able to validate CDI is working correctly.

@jgambarios jgambarios removed their assignment Sep 10, 2024
dsolistorres pushed a commit that referenced this issue Sep 18, 2024
…s:29552 (#29822)

Proposed Changes
In this PR we are upgrading to jersey 2.28
We are changing DotRestApplication to use package discovery to load
Resources and Providers
We're making some adjustments on some Mappers to preserve the present
behavior
We're introducing CDI through Weld container
This PR fixes: #29552

---------

Co-authored-by: spbolton <steve.bolton@dotcms.com>
@bryanboza
Copy link
Member

Fixed, tested on trunk // Docker // FF

Note: Every one of the mentioned APIs have their corresponding postman test running and no failures reported.

POST: /v1/apps/{key}/{siteId}
POST: /v2/languages/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment