Upgrade jetty jakarta #683
Conversation
Only one bug remains:
Failed startup of context
oeje10w.WebAppContext@2f9a01c1{ROOT,/,b=file:///home/taher/Desktop/active/repositories/fork/moqui-framework/execwartmp/ROOT/webapp/,a=STOPPED,h=oeje10s.SessionHandler@2611b9a3{STOPPED}}{file:/home/taher/Desktop/active/repositories/fork/moqui-framework/moqui.war}
java.lang.IllegalArgumentException: URI is not hierarchical
So we need to find a solution for web.xml being inside the war file
also reintroduce servlet listener that was removed with proper updated version
In MoquiStart.java, I have this very specific very clear instruction:
webappClass.getMethod("setClassLoader", ClassLoader.class).invoke(webapp, moquiStartLoader);
Yet, in MClassLoader, I added these three lines to debug:
System.out.println("=== MClassLoader constructor ===");
System.out.println("MCL = " + this);
System.out.println("MCL parent = " + parent);
AND, look at the outcome:
=== MClassLoader constructor ===
MCL = org.moqui.util.MClassLoader@77f991c
MCL parent = oejew.WebAppClassLoader{Moqui Root Webapp}@6bea52d4
Starting MClassLoader with parent org.eclipse.jetty.ee.webapp.WebAppClassLoader
Jetty is injecting its WebAppClassLoader even though there is a clear
instruction for it to disregard that class loader. Their API is
confusing at best!
This solution solved classpath problems properly by making MClassLoader
a child of StartClassLoader as intended.
With this fix, moqui runs properly on both embedded mode AND when deployed on a servlet container. Need to add documentation on how to test in a servlet container.
Apparently in jetty 12 on EE10+ due to the separation of websockets and EE stack from server stack, we cannot retrieve the HTTP session from websocket handshake. Therefore fixing the bug in both UserFacade and MoquiAbstractEndpoint accordingly. jetty/jetty.project#11809
This reverts commit 2afc70c.
acetousk
left a comment
acetousk
left a comment
There was a problem hiding this comment.
Looks good overall!
Mostly just want to ensure backwards compatibility where possible.
| @Override boolean isRequestedSessionIdValid() { return true } | ||
| @Override boolean isRequestedSessionIdFromCookie() { return false } | ||
| @Override boolean isRequestedSessionIdFromURL() { return false } | ||
| @Override boolean isRequestedSessionIdFromUrl() { return false } |
There was a problem hiding this comment.
Seems like there's a bunch of unnecessary function deletions around here:
- getRealPath etc
Might as well keep them unless there's a good reason to delete.
There was a problem hiding this comment.
Whatever is removed is because it no longer exists. The stub is simply complying with the new signature. You can try to add it and observe the error.
| } | ||
| @Override void sendError(int i) throws IOException { status = i } | ||
| @Override void sendRedirect(String s) throws IOException { logger.info("HttpServletResponseStub sendRedirect to: ${s}") } | ||
| @Override void sendRedirect(String s, int i, boolean b) { logger.info("HttpServletResponseStub sendRedirect to: ${s}") } |
There was a problem hiding this comment.
seems like we should keep the old version and call the new with defaults here
There was a problem hiding this comment.
This is a stub simply used for our testing! no one is using it to keep backward compatibility
There was a problem hiding this comment.
framework/src/main/groovy/org/moqui/impl/screen/ScreenTestImpl.groovy: // create the WebFacadeStub
framework/src/main/groovy/org/moqui/impl/screen/ScreenTestImpl.groovy: WebFacadeStub wfs = new WebFacadeStub(sti.ecfi, stri.parameters, sti.sessionAttributes, stri.requestMethod)
framework/src/main/groovy/org/moqui/impl/screen/ScreenTestImpl.groovy: // get the response text from the WebFacadeStub
This is the only place where it's used. And the tests seem to be passing
| @Override void recipient(String recipient) throws RejectException { recipientList.add(recipient) } | ||
| @Override | ||
| void data(InputStream data) throws RejectException, TooMuchDataException, IOException { | ||
| String data(InputStream data) throws RejectException, TooMuchDataException, IOException { |
There was a problem hiding this comment.
Seems odd to change from void to string just to return null. Is this needed?
There was a problem hiding this comment.
Change in API signature. I am simply following. We don't "need" to return anything for the mail system to work. But if you have a suggestion then by all means.
| MoquiUsernamePasswordValidator(ExecutionContextFactoryImpl ecf) { this.ecf = ecf } | ||
| @Override | ||
| void login(String username, String password) throws LoginFailedException { | ||
| void login(String username, String password, MessageContext messageContext) throws LoginFailedException { |
There was a problem hiding this comment.
We should add convenience overload here probs.
There was a problem hiding this comment.
We are not calling this? We are complying with subetha new signature. When we upgraded subethasmtp we changed the parent of MoquiUsernamePasswordValidator to be the new org.subethamail.smtp.auth.UsernamePasswordValidator which now requires the context. This is being called externally by the consumer, not by us. This is not internal moqui API, but rather just satisfying an external signature.
2 participants
Full migration to jetty 12.1, and EE 11, and all javax to jakarta migrations everywhere. The code and comments and release notes are self explanatory.