potPourriB.dhallb: Fix encoding of query string

[sjakobi]

Original URL (potPourriA.dhall):

https://-._~%2C!$&'*+;=:@0abc1--12a------a-a--a-0/foo?/-._~%2C!$&'*+;=:@/?

JSON representation of the original binary:

[
    24,
    null,
    0,
    1,
    null,
    "-._~%2C!$&'*+;=:@0abc1--12a------a-a--a-0",
    "foo",
    "/-._~%2C!$&%27*+;=:@/?"
]

JSON representation of the new binary:

[
    24,
    null,
    0,
    1,
    null,
    "-._~%2C!$&'*+;=:@0abc1--12a------a-a--a-0",
    "foo",
    "/-._~%2C!$&'*+;=:@/?"
]

Diff:

9c9
<     "/-._~%2C!$&%27*+;=:@/?"
---
>     "/-._~%2C!$&'*+;=:@/?"

The query string was incorrectly percent-encoded.

[singpolyma]

+ should be percent-encoded because some servers interpret it as a space‎, following the CGI spec

[sjakobi]

+ should be percent-encoded because some servers interpret it as a space‎, following the CGI spec

That's an unrelated potential standard change, right?

Currently + is allowed as a sub-delim:

dhall-lang/standard/dhall.abnf

Lines 587 to 597 in 5ca75a9

	pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
	query = *( pchar / "/" / "?" )
	pct-encoded = "%" HEXDIG HEXDIG
	unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
	; this is the RFC3986 sub-delims rule, without "(", ")" or ","
	; see comments above the `http-raw` rule above
	sub-delims = "!" / "$" / "&" / "'" / "*" / "+" / ";" / "="

[philandstuff]

Query strings should be treated as opaque strings, and not decoded or encoded. We have tests/parser/success/unit/import/urls/escapedQueryA.dhall to verify this behaviour.

It makes sense for us to decode and encode path segments, because we convert local paths to URL paths, which is an operation which involves manipulating the contents of a path. But we don't ever do any manipulation of the contents of a query string, so we should just pass whatever the user gave us around in unmodified form. Introducing percent-decoding or -encoding here just feels unnecessary and a source of subtle bugs; especially since (as you point out) the rules for percent-encoding and -decoding query strings are not the same as for paths.

[sjakobi]

Note that, before #627, the ' in the query string was not percent-encoded. So I really think that this patch is a fix for a small regression – I really don't want to argue for a standard change here.

[Gabriella439]

@sjakobi: My understanding is that what is currently in master is what follows the standard since the standard does not specify to percent-encode query fragments. So without a change to the standard then I believe we should not change the test result.

Also, in general the current standardization trend is towards preserving the original URL as much as possible (i.e. not percent-encoding the query fragment). See #581 for the roadmap of URL-related changes

[sjakobi]

My understanding is that what is currently in master is what follows the standard since the standard does not specify to percent-encode query fragments.

No. As I tried to point out in the PR description, potPourriB.dhallb in master incorrectly uses percent encoding in the query string. The incorrect percent encoding was introduced in #627.

This PR will fix this.

[Gabriella439]

@sjakobi: Oh, I misread. Yeah, then this looks good to me