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

Issue with URL Encoding of space character in query parameter #125

Open
thomas-br opened this issue Aug 25, 2021 · 13 comments
Open

Issue with URL Encoding of space character in query parameter #125

thomas-br opened this issue Aug 25, 2021 · 13 comments

Comments

@thomas-br
Copy link

In pact artifact files generated out of pact-consumer-swift tests I am facing issues with URL encoding.
With the example test:

mockServiceInstance
    .uponReceiving("example request")
    .given("example")
    .withRequest(
        method: .GET,
        path: "/example/odata",
        query: ["$orderby": "CreatedAt desc"]
    )
    .willRespondWith(status: 200)

In the pact file under interaction, the URL is constructed like the following:

"request": {
        "method": "get",
        "path": "/example/odata",
        "query": "$orderby=CreatedAt+desc"
      },

The space in query is unfortunately encoded as a + instead of a %20 which is leading to issues with certain providers running the verification against their systems. Some libraries seem to be very picky of the expected escaped space whether to expect %20 or +. This is currently blocking the implementation of pact tests on provider side.

Is there a way of configuring pact-consumer-swift to escape with a %20 or somehow influence how the pact json is generated?

@andrewspinks
Copy link
Collaborator

andrewspinks commented Aug 27, 2021

Hmm, the + is weird, we are guessing that is coming from the mock server. However, if the provider is expecting a url encoded string, I feel like that should be part of the contract (and what your client will need to do).

i.e. shouldn't the mock be

mockServiceInstance
    .uponReceiving("example request")
    .given("example")
    .withRequest(
        method: .GET,
        path: "/example/odata",
        query: ["$orderby": "CreatedAt%20desc"]
    )
    .willRespondWith(status: 200)

@bethesque
Copy link

That may cause the ampersand to be double encoded, as the query with the hash format expects unencoded params. I don't think this can be fixed unfortunately, as it's part of the underlying behaviour of the Ruby encoding library. I'm afraid you'll have to post process the pact file (find and replace) before it's published.

@TimothyJones
Copy link

TimothyJones commented Aug 30, 2021

Oh hey, I know this one!

The + comes from the W3C recommendation that forms submitted with GET put the values in the query part of the url, encoded as application/x-www-form-urlencoded (which never appears in any headers, because it's the content-type of the query string). This is also where we get the ?x=1&y=2 convention.

RFC 3986 defines the URL syntax, which is mostly about the separate parts of a URL, what they're for, and what characters can go there. It doesn't define any structure to the query string, but it does say that + is valid in a query string, (it is reserved as a sub-delimiter). Spaces are not valid in URLs. If you want to encode characters that are not valid, you must percent encode them - so literal spaces and literal plusses need to be encoded.

As a server, you don't know what content type you have in a query string. But you do know:

  • If you see + in a query string, it is not a literal plus, it is a delimiter within a field.
  • If you see %20 in a query string, it is an encoding for a literal space
  • Many implementations (such as HTML forms submitted with GET) submit query strings in application/x-www-form-urlencoded, where + is a space.
    • You can't tell if your request came from one of those implementations.
    • So, unless the server knows otherwise, it should assume that + is a space.

tl;dr: both + and %20 are spaces if they're in the query string.

Of course, it's ideal if the pact can perfectly match what is actually sent by your client. Does pact-consumer-swift let you specify the raw query string?

@TimothyJones
Copy link

Only after spending 30 minutes writing that up do I see that Beth already linked to a summary of + vs %20 on the PR 🤦

@bethesque
Copy link

But you summarised it so nicely! They should link to YOUR explanation.

@bethesque
Copy link

Unfortunately, what servers should do and what they do do are two different things.

@surpher
Copy link
Contributor

surpher commented Sep 2, 2021

Beth is right about double percentage encoding if writing the test with %20:

// declaring
query: ["$orderby": "CreatedAt%20desc"]

// yeilds
query: "$orderby=CreatedAt%2520desc"

A way to get pact-consumer-swift to allow users define whether to encode or not encode path or query params would probably introduce a breaking change where a Type with an encode flag would be passed instead of Any? (which already smells... fragile). There might be more issues making changes for this as pact-consumer-swift uses an internal API client that proxies the requests to pact-ruby-standalone.

Changes in #126 explicitly convert literal spaces into %20 but again, that might not be what servers actually do do or expect. Even though I am the author of that PR I am reluctant to see it merged. Would expect pact-ruby-standalone to percentage encode paths and queries instead of injecting +, but just like Beth wrote, different providers behave differently to the same request so if it would've worked for your case, might not work for a lot of other users of pact-consumer-swift.

@bethesque
Copy link

In this version of the pact specification, you can actually specify the query string as a string, and it's expected that it is encoded when the user specifies the string, so that's probably the best solution in this case.

I believe this is not supported in later versions of the specification though, which is something I kept meaning to bring up, but it never got to the top of the priority list.

@surpher
Copy link
Contributor

surpher commented Sep 6, 2021

Based on what @bethesque wrote, @thomas-br does the test/contract work per your expectations if you write your test like:

.withRequest(
    method: .GET,
    path: "/example/odata",
    query: "$orderby=CreatedAt%20desc"
)
.willRespondWith(status: 200)

@thomas-br
Copy link
Author

@surpher Yes it does. The only downside of the way shown in the snipped is that it introduces a strict checking of the order of the query params if there are multiple ones.

@surpher
Copy link
Contributor

surpher commented Sep 7, 2021

Yeah I understand and was wondering about that. @bethesque when passing a string for query the pact-ruby-standalone is strict about the params' order in v2, right?

@bethesque
Copy link

I thought I did some magic logic in there to do a smarter match, but if @thomas-br has tested it any found that the order makes a difference, then I must be thinking of something else.

@TimothyJones
Copy link

Part of the problem is that it's not easy to know what format the query string is in. By convention (but not standard), they're x-www-form-encoded, but something that autodetected query params and reasoned about them could fail in this case, because spaces in x-www-form-encoded are + and not %20.

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 a pull request may close this issue.

5 participants