Repository navigation
Feature/1004 update javalin drop tomcat - #1985
MikeNeilson wants to merge 76 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
| .withUiWar(new File(uiPath)) | ||
| .withDataSource(ds) | ||
| .build(); | ||
| api.start(); |
There was a problem hiding this comment.
| api.start(); | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| api.stop(); | |
| if (ds instanceof AutoCloseable closeable) { | |
| try { | |
| closeable.close(); | |
| } catch (Exception ex) { | |
| logger.atWarning().withCause(ex).log("Error closing data source."); | |
| } | |
| } | |
| }, "cda-shutdown")); | |
| api.start(); |
|
|
||
|
|
||
|
|
||
| //TODO: The rest |
There was a problem hiding this comment.
Are these gonna get added back or dropped. Some of this commented out stuff seems important
There was a problem hiding this comment.
Still figuring that out. I did get the scheme processor working,
but the returned OpenAPI object is now a builder from javalin-openapi that doesn't behave the same way... plus most of what we were doing here can no be moved to the controllers, like examples.
so I might decide to let those go for this PR, and then come back and do a follow up on them. As much as I want it all to work. at a minimum if this gets merged into main it needs to work for existing usage. And there's already new work that not going to be super trivial to rebase into this.
There was a problem hiding this comment.
Put a comment in explaining, got the security elements working again. Decided to drop the examples as the method of handjamming it in like that will change and the new system should handle it better so it should just be migrated.
Co-authored-by: Ryan Ripken <89810919+rma-rripken@users.noreply.github.com>
Co-authored-by: Mike Neilson <gamingmike@gmail.com>
b3c754c to
b34c4d2
Compare
There was a problem hiding this comment.
I thought you were going to delete this?
There was a problem hiding this comment.
yep... and then totally forgot about it.
Do you want a list of controllers that mismatch? Otherwise this PR is going to break a bunch of clients - right? I asked Claude to check the paths vs the ApiServletRouteConfiguration. Thats this list: ProjectController POST /project and PATCH/DELETE /project/{name} (route /projects), |
Yeah, that would be helpful for sure. I thought I went from top to bottom, though even if I did I clearly missed a bunch. |
| description = "Update CWMS Property", | ||
| method = HttpMethod.PATCH, | ||
| methods = HttpMethod.PATCH, | ||
| path = "/properties{" + NAME + "}", |
There was a problem hiding this comment.
this probably needs a slash
| }, tags = {TAG} | ||
| }, tags = {TAG}, | ||
| headers = {@OpenApiParam (name = CwmsDataApi.IS_NEW_LRTS, description = CwmsDataApi.LRTS_DESCRIPTION)}, | ||
| path = "/timeseries/identitifer-descriptor/{" + TIMESERIES_ID + "}" |
There was a problem hiding this comment.
| path = "/timeseries/identitifer-descriptor/{" + TIMESERIES_ID + "}" | |
| path = "/timeseries/identifer-descriptor/{" + TIMESERIES_ID + "}" |
| methods = {HttpMethod.DELETE}, | ||
| tags = {TAG}, | ||
| headers = {@OpenApiParam (name = CwmsDataApi.IS_NEW_LRTS, description = CwmsDataApi.LRTS_DESCRIPTION)}, | ||
| path = "/timeseries/identifier/{" + TIMESERIES_ID + "}" |
There was a problem hiding this comment.
| path = "/timeseries/identifier/{" + TIMESERIES_ID + "}" | |
| path = "/timeseries/identifier-descriptor/{" + TIMESERIES_ID + "}" |
This branch has not been deployed
2 participants
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