SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 #2501

iamsanjay · 2024-06-06T03:42:47Z

https://issues.apache.org/jira/browse/SOLR-17321

As we started preparing (#2497 ) the move to change the Java version for the main branch, It has been decided to merge the deprecated API changes first, and thus reducing the change burden for #2497.

Here we are replacing the deprecated URL with new Syntax using URI.

//Deprecated
URL url = new URL(urlStr);

to

URL url = URI.create(urlStr).toURL();

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

iamsanjay · 2024-06-06T03:46:20Z

Though new Locale is deprecated, however the newer syntax Locale.of is not available in Java 11. Therefore It can only be done after migrating to Java21 or along with it.

Should we use forbidden APIs to Suppress the API which is forbidden in Java 21? Thread#getID and some other stuff related to SecurityManager.

epugh

This does make sense as the right route! Lgtm!!!

epugh · 2024-06-06T10:18:52Z

Seems a shame to add a to. Of suppress for locale and then remove it... might be cleaner to do that one as part of the upgrade.

epugh · 2024-06-06T10:20:02Z

As far as security manager goes, don't we need to just tackle that and solve the issue? Versus suppress it and ignore the problem?

iamsanjay · 2024-06-06T12:43:39Z

As far as security manager goes, don't we need to just tackle that and solve the issue? Versus suppress it and ignore the problem?

Removing a SecurityManager will not be a simple task as some of these settings, I believe, were put in place to fix Security issues. For now, using forbidden would clear the path to the upgradation, Lucene did the same thing. However, now that the JDK has marked it for removal, future JDK upgradation would only be possible after removing it So we should start looking into it right after upgrading to Java 21.

And that's why I suggested marking SecurityManager, and related stuff, forbidden which is a viable solution for now.

epugh · 2024-06-06T12:47:04Z

As far as security manager goes, don't we need to just tackle that and solve the issue? Versus suppress it and ignore the problem?

Removing a SecurityManager will not be a simple task as some of these settings, I believe, were put in place to fix Security issues. For now, using forbidden would clear the path to the upgradation, Lucene did the same thing. However, now that the JDK has marked it for removal, future JDK upgradation would only be possible after removing it So we should start looking into it right after upgrading to Java 21.

And that's why I suggested marking SecurityManager, and related stuff, forbidden which is a viable solution for now.

yeah.. It sort of feels like that is the opposite of "get the code ready for java 21 first" like you are doing here with Url. However I also understand that it may spiral into a bigger thing. The important thing is to keep moving forward! Thanks for this.

iamsanjay · 2024-06-06T12:54:53Z

Actually I started looking into it, the first issue that I found was "XXE attack in SolrConfig.xml" and a proposed fix (https://issues.apache.org/jira/browse/SOLR-12316). Afterwards the more improved version of fix involved SecurityManager as in running Solr in sandbox via SecurityManager (https://issues.apache.org/jira/browse/SOLR-13984).

And this was the comment!

The correct way to fix all issues we have seen the last time is very simple: LET'S RUN SOLR INSIDE A SECURITY MANAGER IN PRODUCTION (like in tests). Elasticsearch is doing this, so please please let's do this instead. But this requires to finally get rid of the webapplication and start.jar and add our own bootstrapping (like in tests) that configure Jetty and Security Manager from our own org.apache.solr.bootstrap.Main.java (or similar). -- Uwe

But I will keep on investigating, How and where Solr using SecurityManager?

dsmiley · 2024-06-06T16:50:02Z

Can we please keep the commentary here focused on what this PR title says this is about -- URL API?

dsmiley

Nice focused PR; thanks.

So did you find IntelliJ "structural replace" helpful? It's been quite a while since I last used it. It was clunkier than I wished... I wish it could detect when I'm trying to look for in a smarter way without me having to know the feature well. Ideally there would be a resource of popular/useful patterns, as one might expect there to be for JDK and major framework upgrades. Alas.

epugh · 2024-06-08T11:21:36Z

I like the forbidden api addition!

iamsanjay · 2024-06-10T04:09:29Z

All the usages of new URL (except one) has been replaced with URI.create(url).toURL(). The exception was one specific case which is testing the presence of caret in the URL as It was an old issues (https://issues.apache.org/jira/browse/SOLR-5971) that failed due to the caret. Now replacing it with new Syntax fails because caret is in the disallowed list for URI and thus failed. I do not want to encode the character, because the whole point of test is to test caret without encoding it, instead I used @SuppressForbidden.

Because #2497 is still in progress, So, meanwhile, I added "new URL" to the forbidden api to make sure no one would use it.

solr/CHANGES.txt

iamsanjay · 2024-06-11T14:18:26Z

I added the changes under 9.7, I did not discussed this. So Should we consider this change for 10 only or release it in 9.7 as well?

HoustonPutman · 2024-06-11T14:44:12Z

Backport it to 9.7 IMO

epugh · 2024-06-11T16:11:16Z

Agreed. When I added basic auth to the solr cli I did NOT backport it, and now it's a constant source of pain.

iamsanjay · 2024-06-13T03:32:30Z

Going to merge this and back port to 9x on Friday, around 7PM IST.

dsmiley

Why use ForbiddenAPI for a deprecated thing? The JDK (with IDE support too) and javac already handle it in its own way. ForbiddenAPI principally exists for non-deprecated things we insist on not using.
cc @uschindler

iamsanjay · 2024-06-13T07:07:13Z

Why use ForbiddenAPI for a deprecated thing?

As we are still in migration phase, It will not be deprecated until we move onto Java 21. Meanwhile, If someone tries to use it , they will get the warning message with an alternative API recommendation.

uschindler · 2024-06-13T07:18:59Z

Why use ForbiddenAPI for a deprecated thing? The JDK (with IDE support too) and javac already handle it in its own way. ForbiddenAPI principally exists for non-deprecated things we insist on not using. cc @uschindler

In Java 11 it is not yet deprecated. So this allows to forbid it now.

Actually once we are on Java 21, the forbiddenapis signatures will complain, too (as deprecated APIs are reported, too). If you ask why forbiddenapis reports deprecated stuff: It's because normally during compilation, JDK only reports a warning for all deprecations (not only in the JDK also in your own code or dependencies). Forbiddenapis allows to restrict only JDK-forbidden stuff and you can keep other deprecations supressed. This is especially important for API developers, because you deprecate your own stuff but still have tests for it.

uschindler

Actually this is partly a bit strange. In general the move should be to go away from URL completely. Theres code changed here which at some point creates an URL but later passes it to Jetty as URI. So It would be better to replace all URL by URI from beginning.

There are also some roundtrip conversions, please remove them.

solr/solrj/src/test/org/apache/solr/common/util/ContentStreamTest.java

solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java

dsmiley · 2024-06-13T08:55:08Z

What I love about our community is that we have so much expertise to call upon; thanks Uwe :-)

Forbiddenapis allows to restrict only JDK-forbidden stuff

Then lets do that and not single out a specific thing (URL).

uschindler

Small change regarding exception handling in the POST tool, otherwise looks fine to me.

uschindler · 2024-06-16T17:18:46Z

solr/core/src/java/org/apache/solr/cli/PostTool.java

@@ -389,7 +389,7 @@ private void doWebMode() {
      numPagesPosted = postWebPages(args, 0, out);
      info(numPagesPosted + " web pages indexed.");

-    } catch (MalformedURLException e) {
+    } catch (URISyntaxException e) {


URISyntaxException is only thrown by new URI(), the create function does not. Not sure how we should handle this.

See documentation of URI#create(String): "This convenience factory method works as if by invoking the URI(String) constructor; any URISyntaxException thrown by the constructor is caught and wrapped in a new IllegalArgumentException object, which is then thrown."

I would make this consistent to other code and prefer a clear exception.

I started using URI.create for the same reason that you just explained as it throws unchecked exception which allows us to avoid explicitly handling the URISyntaxException.

Should we switch to new URI wherever we can without changing too much? I am open to that.

I think both is fine. I just noticed that the "catch" here looks obsolete (I wonder why compiler did not complain).

doWebMode() is calling appendUrlPath that, as per new changes, is throwing URISyntaxException.

protected static URI appendUrlPath(URI uri, String append) throws URISyntaxException { var newPath = uri.getPath() + append; return new URI(uri.getScheme(), uri.getAuthority(), newPath, uri.getQuery(), uri.getFragment()); }

I have replaced the URI.create with new URI in PostTool.java. Also replaced in few test classes as well.

Looks great!

I have replaced the URI.create with new URI in PostTool.java. Also replaced in few test classes as well.

Actually you did this everywhere, which I am not sure if its a good idea. In PostTool i recommend to change this to get a consistent error message in both cases: for original URI and for the appendUrlPath() method.

I am not a big fan of checked Exceptions, so I would have used new URI only to make it consistent with the code around it.

I've reverted the changes and updated it only for PostTool.

git diff f2841ac..0007bb082 --name-only solr/CHANGES.txt solr/core/src/java/org/apache/solr/cli/PostTool.java

iamsanjay · 2024-06-17T16:23:27Z

Ran the failed test multiple times and it's passing. Flaky!

ERROR: The following test(s) have failed:
  - org.apache.solr.client.solrj.TestLBHttp2SolrClient.testTwoServers (:solr:solrj)
    Test history: https://ge.apache.org/scans/tests?search.rootProjectNames=solr-root&tests.container=org.apache.solr.client.solrj.TestLBHttp2SolrClient&tests.test=testTwoServers http://fucit.org/solr-jenkins-reports/history-trend-of-recent-failures.html#series/org.apache.solr.client.solrj.TestLBHttp2SolrClient.testTwoServers
    Test output: /tmp/src/solr/solr/solrj/build/test-results/test/outputs/OUTPUT-org.apache.solr.client.solrj.TestLBHttp2SolrClient.txt
    Reproduce with: ./gradlew :solr:solrj:test --tests "org.apache.solr.client.solrj.TestLBHttp2SolrClient.testTwoServers" -Ptests.jvms=96 "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=BC6F7BFC1228CAE8 -Ptests.timeoutSuite=600000! -Ptests.file.encoding=ISO-8859-1
FAILURE: Build failed with an exception.

It recently started failing (I hope I am looking at it in a right way!)

PS :- Thank You @dsmiley. Test history URL in Action!

iamsanjay · 2024-06-20T03:32:09Z

If there are no more comments/issues, I would like to merge this by Friday. Thanks!

…pache#2501) * Switch to URI * Added URL to the forbidden API ------------- Co-authored-by: Uwe Schindler <[email protected]> Co-authored-by: David Smiley <[email protected]> (cherry picked from commit 2a56bfc)

HoustonPutman · 2024-06-26T17:43:07Z

solr/test-framework/src/java/org/apache/solr/handler/BackupRestoreUtils.java

@@ -109,7 +110,7 @@ public static void runReplicationHandlerCommand(
  }

  static void executeHttpRequest(String requestUrl) throws IOException {
-    URL url = new URL(requestUrl);
+    URL url = URI.create(requestUrl).toURL();


This is causing TestSolrCoreSnapshots.testBackupRestore to fail on Windows machines.

java.lang.IllegalArgumentException: Illegal character in query at index 117: http://127.0.0.1:64027/solr/admin/cores?action=BACKUPCORE&core=SolrCoreSnapshots_shard1_replica_n1&name=u&location=C:\Users\jenkins\workspace\Solr-main-Windows\solr\core\build\tmp\tests-tmp\org.apache.solr.core.snapshots.TestSolrCoreSnapshots_9D4706448466E215-001\tempDir-002&incremental=false&commitName=ydfbp at __randomizedtesting.SeedInfo.seed([9D4706448466E215:2BFC06FAB6D52C5B]:0) at java.base/java.net.URI.create(URI.java:883) at org.apache.solr.handler.BackupRestoreUtils.executeHttpRequest(BackupRestoreUtils.java:113)

…r Java 21 (#2528) * SOLR-17321: Remove Deprecated URL ctors in Preparation for Java 21 (#2501) * Switch to URI * Added URL to the forbidden API ------------- Co-authored-by: Uwe Schindler <[email protected]> Co-authored-by: David Smiley <[email protected]> (cherry picked from commit 2a56bfc)

iamsanjay added 2 commits June 6, 2024 07:11

Remove deprecated URL API for Java 21 preparation

5816530

Merge branch 'main' into PrepSolr_For_Java21

e0eb40d

github-actions bot added test-framework jetty-server client:solrj module:jwt-auth tests cat:search cat:cloud cat:index cat:cli cat:schema cat:security cat:packagemanager labels Jun 6, 2024

epugh approved these changes Jun 6, 2024

View reviewed changes

dsmiley approved these changes Jun 6, 2024

View reviewed changes

iamsanjay added 2 commits June 8, 2024 11:33

Merge branch 'main' into PrepSolr_For_Java21

bbec5d4

Replace few more usages and added URL to forbiddenAPI

d85c171

github-actions bot added the tool:build label Jun 8, 2024

dsmiley reviewed Jun 10, 2024

View reviewed changes

solr/CHANGES.txt Outdated Show resolved Hide resolved

Merge main and put change txt under Other

1b67d2b

dsmiley reviewed Jun 13, 2024

View reviewed changes

uschindler requested changes Jun 13, 2024

View reviewed changes

solr/solrj/src/test/org/apache/solr/common/util/ContentStreamTest.java Outdated Show resolved Hide resolved

solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java Show resolved Hide resolved

iamsanjay added 4 commits June 13, 2024 18:28

Merge branch 'main' into PrepSolr_For_Java21

95ca5eb

removed unnecessary conversions of URI to URL

3d35804

Merge main

8aa9bfd

tidy

0007bb0

iamsanjay requested a review from uschindler June 16, 2024 16:19

uschindler approved these changes Jun 16, 2024

View reviewed changes

iamsanjay added 3 commits June 17, 2024 18:54

Replaced URI.create with new URI for improved exception clarity

01d0842

revert usage of new URI

4da65e8

revert for PostToolTest as well

f2841ac

iamsanjay requested a review from uschindler June 18, 2024 12:13

uschindler approved these changes Jun 20, 2024

View reviewed changes

epugh approved these changes Jun 20, 2024

View reviewed changes

iamsanjay merged commit 2a56bfc into apache:main Jun 21, 2024
4 checks passed

iamsanjay mentioned this pull request Jun 21, 2024

[branch_9x] SOLR-17321: Remove Deprecated URL ctors in Preparation for Java 21 (… #2528

Merged

HoustonPutman reviewed Jun 26, 2024

View reviewed changes

iamsanjay mentioned this pull request Jun 27, 2024

SOLR-17321: Fix TestSolrCoreSnapshots.testBackupRestore #2539

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 #2501

SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 #2501

iamsanjay commented Jun 6, 2024 •

edited by malliaridis

Loading

iamsanjay commented Jun 6, 2024

epugh left a comment

epugh commented Jun 6, 2024

epugh commented Jun 6, 2024

iamsanjay commented Jun 6, 2024

epugh commented Jun 6, 2024

iamsanjay commented Jun 6, 2024

dsmiley commented Jun 6, 2024

dsmiley left a comment

epugh commented Jun 8, 2024

iamsanjay commented Jun 10, 2024

iamsanjay commented Jun 11, 2024

HoustonPutman commented Jun 11, 2024

epugh commented Jun 11, 2024

iamsanjay commented Jun 13, 2024

dsmiley left a comment

iamsanjay commented Jun 13, 2024

uschindler commented Jun 13, 2024

uschindler left a comment

dsmiley commented Jun 13, 2024

uschindler left a comment

uschindler Jun 16, 2024

uschindler Jun 16, 2024

iamsanjay Jun 17, 2024

uschindler Jun 17, 2024

iamsanjay Jun 17, 2024 •

edited

Loading

iamsanjay Jun 17, 2024

epugh Jun 17, 2024

uschindler Jun 17, 2024

iamsanjay Jun 17, 2024 •

edited

Loading

iamsanjay Jun 17, 2024

iamsanjay commented Jun 17, 2024

iamsanjay commented Jun 20, 2024

HoustonPutman Jun 26, 2024

SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 #2501

SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 #2501

Conversation

iamsanjay commented Jun 6, 2024 • edited by malliaridis Loading

Checklist

iamsanjay commented Jun 6, 2024

epugh left a comment

Choose a reason for hiding this comment

epugh commented Jun 6, 2024

epugh commented Jun 6, 2024

iamsanjay commented Jun 6, 2024

epugh commented Jun 6, 2024

iamsanjay commented Jun 6, 2024

dsmiley commented Jun 6, 2024

dsmiley left a comment

Choose a reason for hiding this comment

epugh commented Jun 8, 2024

iamsanjay commented Jun 10, 2024

iamsanjay commented Jun 11, 2024

HoustonPutman commented Jun 11, 2024

epugh commented Jun 11, 2024

iamsanjay commented Jun 13, 2024

dsmiley left a comment

Choose a reason for hiding this comment

iamsanjay commented Jun 13, 2024

uschindler commented Jun 13, 2024

uschindler left a comment

Choose a reason for hiding this comment

dsmiley commented Jun 13, 2024

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamsanjay Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamsanjay Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamsanjay commented Jun 17, 2024

iamsanjay commented Jun 20, 2024

Choose a reason for hiding this comment

iamsanjay commented Jun 6, 2024 •

edited by malliaridis

Loading

iamsanjay Jun 17, 2024 •

edited

Loading

iamsanjay Jun 17, 2024 •

edited

Loading