Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Java Servlet API 3.0.1 to Jakarta Servlet API #1269

Open
sandeepnkulkarni opened this issue May 13, 2022 · 16 comments
Open

Upgrade Java Servlet API 3.0.1 to Jakarta Servlet API #1269

sandeepnkulkarni opened this issue May 13, 2022 · 16 comments

Comments

@sandeepnkulkarni
Copy link

Currently CoreNLP project uses old version of Java Servlet API 3.0.1. It was released way back in 2011. Security scanner is showing it as EOL version.
Is it possible to upgrade to newer Jakarta Servlet API 4.0.4 at least? It supports Java 8, so it will not break any backward compatibility. There are guide available online about moving from javax to jakarta namespace.

@AngledLuffa
Copy link
Contributor

What happens to any projects which happen to be using the old stuff still if we update? Although I suppose the opposite argument works for projects which have migrated and still want to use CoreNLP.

Also, how would updating this interact with things such as Tomcat?

Ultimately I think we'd be happy to take a PR with this change, but our threshold of doing it ourselves would be "super bored and nothing else going on"

@eduarddrenth
Copy link

Staying in sync with developments in the stuff (java in this case) you use imo is a must. Jakarta ee 10 and the new jdk versions have a lot to offer. I would upvote to systematically invest time in this. Now you are glueing users of your great library to old libraries.

@AngledLuffa
Copy link
Contributor

You're not wrong. The issue is that there is one person qualified and available to do it, and unfortunately they do not fall in the "motivated" circle on that Venn diagram.

Maybe I ... er ... that person will try to find time before the end of the year

@eduarddrenth
Copy link

Maybe I can help, I have a lot of experience in java/xml/ee/jaxb etc. I do not have a lot of time though
Is the xml part in the lib restricted to the configuration?

@AngledLuffa
Copy link
Contributor

If only we could find a person with time :)

I know there's XML used internally as part of the SUTime representation, but I don't believe that's part of the java / jakarta issue

Maybe I can start digging into compatibility some today and see how much work it really is

@AngledLuffa
Copy link
Contributor

I don't think any of these uses are particularly challenging, so hopefully they can all be replaced with the newer version:

src/edu/stanford/nlp/ie/ner/webapp/NERServlet.java
src/edu/stanford/nlp/naturalli/demo/CORSFilter.java
src/edu/stanford/nlp/naturalli/demo/OpenIEServlet.java
src/edu/stanford/nlp/patterns/SPIEDServlet.java
src/edu/stanford/nlp/pipeline/webapp/CoreNLPServlet.java
src/edu/stanford/nlp/time/suservlet/SUTimeServlet.java

@AngledLuffa
Copy link
Contributor

Oh, even the namespaces are apparently the same in servlet 4.0.4? This is a lot easier than I thought

@AngledLuffa
Copy link
Contributor

#1388

(will have to test it, including bringing back the currently defunct nlp.stanford.edu:8080 demos)

AngledLuffa added a commit that referenced this issue Oct 10, 2023
Remove the old javax.servlet jar (4.0.4 apparently has fewer security issues)

Update all servlet uses in .xml files to use jakarta 4.0.4.  #1269
AngledLuffa added a commit that referenced this issue Oct 10, 2023
Remove the old javax.servlet jar (4.0.4 apparently has fewer security issues)

Update all servlet uses in .xml files to use jakarta 4.0.4.  #1269
@AngledLuffa
Copy link
Contributor

We're running Tomcat 9, which I read uses servlet api 4. I believe that means upgrading shouldn't cause any problems with someone adding a feature based on the newer Jakarta version of the servlet which doesn't actually work in our Tomcat installation. (Not that that is a likely event anyway.) So, I merged this into dev, and it should be part of the next CoreNLP release, whenever that is.

@eduarddrenth
Copy link

Nice, this may even work with ee 10 jdk 17, can I try that when building from the dev branch?

@AngledLuffa
Copy link
Contributor

AngledLuffa commented Oct 13, 2023 via email

@eduarddrenth
Copy link

eduarddrenth commented Oct 13, 2023 via email

@eduarddrenth
Copy link

eduarddrenth commented Nov 17, 2023

Tested this return new Sentence(form).lemmas(), works nicely in payara 6 ee 10. Built corenlp from dev using pom-java-17.xml.

@eduarddrenth
Copy link

Now I include in pom.xml:

            <dependency>
                <groupId>edu.stanford.nlp</groupId>
                <artifactId>stanford-corenlp</artifactId>
                <version>4.5.5</version>
            </dependency>

But I have to git clone and mvn install -f pom-java-17.xml first.

Will this jakarta update make it to maven central somtime?

Regards, Eduard

@AngledLuffa
Copy link
Contributor

eventually, yes, but probably early january

@eduarddrenth
Copy link

Good to know, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants