-
Notifications
You must be signed in to change notification settings - Fork 467
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
Comments
### 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>
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: We now have a Regarding CDI: 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
}
} |
…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>
While working on the new job queue system I was able to validate CDI is working correctly. |
…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>
Fixed, tested on trunk // Docker // FF Note: Every one of the mentioned APIs have their corresponding postman test running and no failures reported.
|
Create branch based upon https://github.com/dotCMS/core/tree/steve-poc-add-cdi
Thorough testing of existing code and endpoints
The text was updated successfully, but these errors were encountered: