Is DeepQuery missing a HasLink instance?

[saurabhnanda]

Upgraded to servant-server-0.20.2 to specifically use the new DeepQuery feature, however, got the following compilation error upon using it:

/home/vl/vacationlabs/haskell/src/Plugin/ItinerarySegments/Links.hs:13:14-38: error:
    * There is no instance for HasLink (DeepQuery
                                          "search" Models.ItinerarySegment.SearchOpts :> ...)
    * In the expression:
          allFieldLinks' linkToText :: Routes (AsLink Text)
      In a pattern binding:
        Routes {..} = allFieldLinks' linkToText :: Routes (AsLink Text)
   |
13 | Routes{..} = allFieldLinks' linkToText :: Routes (AsLink Text)
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^

[tchoutri]

cc @divarvel

[saurabhnanda]

@tchoutri @divarvel does this look good to raise a PR (except the toS stuff, of course):

instance (KnownSymbol sym, ToDeepQuery record, HasLink sub) => HasLink (DeepQuery sym record :> sub) where
  type MkLink (DeepQuery sym record :> sub) a =
    record -> MkLink sub a

  -- toLink :: (Link -> a) -> Proxy endpoint -> Link -> MkLink endpoint a
  toLink toA _ lnk record =
    toLink toA (Proxy @sub) $ addParams lnk
    where
      k :: Text
      k = toS $ symbolVal (Proxy @sym)

      mkSingleParam x = 
        let (a, b) = generateDeepParam k x
        in SingleParam (toS a) (toS $ escapeURIString isUnreserved $ toS $ fromMaybe "" b)

      addParams lnk = 
        DL.foldl' (\memo x -> addQueryParam (mkSingleParam x) memo) lnk $ 
        toDeepQuery record

Also, why does ToDeepQuery deal with Text when inserting the results into the actual Link would force one to convert them to String?

[divarvel]

ah yes, the missing HasLink instance is an overview indeed.

The PR makes the choice to use Text everywhere instead of String in alignment with Network.HTTP.Types which provides QueryText, and more generally to avoid using strings as much as possible since it’s good haskell practice.

I forgot a bit about the original PR, but the HasLink instance should look a lot like the HasClient instance.

A PR adding HasLink support for DeepQuery should also add it to QueryString (the subtle difference is that DeepQuery qs params should be added to the query string, while QueryString qs params should replace the query string (see the difference between the two HasClient impls).

[tchoutri]

@divarvel @saurabhnanda Can either one of you submit a PR? I'm preparing the next release with bugfixes. :)

[saurabhnanda]

@tchoutri @divarvel we have been used the HasLink instance of DeepQuery shared above for a few months in production now. Every time we end-up using it, we land-up with a bug-report in our QA environment because of the following:

• ToDeepQuery expects non url-encoded values for URL params
• However, FromDeepQuery expects the implementation to url-decode the param values manually

Is this observation correct, or have we misunderstood something?

[saurabhnanda]

@tchoutri every time I deal with QueryParams in Haskell I end-up wondering why we don't have newtypes to differentiate between encoded vs non-encoded param values? Isn't that the text-book case for using newtypes?

[tchoutri]

@saurabhnanda yeah that's a good use-case, especially if it prevents incidents.

[tchoutri]

@kutyel has volunteered for the PR, I just need @divarvel to confirm/infirm https://github.com/haskell-servant/servant/issues/1784#issuecomment-2765698056 and we should be good.

[divarvel]

yeah, query params handling is complex since in many cases you can provide them un-encoded to user-agents and it will work. in some cases it’s the expected behaviour (people usually prefer seeing [] in the url box, rather than %5B%5D).

As for the requirements of FromDeepQuery and ToDeepQuery, it depends on where servant-server and wai place the responsibility of url decoding query param segments, and the converse for ToDeepQuery. Both HasClient and HasServer instances pass values as-is, without performing encoding or decoding, so it depends on wai for the server and appendToQueryString in servant-client. Neither wai nor servant-client mention explicitly what’s expected wrt encoding of query params there, sadly.

[tchoutri]

Ok, looks like it's also the opportunity for us to clarify that.

[divarvel]

Yes. Thinking about it, imo ToDeepQuery and FromDeepQuery should only work on non-encoded values. So it would be good to make sure that the HasClient and HasServer instances correctly expect/pass decoded values

[tchoutri]

@kutyel You can even perform a partial encoding, with / becoming %2F, but leave brackets and @ in order to increase readability.