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

WebClient - Uri resolved without query params when path and query params are mixed. #8566

Open
ashcor opened this issue Mar 26, 2024 · 4 comments · May be fixed by #8614
Open

WebClient - Uri resolved without query params when path and query params are mixed. #8566

ashcor opened this issue Mar 26, 2024 · 4 comments · May be fixed by #8614
Assignees
Labels
4.x Version 4.x triage
Projects

Comments

@ashcor
Copy link

ashcor commented Mar 26, 2024

I would expect the resolved uri to be the same in all four examples below. Instead, the query params are missing when path and query params are mixed when building requests.

Environment Details

  • Helidon Version: 4.0.6
  • Helidon SE
  • JDK version: 21
  • OS: Mac OSX
  • Docker version (if applicable):

Problem Description

See tests below. The test fails for test1 and test2. Would expect the same result as for the other two tests.

Steps to reproduce

private final String expected = "https://eu.example.com:443/a/b?q=one";

    @Disabled
    @Test
    void test1() {
        var uri = WebClient.create()
                .get()
                .uri("https://eu.example.com/{path}/b")
                .pathParam("path", "a")
                .queryParam("q", "one")
                .resolvedUri();
         // actual: "https://eu.example.com/a/b"
        assertEquals(expected, uri.toString());
    }

    @Disabled
    @Test
    void test2() {
        var uri = WebClient.create()
                .get()
                .uri("https://{region}.example.com/a/b")
                .pathParam("region", "eu")
                .queryParam("q", "one")
                .resolvedUri();
        // actual: "https://eu.example.com/a/b"
        assertEquals(expected, uri.toString());
    }

    @Test
    void test3() {
        var uri = WebClient.create()
                .get()
                .uri("https://eu.example.com/a/b")
                .queryParam("q", "one")
                .resolvedUri();
        assertEquals(expected, uri.toString());
    }

    @Test
    void test4() {
        var uri = WebClient.create()
                .get()
                .uri("https://{region}.example.com/a/b?q={q}")
                .pathParam("region", "eu")
                .pathParam("q", "one")
                .skipUriEncoding(true)
                .resolvedUri();
        assertEquals(expected, uri.toString());
    }
@github-actions github-actions bot added this to Triage in Backlog Mar 26, 2024
@m0mus m0mus added 4.x Version 4.x triage labels Mar 28, 2024
@m0mus m0mus moved this from Triage to Sprint Scope in Backlog Mar 28, 2024
VerKWer pushed a commit to VerKWer/helidon that referenced this issue Apr 4, 2024
The ClientRequestBase class did not properly resolve URIs when path- and
query parameters were mixed. More specifically, the query parameters
were stripped if the URI template was absolute.
@VerKWer VerKWer linked a pull request Apr 4, 2024 that will close this issue
@VerKWer
Copy link
Contributor

VerKWer commented Apr 4, 2024

Oops, I just realised the @Verdent was assigned. I had started looking into this out of curiosity and never refreshed the tab with the issue open before creating a PR. I hope I didn't step on anyone's toes. Otherwise, the PR can be ignored with apologies to @Verdent.

VerKWer pushed a commit to VerKWer/helidon that referenced this issue Apr 4, 2024
The ClientRequestBase class did not properly resolve URIs when path- and
query parameters were mixed. More specifically, the query parameters
were stripped if the URI template was absolute.
@Verdent
Copy link
Member

Verdent commented Apr 4, 2024

@VerKWer This is absolutely fine. If you found a solution, just create PR and assign me as a reviewer :-)

@VerKWer
Copy link
Contributor

VerKWer commented Apr 4, 2024

Glad to hear it :). I'm not sure I have the rights to assign reviewers. The PR is #8614, linked to this issue.

@VerKWer
Copy link
Contributor

VerKWer commented Apr 5, 2024

While looking into this, I noticed that the ClientUri.resolve(URI) method has some unintuitive logic to it (at least for my understanding). It seems to be a mixture of "normal" URI resolution (according to RFC3986, Section 5.2) and something more useful when constructing URIs.

If the provided URI is absolute, the existing query parameters are cleared and replaced by the ones from the provided URI. Otherwise, the query parameters are merged. Clearly, merging the query parameters can be useful when constructing URIs but I'm puzzled by the exception for absolute paths. As far as I can tell from looking at the RFC, whether the path is absolute has no influence on the resolved query parameters.

Maybe @tomas-langer has some insights here, having written the original ClientUri?

@Verdent Verdent moved this from Sprint Scope to In Progress in Backlog Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x triage
Projects
Backlog
  
In Progress
Development

Successfully merging a pull request may close this issue.

4 participants