[branch_9x] SOLR-17321: Remove Deprecated URL ctors in Preparation for Java 21 (…#2528
Conversation
…pache#2501) * Switch to URI * Added URL to the forbidden API ------------- Co-authored-by: Uwe Schindler <uschindler@apache.org> Co-authored-by: David Smiley <dsmiley@apache.org> (cherry picked from commit 2a56bfc)
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
LGTM. I assume you saw the one precommit burp.
|
I’d like to resolve this issue, but before that, we need to address issue #2539. A test case failed on a Windows machine because of the backslash character. URI is stricter compared to URL, which raises some concerns about this code change. Could there be more failures like this if we merge this code into 9_x? I am going to point out all the main core classes (not the test cases) where we swapped URL with URI. I’d really appreciate any feedback on whether you think this change might cause issues, especially with passing a string that could contain special characters. |
|
Mainly, we are looking for any special character that could fail the URI parsing. So far there is one example from the past where passing caret(^) symbol in the query params failed it, and now recently with passing backslash() in the path query params. |
I don't think I quite understand the thrust of your comment/question. We should be properly handling all URI to obey the stricket definition, and if we aren't, we should figure out why and fix it right? I think the changes you've made here are right, and our testing should pick if there are other places that URI being stricter than URL causes an issue then we fix them as we go? At least as far as I know there aren't any special nuances that would cause problems, but again, I only know parts of Solr's code base! I suppose if we had the ability to "fuzz" our inputs we could proactively test out all the variations of the old URL inputs to test them, but that feels like a major undertaking! |
|
cherry-picked the latest fix (#2539) and moved the change description under 9.8. Should we consider including this in 9.7 if it's still possible? |
|
I lean towards letting this happen in 9.8... Since what we really care is that this gets us to Java 21 which will only be in |
|
I will leave this open for another few days, just to see if we get any other +1 on the code change, and then look to merge. |
|
Can we merge this one? |
|
Tests all pass, and no issues raised, so I htink you can merge! |
2 participants
#2501 backport blocked due to SOLR-16911. For conflicted files, I used the older code only rather than moving bits from #1810