Query parameter encoding

[tomsmalley]

In v0.18, query parameters were converted with toQueryParam and later encoded as part of renderQuery.

In v0.19, this was changed so that conversion and encoding of query parameters is done at once by toEncodedUrlPiece, and the parts are joined without doing extra encoding.

At first glance this seems like a good change since it allows more control over how encoding is done, but there's a subtlety:

The default implementation of toEncodedUrlPiece uses encodePathSegmentsRelative, which uses urlEncodeBuilder but with the first argument set to False: i.e., denoting the input as a path element, not a query string.

So, when using servant "end to end" on the same API with String or Text query params, the server no longer gets what the client sent if any of the extra query special characters are present &=+$,. I think this change wasn't intentional, thus this ticket.

I suppose a more correct change would be to add toEncodedQuery or something to the class, but that's a much larger change.

[gdeest]

@tomsmalley Thanks for opening an issue ; as the author of the change I can attest that this indeed wasn't intentional (and I am truly sorry for any problem it may have caused downstream).

It sounds like the better fix might be to restore the original implementation and allow more legal characters (such as [], which I think prompted the initial change) in http-api-data. What do you think ?

[tomsmalley]

@gdeest I think we're talking about different patches, this is the change that caused the query param issue: #1432

[tomsmalley]

The workaround we have is to replace all of the Text in query params in our API with

newtype QueryEncodedText = QueryEncodedText {unQueryEncodedText :: Text}
  deriving newtype (ToParamSchema)

instance ToHttpApiData QueryEncodedText where
  toEncodedUrlPiece = urlEncodeBuilder True . encodeUtf8 . unQueryEncodedText
  toUrlPiece (QueryEncodedText t) = t

instance FromHttpApiData QueryEncodedText where
  parseUrlPiece = pure . QueryEncodedText

i.e. encoding as a query param in the path piece encoding function, and not decoding in the parser.

[GambolingPangolin]

Oops! I fixed one problem but created another 🤦🏻‍♂️ Zooming out as far as possible, I think that trying to get to the point where ToHttpApiData and FromHttpApiData instances completely handle encodings would be best. It is a confusion of responsibility when the class members define a textual representation that needs further encoding. However that is a huge coordination challenge and might not even be possible at this point.

The least worst solution that I can see is to revert the change. Handling string-like query parameters and path segments without much ceremony is a much more important use case than handling raw binary query parameters.

Since the unreserved characters for path components are a superset of those for query strings, another option is to add a pass over the encoded query string bits to encode any remaining special characters among ":@&=+$,".

[lagunoff]

@tomsmalley @gdeest @GambolingPangolin Checkout #1597, there is a quick fix that encodes special symbols in param values. Certainly a good solution would be add method like toEncodedQuery to ToHttpApiData typeclass, but now it's much easier to apply fix on servant side to encodeQueryParamValue function as it's done in the PR