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

fix: make query param order deterministic when adding params #239

Merged
merged 1 commit into from Mar 20, 2018

Conversation

bpossolo
Copy link
Contributor

Pull Request Checklist

  • Have you read through the contributor guidelines?
  • Have you signed the Typesafe CLA?
  • Have you squashed your commits?
  • Have you added copyright headers to new files?
  • Have you checked that both Scala and Java APIs are updated?
  • Have you updated the documentation for both Scala and Java sections?
  • Have you added tests for any changed functionality?

Fixes

Fixes #48

Purpose

Ensure deterministic query param order when adding params through the WSRequest interface.
This does not address order when adding to a query param that already contains a value.
for example

req.addQueryParam("p1", "v1")
req.addQueryParam("p2", "v2")
req.addQueryParam("p1, "v3")

will yield p1=v1&p1=v3&p2=v2 which is still not perfect but is at least deterministic

Background Context

I wanted to make the minimal code change to achieve deterministic behaviour.
When generating digital signatures of urls (for example of an Authorization header) it's important to have a deterministic order to be able to correctly compute the digest.

References

playframework/playframework#6884
#102

@schmitch
Copy link
Contributor

schmitch commented Mar 1, 2018

I think it would be good, to also have the example that you provided inside a test case, i.e. add p1 twice.

@bpossolo
Copy link
Contributor Author

bpossolo commented Mar 1, 2018

haha. that's funny. i originally wrote a test for it but then deleted it because i thought it might not be desired behaviour for the future. i'll re-add

@bpossolo
Copy link
Contributor Author

bpossolo commented Mar 1, 2018

updated

@bpossolo
Copy link
Contributor Author

bpossolo commented Mar 7, 2018

bump?

Copy link
Contributor

@schmitch schmitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newly added test should be split into 3 test cases

@@ -333,6 +333,32 @@ class AhcWSRequestSpec extends Specification with Mockito with DefaultBodyReadab
request.getUrl must contain("p1=v1")
}

"preserve query param order" in {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically this should be split into multipe tests.

@bpossolo
Copy link
Contributor Author

bpossolo commented Mar 8, 2018

@schmitch thanks for clarifying. updated.

@schmitch
Copy link
Contributor

schmitch commented Mar 8, 2018

/cc @marcospereira guess this can be merged

@bpossolo
Copy link
Contributor Author

any chance we could get this in? its blocking a project

@ktoso
Copy link
Member

ktoso commented Mar 20, 2018

WDYT @marcospereira ?

Copy link
Member

@richdougherty richdougherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - as you say a better behaviour would be to avoid any reordering even if there are params with the same name, but I'm happy that this is an improvement because it is deterministic.

@richdougherty richdougherty merged commit c521ad1 into playframework:master Mar 20, 2018
@bpossolo
Copy link
Contributor Author

@richdougherty thank you!

@richdougherty
Copy link
Member

Backported to 1.x.x branch.

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 this pull request may close these issues.

None yet

6 participants