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

Implement Jakarta Servlet 5 for Jetty 11 #106

Open
wants to merge 8 commits into
base: openrefine
Choose a base branch
from

Conversation

tfmorris
Copy link
Member

Fixes #105

  • Update to Servlet 5 with the Jakarta names replacing javax names - Breaking API change
  • switch from org.apache.commons.collections.ExtendedProperties to org.apache.commons.configuration2.PropertiesConfiguration - Breaking API change
  • switch to Apache commons-lang3
  • update to Velocity 2.3 from 1.6.3

The type org.apache.commons.collections.ExtendedProperties appears
in the interface and has been replaced by the type
org.apache.commons.configuration2.PropertiesConfiguration

- Apache commons-collections 3.2.2 -> commons-collections4 4.4
    and commons-configuration2 2.9.0
- Apache commons-lang 2.6 -> commons-lang3 3.9
- replace deprecated Locale constructor with Locale.Builder
- avoid using URLs when URIs will do to avoid potentially slow DNS lookups
- make better use of type checking
- make better use of try-with-resources
- narrow exceptions caught
- simplify loops
- Major bump of Butterfly version to 2.0.0.
  to reflect new package names
@tfmorris
Copy link
Member Author

Although Jetty 11 requires Servlet 5 and the jakarta package names, Jetty 12 restores compatibility with the old package names, so we don't actually have to change. I reverted those changes (04a82db) and was able to successfully test with Jetty 12. I also added a commited to fix a bug in the Velocity getResourceReader() which was ignoring the requested encoding. We always use UTF-8, so this probably was benign.

Since the Velocity 1.6 to 2.3 update has all the compatibility features turned on, but still could potentially cause template processing differences.

The vulnerable dependency updates introduced a couple of (minor) breaking API changes.

Because of this I'm tempted to say that we should take all the pain at once and update to the new Javakart namespace and the Servlet 5.0 API.

What do others think? @wetneb ? (I know you already approved, but that was before the release of Jetty 12 and before I realized that it doesn't mandate Servlet 5).

@wetneb
Copy link
Member

wetneb commented Apr 9, 2024

Sorry that I forgot to reply here… I don't have strong feelings about this.

From what I understand, the breaking changes that this PR currently introduces are really minimal, meaning that if we release Butterfly and OpenRefine with those changes, most extensions should just work (unless they rely on ExtendedProperties, which is unlikely). I think the Servlet 5 API update will have a bigger impact, at least on any extension implementing Commands (which there are a lot of).

So perhaps it'd be nice to release a new version of Butterfly with those changes already, as we'd be able to ship this in OpenRefine 3.8 / 3.9 without breaking anything.

But also happy with upgrading it all at once if you prefer.

@wetneb
Copy link
Member

wetneb commented Apr 9, 2024

Pinging @AtesComp as maintainer of RDF-transform.

@AtesComp
Copy link

AtesComp commented Apr 9, 2024

Thanks, I'll test and update RDF Transform as needed.

@tfmorris
Copy link
Member Author

@AtesComp my interpretation of @wetneb 's "ping" (correct me if I'm wrong), was not necessarily a request for coding work, but rather for feedback on which direction we should go vis a vis Servlet 5 API and breaking Jakarta package naming changes versus keeping things (mostly, perhaps/hopefully 100%) compatible for existing extensions.

Any feedback on that question from your point of view?

@AtesComp
Copy link

Ah, well I'm all in for using the latest and greatest. If it breaks, it looks like a minor update. Guidance documentation should be made available for any breaking change.

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

Successfully merging this pull request may close these issues.

Update to Jetty 12
3 participants