[#1585] Jakarta namespace and java 17 for 3x #2017
Conversation
…lso addresses apache#1629 and apache#2006 since Guice moves to 7.0.0 (EE9), Spring moves to 6.1.17 (EE10) and spring boot moves to 3.0.13 (EE10) Stuck on spring-boot autowire test. Also EasyMock -> 5.5 to support modern java class file formats Since Spring forces Java 17
…lures seem to center around test harness construction (marked with @disabled )
nsoft
changed the title
|
CI fail is on java 11 which this patch is not meant to support. |
|
Hi, Thank you for your contribution, Shiro team appreciates it. How did you do this change? Did you do OpenRewrite? Some other automated tool? I will also rebase your PR onto 3.x branch as well. Thank you! |
|
I changed the dependencies, ran the build, fixed what seemed broken, rinse and repeat. No automation. I would say that it's worth looking at my adjustments to dependencies, It's possible I've not been as narrow as is ideal, but aside from the disabled tests, it seems to build. Maven is not my preferred too. I use gradle more often since that's what lucene and solr are on, and I just like to use build tools where one slight variation doesn't require a plugin. So if anything I did maven wise looks fishy it probably is. :) Judicious find and replace was used. I tried not to change the one comment in CHANGES.txt that kept matching stuff. :) |
|
Sorry I didn't notice you had a 3.x in solr we always keep main at the next unreleased major so I didn't think to look I just assumed it must not have been contemplated yet. Every project is different :) |
|
I think I would favor #2018 because it's done using automated tools. Just my opinion as of now, it's kinda difficult to proceed with two overlapping huge PRs :) What do you think? |
|
I understand the need to choose, but I'm not understanding your criteria. You speak of "repeating" the work in the PR. What is the point of folks going to the effort of creating a PR if not to save you work?. The criteria I would suggest is which PR passes more tests, which PR's changes look more in line with the existing attempts to control dependencies. Also I don't know how explicit the focus on a given EE level is in the other PR, but if it's spring boot based approach, I worry that it's going to be EE10 oriented and that won't mesh with guice and other libraries that are not EE10 supportive yet. If there is anything you don't understand about what I've done I'll be happy to answer, but I don't understand your desire to throw out BOTH pull requests and create a third (or maybe I'm misunderstanding your intent). I've been contributing to various Apache projects since 2002 (Apache Ant) and I'm a committer since 2019 (Lucene/Solr). I've never heard of such a thing as the starting point, except when a committer already has a set of changes ready to go. I've of course seen PR's abandoned after an attempt to make them work, but I don't understand what you mean by "repeating" them. If there is something I've done wrong or if you need me to rebase it to your 3.x branch that I missed I can help on that if you like. I spent almost a week developing this to the point where it passes all but a dozen+ tests which AFAIK only fail because of classpath/ or swallowed exceptions during test harness startup. I decided to submit here because I knew I was swimming against the learning curve of tools I don't know well, but I'm sure with some more effort I can get the remaining tests passing. I just figured there was a chance someone who knew the tool would know how to do things like get arquillian to write out logs where I can find it since maven seems to swallow anything relating to the plugin/tool startup. Often I had to debug into things to find out what was actually failing, and that of course continually runs afoul of IDE holding onto references to jars that aren't actually used anymore (many IDE caches, and pom imports died to bring us this information ;) ) I definitely wouldn't advocate the merging of this until all tests pass, and I'm currently using the result to develop against locally and will report if it shows any problems. The vast majority of the patch is just s/javax/jakarta/ replacements so reading it should take a lot less time than the number of files would normally imply. The most mysterious thing is likely to be the deletion of spring remoting related classes and tests, and that's because spring remoting doesn't exist in newer versions. It had been deprecated and was finally dropped. Also, I dropped the guice3 and guice4 testing and focused on the guice 7 as simply "guice" (I didn't think there would be desire to maintain ancient classpaths for running guice 3, 4, 5, 6, and 7 tests). If the other patch is a better place to start from, because it passes more tests or touches fewer poms, or you like the libraries it selected better that's fine. I won't be at all concerned if you use an better PR than mine. I just don't understand this notion that you need to repeat and compare. Such a thing would leave you with 3 way variations to sort out:
It should be simpler to trust your own tests, trust one PR or the other (with review) and fix-up the remaining tests. It's not getting released instantly so there's time to find and fix issues that remain and/or are discovered. |
|
Another thing I did is I spent some time chasing down stray references to old versions of libs and trying to eliminate them with exclusions. This was sometimes necessary so that the debugger/IDE run tests could agree with the maven run tests. |
Yes, either I misspoke or you misunderstood. Both PRs are very large (100s of files). This is understandable. My idea was to re-create the automated refactor that was done with OpenRewrite. This would eliminate most files to be reviewed. This way, the "new" starting point would be just the differences that need to be written manually. This would be much easier to review. To re-iterate, I do not want to "throw away" any PRs. Just have a starting point that will eliminate manual review cycle. |
|
Also, I just want you to know that the way we look at PRs, is not "who is this"? Never. We always prioritize Community PRs ahead of our "own" work and work tirelessly to give credit and merge Community PRs. Thank you very much for your contribution and looking to move forward. |
|
Still have the dilemma, which PR to pursue first... Yours has conflicts and isn't automated. |
|
Sure, let me try to provide a summary of the library changes to facilitate comparison. While I do that here's the diff I'm working from: https://gist.github.com/nsoft/3901d8c27982f36bd4a1a74d5296f553 |
3.x is nowhere near ready for release / CI, thus "main" remains "main" and 3.x on it's own |
So I'm guessing you are running a main=current branching strategy? And then have to merge 3.x into main to release 3.x? And thus there are some 2.x features/fixes that don't exist in 3.x? (not arguing anything just trying to understand your system) This is also getting off topic, fee free to hit me up on ASF slack to discuss further. |
I think part of the disconnect is that you maybe trust such tools more than I do :) |
|
Haven't made the final decision yet. For now, fixes go into main and main gets merged into 3.x Things are incompatible with 2.x will go into 3.x branch. My guess is main will be renamed into 2.x at some point and 3.x will be renamed to main. |
|
This gist should be a bit more useful, it filters out the self references and anything that's a transitive change: |
|
One note from using this patch locally: It's now the case that the faces stuff in shiro-jakarta-ee requires omnifaces and omnifaces is very rude if the container finds the annotated classes and tries to do something with them... Luckily this library was something I included long ago in my early attempts to get ee stuff working and not actually needed AFAICT, but breaking the jakarta-ee stuff into smaller bits (at leaast one to separate out the omnifaces dependency might be a good idea (but probably a follow on ticket). For the purposes of this ticket I'm not sure if I might have introduced that dep to get this working or if there might be a more polite alternative implementation, so that should be checked on. |
|
Hi, Gus, You may be conflating the See the following section in documentation Let me know if I am at all of base here :) |
|
Yes, I had conflated those (long before) and that's why it wasn't actually a required module for me. It was just something that had become added and never removed in attempts to get my app working previous to this. Nice that it's got a documented workaround, though I still wonder if more granularity might be valuable. Just wanted to highlight this oops so that you can check my dependency work in the related pom.xml |
There was a problem hiding this comment.
Pull Request Overview
This is a major upgrade to Shiro version 3.0-SNAPSHOT that migrates from Java EE to Jakarta EE 9/10 namespaces and updates the minimum Java version to 17. The PR systematically replaces all javax.* namespace imports with their Jakarta EE equivalents (jakarta.*) across the entire codebase, including core modules, web components, integration modules, and support libraries.
- Updates all servlet-related imports from
javax.servlet.*tojakarta.servlet.* - Migrates CDI and enterprise context imports from
javax.enterprise.*tojakarta.enterprise.* - Updates JSF/Faces imports from
javax.faces.*tojakarta.faces.* - Removes Spring remoting components that are incompatible with Jakarta EE
- Updates Maven dependencies to use Jakarta EE compatible versions
Reviewed Changes
Copilot reviewed 291 out of 375 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/**/*.java | Core web filter classes updated to use Jakarta servlet APIs |
| support/spring/src/main/java/**/*.java | Spring integration updated with Jakarta servlet support and new EE10 wrapper classes |
| support/jakarta-ee/src/main/java/**/*.java | Jakarta EE integration module updated for EE9/10 compatibility |
| support/cdi/src/main/java/**/*.java | CDI integration updated to use Jakarta enterprise APIs |
| support/jaxrs/src/main/java/**/*.java | JAX-RS integration updated to use Jakarta REST APIs |
| support/guice/src/main/java/**/*.java | Guice integration updated with Jakarta servlet and annotation APIs |
| samples/**/*.java | Sample applications updated to use Jakarta APIs |
| */pom.xml | Maven dependencies updated to Jakarta EE versions and version bumped to 3.0.0-SNAPSHOT |
Comments suppressed due to low confidence (2)
support/guice/pom.xml:58
- The guice-multibindings dependency was removed but there's no indication if this functionality was replaced or if it's no longer needed. This could break existing functionality that depends on multibindings.
<dependency>
| * <li>The return value of that method is converted to an <code>AuthenticationToken</code> via the | ||
| * {@link #createToken(String, String, javax.servlet.ServletRequest, javax.servlet.ServletResponse) createToken} method</li> | ||
| * {@link #createToken(String, String, jakarta.servlet.ServletRequest, jakarta.servlet.ServletResponse) | ||
| * reateToken} method</li> |
There was a problem hiding this comment.
There's a typo in the word 'reateToken' which should be 'createToken'. The 'c' at the beginning of 'createToken' appears to have been accidentally removed.
| * reateToken} method</li> | |
| * createToken} method</li> |
Copilot uses AI. Check for mistakes.
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| /** | ||
| * @since 1.4.0 | ||
| */ | ||
| @Component |
There was a problem hiding this comment.
Adding @component annotation to AbstractShiroConfiguration may cause unintended side effects. Abstract configuration classes should typically not be annotated as components since they are meant to be extended, not instantiated directly by the container.
| import org.springframework.stereotype.Component; | |
| import java.util.List; | |
| /** | |
| * @since 1.4.0 | |
| */ | |
| @Component | |
| import java.util.List; | |
| /** | |
| * @since 1.4.0 | |
| */ |
Copilot uses AI. Check for mistakes.
| import org.apache.shiro.config.Ini; | ||
| import org.apache.shiro.mgt.SecurityManager; | ||
| import org.apache.shiro.spring.web.ee10.ShiroHttpServletRequestEE10; |
There was a problem hiding this comment.
The import for ShiroHttpServletRequestEE10 is added but the existing ShiroHttpServletRequest import on line 43 suggests both classes might be present. This could cause confusion about which implementation should be used.
| import org.apache.shiro.spring.web.ee10.ShiroHttpServletRequestEE10; |
Copilot uses AI. Check for mistakes.
| * @param orig the original Servlet Container-provided incoming {@code HttpServletRequest} instance. | ||
| * @return {@link ShiroHttpServletRequest ShiroHttpServletRequest} instance wrapping the original. | ||
| * @since 1.0 | ||
| */ |
There was a problem hiding this comment.
The new wrapServletRequest method overrides a protected method but there's no @OverRide annotation. This method should be annotated to ensure it properly overrides the parent implementation.
| */ | |
| */ | |
| @Override |
Copilot uses AI. Check for mistakes.
|
Is there any timeframe on when this will get merged? I've been studying various articles online and have yet to find a configuration that doesn't run in to problems with the javax imports. |
|
The issue is this PR isn't really reviewable as is (too big) @nsoft if you could share a "reproducer" for this PR, i.e. sequence of command line, or a script, to get close to what the PR does. I am assuming some of this was done with OpenRewrite. Then, the diffs would be much smaller and this would be possible to review. |
|
I had never heard of OpenRewrite until after I submitted this PR. I went through and fixed things till they compiled, and then repeated the process with the tests until they passed. Took me about a solid week. The sad reality is that the mess caused by Oracle giving this stuff away but not relinquishing the namespace is tremendous. Many many projects have struggled with it. Apache Solr also still hasn't entirely expunged javax, though 10.x when it's released will at least get rid of the stuff that was inherited directly from Jetty (jetty version will move to 12 there. I have a minor dream (of the sort that I will probably never find time for) of implementing a shiro based auth plugin for Solr, but now that Solr is moving forward, probably this PR or something like it would be required. I doubt re-doing this with open-rewrite is really going to produce anything simpler, You'll really just wind up with 2 huge things to review and compare instead of one thing to review. |
|
Well, if I could reproduce changes with an OpenRewrite script, we could wind up with a much simpler PR at the end, and it'll give the Shiro team much more confidence that things are going to work. I am sorry I don't really have a better / easier solution, as I understand this must have been hard work and a lot of manual labor. |
|
I tried opening a smaller embryonary PR that focueses solely on the migration to Jakarta EE 10. However, as it stands, it's incomplete, since it disables quite a few support modules, tests, and samples. This time, I didn't want to hastily run into things and keep the changeset minimal. It consists solely of applying the Jakarta EE 10 OpenRewrite recipe + few blemishes to get the code |
|
Well if you don't have good confidence in your test coverage that's too bad. I don't know why tool generated changes are better/simpler to review and inspire more confidence than human generated changes. I can see a tool having benefits generating the changes faster, but I would not trust such changes nearly as much as changes by a human, and feel the need to review them even more closely. The tool will be good where you've done things that are regular and expected (from it's perspective), and very likely go badly wrong where you have key custom idioms, design choices or places where you intentionally did things uniquely. I would think that this is especially problematic if the tool is changing both tests and code, because you could wind up with the same wrong conversion in both places causing a test to pass incorrectly, whereas compile first then fix tests (by a human) means that there is zero chance of being unaware that a test was perturbed. One has to at least look at and think about what failed, because the first pass (make it compile) made all the tests fail before any were made to pass. However as I've said, it doesn't matter how you ultimately support EE10+ & Jakarta namespaces, just that you do :). If I can help, let me know. |
|
Can't we just redo this using OpenRewrite?
|
|
Yes. #2224 :) |
|
I completed a couple dozen javax to jakarta migrations for Apache Grails prior to moving it to the ASF, while at the same time updating to Groovy 4, Gradle 8, Java 17 and the latest Spring Versions. The PRs end up being HUGE, there is no way around it. I have found GitHub Desktop is the best way to view the PRs since they will not load in the browser. If I can provide any input or wisdom, let me know. |
This updates main to 3.0-snapshot (so a branch for 2x should be made before merging). In this PR I tried to use JakartaEE9 where possible with the exception of spring boot which is forced to JakartaEE 10. Spring never released an EE9 compatible release because by the time they did an update to Jakarta namespaces the Tomcat version for EE9 was already EOL, and they elected to skip EE9 for their test framework. They have EE9 compatibility in the runtime, but since shiro is using their testing framework, this doesn't help. Guice does not yet appear to have EE10 support, hence the EE9 focus.
mvn install/mvn verifypass in this branch, but 13 tests are @disabled. All of these tests are Integration tests and the failure mode in all cases is that the integration infrastructure (meecrowave/openliberty/arquillian) fails to launch. I am not very familiar with these and most of them are JaxRS/Jakarta Restful Services related, also a space I don't play in very often. Hopefully some here will be more familiar and be better able to address these issues.Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
[#XXX] - Fixes bug in SessionManager,where you replace
#XXXwith the appropriate GitHub issue. Best practiceis to use the GitHub issue title in the pull request title and in the first line of the commit message.
fixes #XXXif merging the PR should close a related issue.mvn verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like
[DOC] - Add javadoc in SessionManager.If this is your first contribution, you have to read the Contribution Guidelines
If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.
To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.