Proposed fix for https://github.com/networknt/openapi-parser/issues/43#56
Conversation
|
Just fixed (de5e134) missing delegation to three methods that I had accidentally overlooked... |
|
@AndreasALoew Is it completed? If yes, I will merge it and try it out. Thanks. |
|
Yes, it's complete now and works fine. We passed all our project-specific tests as well 😄 You can now (by commenting in/out version property and dependency) choose between
Happy testing! 👍 |
|
@AndreasALoew One of our users reports an issue with light-4j 2.1.38-SNAPSHOT version due to this change. With Java 11, there are some warning message like below. With Java 17 and above, the server cannot start anymore with the following error messages. It looks like we cannot use reflection or we need to change the way we use reflection. What do you think? Thanks. |
|
Hi @stevehu , good news: both user reports issues are easily resolved and do not require any changes to the codebase. The first one is due to the fact that from Java 9 (with the introduction of the module system), reflective access between packages needs to either be included in the module info files (which we obviously cannot do for JDK packages, as we don't own them), or otherwise "opened up" by JVM command-line options on startup: For older versions such as JDK11, it is sufficient (though not great practice) to simply use the default of To get fully rid of the warning, you need to "open up" the JDK modules for access by "unnamed module" (such as in our case openapi-parser and/or light-4j): In line 270 added here: Line 270 in 0f8c89f you can find the option: --add-opens=java.base/java.lang=ALL-UNNAMED that is needed to do this opening without causing an error message.
From Java 17, the So can you please try to add Of course, you also need to take care of whether you want/need the "javax" or the "jakarta" version of the mail implementation in your classpath - change the dependency from Lines 141 to 145 in 0f8c89f to Lines 147 to 151 in 0f8c89f to switch from javax (latest is 1.6.x) to jakarta (2.1.y) versions... Please report back on your success... 🤞 Best regards |
|
Given that we have some users on Java 17 and Java 21, we have to roll back the changes so that they can use the snapshot version for internal tests. As more and more users are moving to the Graalvm for native images, reflection is not just an issue for now but a bigger issue in the future. I am wondering if we can pick up one of the libraries without reflection. In our current use case, we are using javax.mail and it is sufficient enough as we are not using JEE. |
|
OK, @stevehu , while I can understand your point, it still is sad and a pity. 😢 If your main goal indeed is to get fully rid of/stay compeletly free of Java reflection (the usage of which btw, can also be "announced"/configured beforehead for GraalVM native images: https://www.graalvm.org/latest/reference-manual/native-image/metadata/#reflection ), then I am at my wits' end - and our ways should likely spread up here, me doing just another fork of your "openapi-parser": Our scenario is plain "good old" Java/Jakarta EE, where we MUST switch to Jakarta EE 10 and thus, "jakarta.mail" in the next couple of weeks. Staying with the old "javax.mail" as you propose is a true NO-GO for us even for legal reasons (Oracle is not allowing jaxax packages owned by Jakarta EE later than Java/Jakarta EE8 any more!). But I can perfectly understand that if light-4j is still on the "javax" side of things (IMHO, it can't stay there forever!), you don't like this. Sorry - but if want to keep the code base free of the use of Reflection, the only way you can do is split up your repo into two major version branches: one old version linking with "javax.mail" without reflection, and one new version linking with "jakarta.mail" without reflection. Many third-party products do this by opening up a new major version (i.e. keep an old - still maintained - 2.1.x branch using "javax", but create a new 3.y branch including main for "jakarta"). But if you want to stay in "main" with "javax.mail", this is a valid statement. So if you prefer to revert "my" commit, I would move forward and fork your repo one more time for Jakarta EE usage - just let me know. Thanks & best regards |
|
@AndreasALoew My goal is to support as many users as possible. Since the InternetAddress is used only to validate the email in the specification, we can find an alternative way to do that. I am looking at the following link and there are two good candidates (apache-validation and regex). I haven't spent time digging into the details yet but will spend some time later. What do you think about this direction? Thanks. |
|
OK, well - that opens up the space to look for more and indeed intersting solutions. Unfortunately, you didn't include the link you mentioned, but I would NOT opt to try and "build your own" using regexs, as the RFCs behind are awfully complex and they define the offical rules set that must be matched. I myself have already run across https://github.com/RohanNagar/jmail - which you can even check out at https://www.rohannagar.com/jmail/ testing for its behaviour (make sure to tick at least some of the Advanced Options, otherwise IMHO it is too lax!), but reading through its README markdown document, it both has the spec-related "RFC" background as well as even more configurability than you may ever need... 😉 So if you can provide some more information to me as to which of the Advanced Options you would like to check for (see docs at https://github.com/RohanNagar/jmail/blob/master/README.md and the testing page), I can then again create a proposed patch; this time by replacing " Looking forward to your opinion on this solution path...😄 |
|
Sorry. This is the link I was read. https://stackoverflow.com/questions/624581/what-is-the-best-java-email-address-validation-method I also asked AI and it gave me the following answer. I think this is good enough. What we are doing is to validate the email in the contact section of the specification. As long as the format is correct, we really don't care if the email address is real or not. What do you think? |
|
Well, if I were in your shoes, I'd rather be careful and NOT follow AI advice (if AI has read some nonsense frequently enough, it thinks it's "true" as it has the highest probability - only true if all information sources were equally valid, which they ARE NOT)... :sad: I'd rather choose There also is https://github.com/bbottema/email-rfc2822-validator BUT it still has an optional (and only "javax"-style, NOT "jakarta" YET...) optional dependency to the "javax.mail" implementation, which means we would not win anything: "Note: Jakarta Mail is now an optional dependency, which you need to add yourself, but only if you use the parsing facilities of this library (rather than only the validation function)"... :sad: So if you would be so kind as to tell me which of jmail's optional features you would want to activate, I volunteer to create a new proposed patch of "openapi-parser", replacing "javax/jakarta mail" by "jmail" and send you a new PR... Thanks & best regards, |
|
... and please also look at the comparison table between these options in the lower half of this page: (don't know how the AI proposed simple regex setup would finish here, but I assume last place...) |
|
@AndreasALoew Yes. Jmail looks very good. Let's use it to replace the javax.mail. That is a very good pick. |
ghost
mentioned this pull request
1 participant
Proposed fix for #43 along the lines of what I described there:
Using Java Reflection API, dynamically loading either the "jakarta" (1st prio) or the "javax" (2nd prio) implementation of the target classes as the delegate objects, and putting delegating proxies in front of them as part of the "openapi-parser" project which forward all calls to the respective delegates...