QueryParamForm Revisited

[erewok]

Hi Everyone,

Included here are changes to finalize QueryParamForm (see Issue #728 and PR #729) which I mentioned to @alpmestan on IRC. The original request seemed like a good idea to me and the help-wanted label (plus the fact that it hasn't been updated in 15 months) made me think it would be a good thing to contribute.

I broke this PR description down by Servant- section so I could highlight solutions and questions pertinent to each section. There's a todo section at the bottom.

I've been kicking this PR around in my head for a couple of days and I think I could use some of your feedback to polish it off.

Sevant-Server

In this area, I went with something similar to the original PR but there was one issue I went back and forth on. Consider the following example:

type SampleApi = 
  "simple" :> QueryParamForm "searchQ" BookSearchParams :> Get '[JSON] [Book]
   :<|> "complex" :> QueryFlag "published" :> QueryParamForm "searchQ" BookSearchParams :> Get '[JSON] [Book]
   :<|> "confused" :> QueryFlag "published" :> QueryParamForm "searchQ" BookSearchParams :> QueryParam "sort" Bool :> Get '[JSON] [Book]

Are all of these valid?

Consider that they can all work using urlDecodeAsForm:

Web.HttpApiData Web.FormUrlEncoded
λ> urlDecodeAsForm "anotherthing=fly&cname=test&cemail=bla&whatabout=1&cmessage=test&other=hey" :: Either Text ContactForm
Right (ContactForm {cname = "test", cemail = "bla", cmessage = "test"})

However, it's more complex to allow all of these from a server implementation. The easy answer (which I did here) is to check if there are any query params and then try to decode the QueryParamForm sym a, otherwise, assume there are none. That may make sense because a QueryParamForm could be a way of wrapping up all QueryParams into one: in other words, you wouldn't use a QueryParamForm in combination with other query params.

If we go with this simple answer, then the docs should definitely be updated to be super clear: only use one QueryParamForm if you aren't using any other Query- things.

I wasn't really satisfied with my implementation of this simple version, to be honest, but I wanted to see what the group thinks before working on a more complex implementation.

(By the way, if you look at my tests, you can see me muddling through and exploring this.)

Sevant-Client

This one seemed more straightforward: it's a ToForm a => Maybe a arg and then you can urlEncode it. I need to add tests for it, though, and validate it's definitely correct.

Servant-Docs

My first implementation of this just treated a QueryParamForm item as another DocQueryParam, but I wasn't really satisfied by that.

Instead, I went with a set of constraints (ToForm a, ToSample a, Data a) and then I created a sample and used the constructors to make a bunch of DocQueryParams.

It's not perfect (all Values are repeated), but I liked it better than the first solution I had. It ended up making docs that look like this:

## GET /qparamform

### GET Parameters:

- dt1field1
     - **Values**: *dt1field1=field%201&dt1field2=13*
     - **Description**: Query parameter

- dt1field2
     - **Values**: *dt1field1=field%201&dt1field2=13*
     - **Description**: Query parameter

Would love to hear what you all think and would like to see. There are certainly other directions you could go in here as well, such as representing these things completely differently.

Servant-Foreign

Honestly, wasn't sure on this one. Please take a look and let me know it it makes sense.

To Do

• Update servant-server docs to make it really clear whether you can use QueryParamForm with QueryParam, QueryFlag`, etc.
• Look into tests for servant-client-core.

[erewok]

I ran the tests locally using GHC-8.4.3 and they all passed (same for Travis build). Travis says there was an exit code 1 for 8.2.2 (but I didn't see any failing tests?) and it didn't complete for 8.0.2?

[alpmestan]

Thanks for the patch! I'm just leaving a quick comment right now, to try and address your question. Will do a proper review when I get a chance (I've been quite busy lately).

Are all of these valid?

Great question. First, we may want to differentiate between the case where the other params are contained in the QueryParamForm one and the one where they are not.

In the case where they are, one could argue your APIs are just as valid as: type API = "hello" :> QueryParam "foo" Text :> QueryParam "foo" Text :> Get '[PlainText] Text. Which I think "just works" with servant-server as it stands. But what happens with servant-client there? You get two Maybe Text arguments to specify the same query parameter. I suppose the last one wins, even though I haven't really tried this. Feel free to. So it looks like there is some potential for things to go wrong whenever there is some overlap, and I think it just generalizes straightforwardly to your QueryParamForm business. While the server side won't mind the redundancy and will happily hand you redundant data (I think), the client side (whose behaviour is always "dual" in a way) doesn't really have a meaningful way to get out of this situation. While I don't have any good solution to prevent the "last one wins" behaviour in the two-QueryParams scenario, I'm beginning to think it might be good to not try to support overlapping params at all and make it clear everywhere. Note that those overlaps make things tricky for other interpretations (docs, foreign) too, which basically have to try and figure out what bits overlap if they want to do things well.

If we allow ourselves to assume there is no overlap (i.e if we tell users "you can mix QueryParam and QueryParamForm as long as they don't overlap"), then I think we can write meaningful interpretations of QueryParamForm that don't try to be too smart and just do what we expect. In which case a QueryParamForm doesn't always need to contain all the query string data, but sometimes potentially just a subset, where the other params are specified separately (or, let's go nuts, with another QueryParamForm, we shouldn't care as long as we leave the responsability of making sure there's no overlap to the user). How does that sound? Am I overlooking something?

[erewok]

In the case where they are, one could argue your APIs are just as valid as: type API = "hello" :> QueryParam "foo" Text :> QueryParam "foo" Text :> Get '[PlainText] Text. Which I think "just works" with servant-server as it stands.

I was surprised by this. To verify, I modified ServerSpec to include another QueryParamApi endpoint:

type QueryParamApi = QueryParam "name" String :> Get '[JSON] Person
                ...
                :<|> "param-odd" :> QueryParam "age" Integer :> QueryParam "age" Integer :> Get '[JSON] Person

qpServer :: Server QueryParamApi
qpServer = queryParamServer :<|> qpNames :<|> qpCapitalize :<|> qpAge :<|> qpAges :<|> qpWacky

  where ...

        qpWacky (Just age1) (Just age2) = return  alice{ age = age1 + age2}
        qpWacky _ _ = return alice

Then I wrote a test like this:

  it "parses multiple query parameters of same name" $
        (flip runSession) (serve queryParamApi qpServer) $ do
          let params = "?age=25"
          response <- Network.Wai.Test.request defaultRequest{
              rawQueryString = params,
              queryString = parseQuery params,
              pathInfo = ["param-odd"]
            }
          liftIO $
              decode' (simpleBody response) `shouldBe` Just alice{
                age = 50
              }

And, as you stated, it works. This is kind of confusing from a client standpoint: would an ideal client understand that it's the same param (which would mean looking at all params for each endpoint comparatively as a set?

I think what you've said otherwise makes sense to me. I'll try to work toward that as a solution for servant-server and come back with any questions.

[alpmestan]

And, as you stated, it works. This is kind of confusing from a client standpoint: would an ideal client understand that it's the same param (which would mean looking at all params for each endpoint comparatively as a set?

Depends on whose ideals we're talking about. My opinion: I think I'd rather implement some day one old idea, which would be to run a bunch of (type level) checks on the API type before it's fed into serve (or client or ...), or maybe just as a separate functionality. The "no duplicate query params" (duplicate in the name of the param, the types could differ and that would still be a problem) check could just be one of them. It just doesn't make any sense to try and work with those API types in my opinion, but it's just not prevented right now.

[erewok]

@alpmestan I have another question for the server implementation that I could use your input on.

Let's say I have a form like this used in an API as QueryParamForm:

data BookSearchParams = BookSearchParams
   { title :: Text
   , authors :: [Text]
   , sort :: Maybe Bool
   } deriving (Eq, Show, Generic)
instance FromForm BookSearchParams

If I'm the server and there are other query params which could also be supplied, I think I need to decide when someone intended to send this form before I can say that it didn't parse successfully.

For example, consider these query strings:

λ> urlDecodeAsForm "title=great&authors=abe&authors=mary" :: Either Text BookSearchParams
Right (BookSearchParams {title = "great", authors = ["abe","mary"], sort = Nothing})
λ> urlDecodeAsForm "authors=abe&authors=mary" :: Either Text BookSearchParams
Left "Could not find key \"title\""
λ> urlDecodeAsForm "limit=1000" :: Either Text BookSearchParams
Left "Could not find key \"title\""

Here, for example, if either the key title or authors is present, then the client probably wanted to use this form to submit query params and a failed parse results in Left "decoding failed" (err400) or something like that?

However, if none of these keys are presents in the query string (but others may be) and it's 'Optional, then it should probably be a Nothing result (the last example above).

There's a bit of intention sniffing here that I'm hesitant about due to the fact that these are not independent query params but a collection of them in some kind of implied relationship. Should I unpack the data structure and expect that if any key not resulting in a Maybe value is present in the query params, then we're going the Left ... route? (Unsure how to build that, actually...)

Alternately, all fields in the record could be wrapped in Maybe and then even empty strings won't fail to parse.

When would I need to return Nothing and when Left "decoding failed..." value? This seems more obvious with single query parameters but with a collection of them, there are some assumptions involved and it's maybe a bit messy.

[alpmestan]

I'm not sure I understand where/why you have to do what you call "intention sniffing". Aren't the FromForm/ToForm instances (so, written or derived by the user) for BookSearchParams exactly where those decisions are made? Can't you on your end just completely rely on those instances and just see whether you get Left or Right etc? It's then up to users to lookup the params they want and decide what to do when some of them are missing etc. No?

[erewok]

Apologies: I think I did a poor job of explaining the problem. This relates to the previous question about having QueryParam as well QueryParamForm in a single endpoint.

Maybe I'm missing something obvious.

For example, let's say we have an API like this where there's a QueryParam as well as a QueryParamForm:

type SearchAPI =
    "search" :> QueryParam "sort" Text :> QueryParamForm "search" BookSearchParam :> Post '[JSON] [Book]

Presumably, from this definition we'd want to be able to parse the sort param if it's been passed and then have Nothing for the search param if the form hasn't been passed, but the server would have to do some deeper inspection on the keys in the query string to do that successfully. This query string, for example, would return a 400 when the server attempted to parse the BookSearchParam form:

λ> urlDecodeAsForm "sort=desc" :: Either Text BookSearchParams
Left "Could not find key \"title\""

A naive server implementation (like the one I have here) would simply check if any query params had been passed and attempt to parse out the form, but then with this naive implementation in the above API example with this query string it's not really possible to get the values Just "asc" and a Nothing because the second parse will always fail.

On the other hand, with something like this:

λ> urlDecodeAsForm "sort=asc&authors=abe&authors=mary" :: Either Text BookSearchParams
Left "Could not find key \"title\""

You'd probably want to actually return a 400.

[alpmestan]

Sorry, but I still don't understand the problem. I must be tired these days, but for me, what you want can entirely be implemented in the ToForm/FromForm instances, given the "natural" implementation of HasServer for QueryParamForm (I haven't examined closely the code for it, but I think it should be pretty close to the one for QueryParam, except that you feed it the entire query string (and the FromForm instance is then free to pick anything it wants from it) instead of just a particular one. But again, my brain has a bit of a hard time these days.

@phadej Your brain usually works a lot better, could you look at @erewok's question above when you've got a bit of time?

[erewok]

Hi @alpmestan. Let me try again to explain myself. Consider this API again that I mentioned before:

type SearchAPI =
    "search" :> QueryParam "sort" Text :> QueryParamForm "search" BookSearchParam :> Post '[JSON] [Book]

Let's assume we've got some handler function of the following form:

searchBooksHandler :: Maybe Text -> Maybe BookSearchParam -> Handler [Book]
searchBooksHandler = ...

Now, consider the following requests and the expected Handler args:

Query Param Sample	Expected Handler Args	Actual Handler Args or Result
?title=great&authors=abe&authors=mary	(Nothing, Just BookSearchParam)	(Nothing, Just BookSearchParam)
?sort=asc&authors=abe&authors=mary	Left "Could not find key \"title\""	400 Left "Could not find key \"title\""
?sort=desc	(Just Text, Nothing)	400 Left "Could not find key \"title\""

In the first case, the requester did not supply the first, optional QueryParam. In the second case, the requester sent in a malformed QueryParamForm. So far so good.

However, there's a problem in that last case: if you just go with FromForm decode on the query params (whenever any are presnet), you'll always get a Left failure, when sometimes you may wish to have had a Nothing, meaning "No QueryParamForm submitted".

That's why I was trying to say above: it seems like you have to look for at least one of the keys from the QueryParamForm in order to guess that the intention was to have submitted one of those.

Does that make sense?

[alpmestan]

Yes, thanks for taking the time to spell things out properly for me. I think I see the problem.

As far as I can tell, the problem basically is that we can't really make Required/Optional work. Because we indeed can't really know if the client even tried to pass a QueryParamForm unless we know what query params to look for, which we don't.

The only reasonable, non-surprising approach that I see is: if fromForm gives us a Left, we either error out or hand the Left to the handler, depending on whether we're using the Strict modifier or the Lenient one, and if it gives us a Right, we either hand the value that it wraps to the handler directly, or keep it wrapped in the Right when handing it to the handler. How does that sound?

[erewok]

Thanks for being patient and working through it with me. What you suggest sounds good. I'll go for that. I will likely be unavailable for working on this over the weekend, so sometime next week I'll try to wrap it up.

I'll pull master too in order to make sure CI is solid.

[alpmestan]

Great, thanks for your work on this!

[erewok]

@alpmestan I think this one is ready. I changed QueryParamForm to be '[Optional, Lenient].

I was also trying to get the documentation correct (still new to the format), and I was not able to build the haddocks locally, so I had to guess a bit on the look of the documentation.

[erewok]

oh wait! I forgot to add a test for servant-client. One last thing, then...

[alpmestan]

So, let me sum it up: if there's no query string, we return Nothing. And if there's one but we can't "decode it", we return Just (Left ...), otherwise we return Just (Right ...). Correct? And that's why we always go for Maybe (Either Text a)?

(sorry, I realize you can't wait for this to land)

[erewok]

No hurry from me to merge anything: I'd rather get it right. You summation is exactly what I ended up with.

[alpmestan]

@erewok Hmm, in that case I think some docstrings need to be updated. E.g the one in the client package mentions wrapping the user's type in Maybe, while there'll be an Either layer too. Other than that it's looking pretty good now.

You might or might not want to add a few words about this either in the tutorial (if you find a good place for it) or in a dedicated cookbook recipe. This is AFAICT the best way to advertise existing or new features/combinators. But that requires time too, so it's up to you :-)

[erewok]

In servant-client, though, it is just a Maybe a because we know that if a ToForm a instance exists, then the client will successfully convert the record. In that case, we just need to know whether it has been supplied or not.

I'd be happy to add something in one of the tutorials about this. In fact, I thought about that a few times: it's hard to understand how to use or why it's Lenient, which results in a Maybe Either.... and a tutorial would allow me more space to lay it out. I wonder if it's worth adding a new tutorial about the different query-options?

[alpmestan]

Well, the tutorial is https://haskell-servant.readthedocs.io/en/stable/tutorial/index.html and is supposed to be a "servant starter kit" kind of thing. Not sure what you're saying fits the format. Maybe a dedicated cookbook recipe to modifiers, with examples using different combinators, including the new one you're introducing here, would be best? If you can make it fit in the tutorial somewhere, that'd work for me as well. Note: this doesn't have to be done in the same PR, if you'd rather split up those two things.

[erewok]

Okay, sounds good. After this one is merged, I'll create a new cookbook recipe and create a new PR. For this one, it looks like I have another merge conflict to resolve, which I'll have to do this evening (California time).

[alpmestan]

Great. Yes, the 0.15 release is getting out, some change in -client-core needs your attention. Your changes will quite likely be shipped in the very next release (AFAICT there's no breaking change, so we can just ship it in 0.15.1 if we want) -- to be confirmed with @phadej.

[erewok]

Hi @alpmestan and @phadej: I think everything we discussed has been done here, so this one should be ready? Please take a look and let me know if you me to make any other changes. Otherwise, I wasn't planning to do any more to it.

[alpmestan]

I'm happy with this now. Glad we worked it all out!

Are you planning to add a cookbook recipe about this or something like that? Or perhaps a tiny section in the tutorial or something. Just to advertise it. Not in this PR though of course.

[erewok]

Yeah, I think the cookbook recipe we talked about up above about modifiers in general would be cool to have and I'd be happy to do it. Feel free to give me some notes on what you'd want in there.

[erewok]

Hi @alpmestan and @phadej,

Hacktoberfest jogged my memory that I opened this PR last year and it's been (with apologies) a year since I thought about it.

I worked on it originally because @alpmestan added help-wanted to #729 which was meant to close #728 , and as #729 hadn't seen any work in quite awhile, I figured I could put it together as a new PR.

I didn't totally love the way it was implemented here but it's also not something I wanted to use right away in Servant, so I didn't do the work of pushing on this PR to a resolution. I mostly did it as a way to contribute back to Servant, which is a project that I have a lot of affection for.

Now that I have remembered it, and in the interests of helping out the Servant project (and hopefully closing issues and PRs helps in that effort), I wanted to know what you'd like to see done with this PR:

• Abandon this one and come up with a better solution some time in the future (and close QueryParamForm Combinator #729 in the process because this is an extension of those).
• Fix merge conflicts, bring it up to date with current Servant, and get it merged.
• Add the missing cookbook recipe we talked about above to this one, fix merge conflicts, bring it up to date, and get it merged.

I respect that it takes a lot of time and effort to run a popular open-source project such as this one, so I'm happy to do whatever you both think should be done with the work already performed here. I have no qualms about abandoning this PR, for instance, if that's what you suggest.

Thanks.

[alpmestan]

Apologies for not staying on top of this as well, I've indeed been spending a bit less time on open-source stuffs.

I'm personally still keen on landing this, once rebased and everything.

[erewok]

Thanks for your reply, @alpmestan . It sounds like you'd vote for option 2 or 3? In that case, I'll start work on bringing it up to date with contemporary servant. Expect an update to this PR sometime soon.

[alpmestan]

We can go for 2, and if you later feel like writing a little thing for the cookbook that'd be great.

[alpmestan]

@phadej Looks good to you?

[phadej]

This seems confusing to me. If QueryParamForm is Optional we should always match that route then, independently whether there is ? or not.

The difference between http://example.com/ and http://example.com/? is too subtle.

[erewok]

I'm not sure I understand the critique about matching routes because they both match: in the first case the handler will get a Nothing while in the second the Handler will get a Just . Left a. The routes do match, though?

This PR has always been based on the original PR which had been abandoned in an incomplete state, so I wouldn't take personally any criticism of it. In fact, I think there's a general criticism worth making that this implementation may cause some surprise among users. One thing I thought may help is to define different, somehow more specific names for the Optional and Required variants.

Another idea would be for the Optional case to return a Maybe form instead of a nested Maybe (Either a form)), in other words to give the user a Just form only in the Just (Right form) case and Nothing everywhere else. You would probably have to surrender the Lenient, Strict axis in that case and just say, "it's there or not there," but that may be confusing too and lead to buggy behavior if a query param is submitted misspelled or mistyped. Actually, this goes back to that conversation @alpmestan and I had above at some point in the hazy past...

[phadej]

This PR remind me how hugely inconsistent (indentation) style we have in servant :( If there only were any formatter which doesn't break on CPP stuff.

[erewok]

@phadej and @alpmestan, aside from the open-ended discussion we had above, I have made the requested changes.

[willbasky]

@erewok @phadej @alpmestan
Hi guys!
What blockers stop merging the PR?

[fisx]

Just lack of time. I just joined the team and have no context here, but I'll try to take a look at it soon.

[erewok]

@willbasky there was some dissatisfaction with the implementation here and @phadej had mentioned elsewhere wanting to go in a different direction.

[willbasky]

@fisx cool! This thing is very wanted.

@erewok I wonder which ones?

If I could help to speed up this PR, let me know, please.

[acondolu]

Hi all! I second the comments above, I find this feature very useful and I am looking forward to it being merged! (Also, thanks @erewok for taking care of this PR!)

The natural question is: what can we do to help have it merged?

By reading the thread above, I think there are mainly two issues that slow down merging:

1. Change of maintainers: earlier this year, servant has been taken over by different maintainers. The new ones did not participate in the above discussion and may need additional time to get up to date. Moreover, they may have different opinions or comments than previous reviewers.

2. The way QueryParamForm should fail: a mismatch in behaviour between QueryParamForm and QueryParam has been discussed. Let me recap. The reason for the mismatch is that, to parse a ParamForm, we rely on the external function urlDecodeAsForm, which either errors or succeeds. With combinators like QueryParam, instead, we can be more precise and distinguish two different kinds of failure: a parsing error vs the query parameter not supplied. This difference is important because it can result in a route being matched and fail with 400, or not being matched at all. Because QueryParamForm relies on multiple parameters, instead, it is ambiguous whether a ParamForm is "present" or not.
   By using the modifiers Strict, Optional, Lenient, one can change the matching behaviour. This PR currently parses QueryParamForm in an Optional and Lenient way, hence obtaining something of type Maybe (Either Text a), where:

   • Nothing if no query string is supplied at all (empty query string)
   • Just (Left err) in case the query string is non-empty and urlDecodeAsForm fails
   • Just (Right x) in case urlDecodeAsForm succeeds.

   In a comment above, phadej was pointing out at the subtle difference between the first two cases, also being contrary to it.

I hope this recap can help unlock the PR (feel free to correct or add anything). It's now been more than 3 years since the first discussions on this feature 🤯

[tchoutri]

Hi @erewok! We are amenable to merging this PR for the next major version of Servant. Could you please address the 2nd point in Andrea's last comment? :)

[erewok]

Hello @tchoutri! It's been a very long time since I've thought about this PR. Reading the comment above it rehashes the conversation we had throughout here. Oleg had an idea which he never clarified but for my own part, I was just trying to finish the feature based on a previous PR, but I don't have much for opinions about the implementation (apologies).

There have been a lot of people over the years who have said that they would us this feature. Perhaps it makes sense to get this PR merged and then to refine it based on user feedback?

[alpmestan]

There have been a lot of people over the years who have said that they would us this feature. Perhaps it makes sense to get this PR merged and then to refine it based on user feedback?

FWIW, sounds good to me. I think our initial efforts of fleshing out and clarifying the subtleties brought this to a good enough place for the time being.

[tysonzero]

Any updates on this front? We've been considering moving to a Post-only api solely because of nice ReqBody ergonomics, but for safe/idempotent endpoints it'd most likely be better to use something like this.

[stevladimir]

Shouldn't it have type Maybe BookSearchParams -> ClientM [Book]?

[verifiedtm]

This looks really great, I would love to see this in Servant.