Hash order was changed in the last release #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is it really a good idea to spend extra cycles for sorting stuff that is valid in any order? @samcv what do you think? Or is it good because it does not leak out the seed this way? Maybe it would be better if the test understood that the result is correct even if the order is different? |
|
I don't know. It's probably better if the result of this kind of thing is
deterministic, since it's a string. It would not make much sense to return
a different string every time. You will have to live with that if it's
stringified; if you don't want to spend the cycles, stringify it
yourself...
|
|
I think we should sort it before stringifying. |
|
Header order should be preserved. For instance: bot detection heuristics often use header order to check if header FOO is used before or after header BAR. |
|
OK. |
|
to clarify: I am against this PR. only the tests themselves should be modified. |
ugexe
reviewed
May 31, 2018
| @@ -50,7 +50,7 @@ lives-ok { $r1.set-method: 'PUT' }, "set method"; | |||
| is $r1.method, 'PUT', 'set-method 1/1'; | |||
|
|
|||
| # parse | |||
| my $req = "GET /index HTTP/1.1\r\nHost: somesite\r\nAccept: test\r\n\r\nname=value&a=b\r\n"; | |||
| my $req = "GET /index HTTP/1.1\r\nAccept: test\r\nHost: somesite\r\n\r\nname=value&a=b\r\n"; # Remember to use alphabetical order | |||
There was a problem hiding this comment.
This should require the CREATION order, not alphabetical order.
|
OK, reverted the merge for now. |
|
Uh, anyone looking at this? |
zoffixznet
added a commit
to zoffixznet/http-useragent
that referenced
this pull request
Jun 18, 2018
Fixes sergot#197 Fixes sergot#199 Kludges sergot#201 Fixes newly failing tests due to hash ordering now being randomized in Perl 6, to prevent potential DoS attacks. The module's design uses hashes to pass data for which ordering is desired, but not required. So the proper fix would be a larg-ish redesign that preserves the ordering of this data. The fix simply sorts the data by keys, destroying original order, but preserving *an* order, so that, for example, tests receive predictable output. Two pieces are affected: 1) Order of HTTP headers[^1]: The RFC says[^2] order doesn't matter, but there's some user demand[^3] for preserving the ordering for third party systems. 2) Order of Multi-Part form data: the HTML spec says[^4] the order of the parts represents the order the elements appear in the HTML. Looking at the .add-form-data method, I see they're passed via a hash, so order information is not available to the module from the very start and a differnt interface would be needed to make it available. [1] sergot#201 [2] https://tools.ietf.org/html/rfc2616#section-4.2 [3] sergot#200 (comment) [4] https://www.w3.org/TR/html4/interact/forms.html#h-17.13.4
ugexe
pushed a commit
that referenced
this pull request
Jun 18, 2018
Fixes #197 Fixes #199 Kludges #201 Fixes newly failing tests due to hash ordering now being randomized in Perl 6, to prevent potential DoS attacks. The module's design uses hashes to pass data for which ordering is desired, but not required. So the proper fix would be a larg-ish redesign that preserves the ordering of this data. The fix simply sorts the data by keys, destroying original order, but preserving *an* order, so that, for example, tests receive predictable output. Two pieces are affected: 1) Order of HTTP headers[^1]: The RFC says[^2] order doesn't matter, but there's some user demand[^3] for preserving the ordering for third party systems. 2) Order of Multi-Part form data: the HTML spec says[^4] the order of the parts represents the order the elements appear in the HTML. Looking at the .add-form-data method, I see they're passed via a hash, so order information is not available to the module from the very start and a differnt interface would be needed to make it available. [1] #201 [2] https://tools.ietf.org/html/rfc2616#section-4.2 [3] #200 (comment) [4] https://www.w3.org/TR/html4/interact/forms.html#h-17.13.4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now it's guaranteed to always be different. You have to return always sorted keys. Hope this fixes the problem with 2018.05