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

[Backport 2.x] add checks to existing unit tests #1106

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

opensearch-trigger-bot[bot]
Copy link
Contributor

Backport c098d8b from #1104.

One for EndpointTest that checks for the GetRequest.id() being correctly encoded. One for PathEncodingTesst to verify that ":" is being encoded.

Updated for spotless.

Signed-off-by: Al Niessner <Al.Niessner@xxx.xxx>
Co-authored-by: Al Niessner <Al.Niessner@xxx.xxx>
(cherry picked from commit c098d8b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@dblock
Copy link
Member

dblock commented Jul 24, 2024

@al-niessner looks like the backport failed as expected, I hear @Xtansia is looking into it, but don't hold back if you know/have/want to try a fix before him

@al-niessner
Copy link
Contributor

I am really shooting in the dark here but will give a whirl.

@al-niessner
Copy link
Contributor

@dblock @Xtansia @jordanpadams @tloubrieu-jpl

I traced it down to PathEncoder and it is exactly what @Xtansia thought. I reduced the checks to:

    String base = "a:b:c::2.0";
    String expected = "/a%3Ab%3Ac%3A%3A2.0";
    assertEquals(expected, org.apache.http.client.utils.URLEncodedUtils.formatSegments(base));
    assertEquals(expected, org.apache.http.client.utils.URLEncodedUtils.formatSegments(Collections.singletonList(base), StandardCharsets.UTF_8));
    assertEquals(expected, org.apache.hc.core5.net.URLEncodedUtils.formatSegments(Collections.singletonList(base), StandardCharsets.UTF_8));

The simplest thing to say is that org.apache.http.client.utils.URLEncodedUtils.formatSegments() does not treat the ':' as a special character while org.apache.hc.core5.net.URLEncodedUtils.formatSegments() does. Other than altering apache code and waiting for its next release or rolling your own encoder fraught with all of its own problems, there is no solution here.

Do you have a snapshot or future release that is based solely on httpClient5? I know this potentially introduces a host of new problems, but at least they will be consistent to fix.

Actual errorr:

org.junit.ComparisonFailure: expected:</a[%3Ab%3Ac%3A%3A]2.0> but was:</a[:b:c::]2.0>

@al-niessner
Copy link
Contributor

Oh, you know what. We can reverse the checks in PathEncoder so that if it finds httpClient5 first then use it but fail back to 4 with all of its known warts. This would essentially fix our problem because httpClient5 is in our classpath already for localhost issues. Would you be alright with such a change?

@al-niessner
Copy link
Contributor

Now I understand what you mean by 2.x prioritizing apache 4. My suggestion above is in contrast to idea.

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

Successfully merging this pull request may close these issues.

2 participants