Repository navigation
Feature/1004 update javalin drop tomcat - #1985
MikeNeilson wants to merge 86 commits into
Conversation
|
NOTE: at best I will have the annotations updated today. Getting the API running again in it's standalone mode will likely take until the end of the week if not longer. |
|
For the record. I have considered keeping with tomcat and just using like embedded tomcat for the packaging, but Javalin was made for Jetty so since that would be a fair amount of work anyway, better to use a tool that was actually built for this given style of packaging an app. |
|
FYI, I am aware this PR is rather huge. The OpenAPI changes basically have to be all or nothing. I think we can all agree on that. It also requires the migration to Jakarta EE (javalin update) which would may or may not (java.security.Principal so probably not) affect the CwmsAaaIdentityProvider. So fair amount of changes to Tomcat anyway. So is this likely to be a pain... yes, but I'm arguing it's worth it. I could pivot back and do the move to standalone Javalin (e.g. Jetty whatever version that was) first, but then without updating javalin frankly it wouldn't be in a releasable state (security concerns, that's a really old Jetty) So. for now I'm going to barrel through. I'm open to addressing concerns and pivoting if they're strong enough. We do have a lot of plans. and the bulk are passing. What's failing is most likely various validation I've intentionally disabled due to the location of initialization changing. |
e041252 to
0263cc0
Compare
Documentation build overview
13 files changed ·
|
|
I'm still working through dealing with the lock icons. the behavior as a whole has change. I may need to be drastic and take the given out put to json, and put it through the Swagger OpenAPI object, but i'm hoping to do it all within the javalin openapi annotations. I think most of issue is assumptions we previously made that are no longer required that may be getting in the way. The primary work of the Javalin update and removal of Tomcat are finished and should be reviewed. I think I got all of the paths in correctly, but it's definitely possible that some have been missed that also weren't detected by the various validations, new and old. NOTE: This does break /CWMSLogin. That is intentional and will be dealt with in a follow up. Once merged CDA is a standalone app like the CWMS Server apps and thus won't be getting randomly deployed to a T7 any time soon. |
33e7c77 to
c7d3788
Compare
Co-authored-by: Ryan Ripken <89810919+rma-rripken@users.noreply.github.com>
d5e5583 to
6d683a7
Compare
|
@rma-rripken @rma-psmorris @krowvin @msweier @oskarhurst There's also a fair amount of follow up work to be had given the known breaking changes for certain environments. |
|
fixes for the audit are over in #2013, may or may not just have to do the same npm audit fix a few times to clear through it. |
Actually, that was just easier to do. |
oskarhurst
left a comment
oskarhurst
left a comment
There was a problem hiding this comment.
I think Openapi uses camelCase by default but our current scheme used kebab case for places like location. shoould we give the location dto the annotation @OpenApiNaming(OpenApiNamingStrategy.KEBAB_CASE) to make sure it matches.
| logger.atFine().log("Starting queue subscriber for RSS feed."); | ||
| QueueManager.ensureRssSubscribers(ds); | ||
| logger.atFine().log("Starting CDA."); | ||
| app.start("127.0.0.1", port); // Always localhost, we only allow containerization or through proxy. |
There was a problem hiding this comment.
Is it correct to hard code to 127.0.0.1 wont this not let it communicate with other containers in our compose like trafik?
There was a problem hiding this comment.
In this case no. when in a container it won't matter, the container system can still route to something running on that containers "localhost".
This is to prevent downstream non-containerized usages from allowing direct connections without a bunch of work.
| secReqs.clear(); | ||
| authenticator.getActiveProviders().forEach(identityProvider -> { | ||
| api.getComponents().addSecuritySchemes(identityProvider.getName(),identityProvider.getScheme()); | ||
| api.withSecurityScheme(identityProvider.getName(), identityProvider.getScheme()); |
There was a problem hiding this comment.
Doesn't withSecurityScheme throw for identity providers that are null. I would say we should probably just ignore, catch, or skip them. To my understanding openapi with return null if the configuration is unset or the discovery fails, but we dont want it to throw just because the first request to keycloak fails right?
There was a problem hiding this comment.
Yep, the previous handling did not, this one does, I put a null check in.
Sorry, misunderstood what you were saying to me directly.
|
NOTE, we apparently need to add Annoying, yes, but the OpenAPI spec is now generated at compile time and doesn't use the mapper we've configured.... why it doesn't used the FasterXML annotation we already have... no idea. Will deal with it tomorrow. |
Summary
Move to latest javalin (7.2.3)
Remove Use of Tomcat, no Tomcat 11+ in our local environment so will just use Javalin's default jetty and package it up.
NOTE: Due to Javalin updating the annotation plugin (this happened in javalin 5) this is going to be a large change as it touches every file and will need to alter how the OpenAPI static test works.
TODO:
Related Issue
Validation
Existing integration tests
Checklist