querystring made more consistent with Fastly

[akrainiouk]

• falco implementation of querystring does not urlencode query parameter names which can produce invalid urls
• Also falco implementation encodes space character with '+' which is inconsistent with Fastly. Fastly use %20 for space encoding.

[richardmarshall]

Did a bit of looking at the details of how Fastly handles query escapes with the querystring.* functions. From a non exhaustive check it seems like they're performing escaping only when setting values but not when reading them or performing other operations on them (filtering, sorting).

For example the following will log /?a=b+c&b=c%20d not /?a=b%20c&b%20c.

log querystring.set("/?a=b+c", "b", "c d");

Fiddle with a few more examples: https://fiddle.fastly.dev/fiddle/e019c129

If you're up to tackling investigating this further and incorporating the needed changes to the PR that would be great. If not we can create an issue to track the remaining work.

[akrainiouk]

Hi Richard, I am afraid we are opening a Pandora box :) The problem of course is deeper than I attempted to address here. Current implementation first parses the source value into a structure then manipulates it and finally converts back to string. This however introduces several unwanted side effects such as grouping name value pairs (implied by underlying query structure), it also makes it hard to follow various Fastly quirks. For instance in some cases (e.g. sort) original '?' separator is preserved even if the query is empty, in other cases (filter) it is removed. Another quirk is that sorting is applied to unsplit name=value pairs so that

querystring.sort({"?foo=one&foo>=too&foo<=three"})

returns

?foo<=three&foo=one&foo>=too

and not

?foo=one&foo<=three&foo>=two

as one might expect. If you guys are interested, in this PR draft I provided an alternative implementation of querystring functions that do not try to pre-parse the source string and attempt to follow Fastly behavior as much as I can check. I converted existing unit tests so that at least existing test cases are still valid and added some more (the behavior is more of less covered in this Fastly fiddle.