New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple QueryParams aren't delimited with & #23

Closed
declension opened this Issue Feb 24, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@declension
Contributor

declension commented Feb 24, 2017

I may have misunderstood, so am quoting the test api / spec I used to narrow down the problem:

Test API

With this basic API:

type QueryStringAPI = "path" :> QueryParam "one" SafeText :> QueryParam "two" SafeText :> Get '[JSON] Text
qsApi:: Proxy QueryStringAPI
qsApi = Proxy

newtype SafeText = SafeText Text
instance ToHttpApiData SafeText
  where toUrlPiece (SafeText st) = st

Spec

..and a serverSatisifies spec like:

burl = BaseUrl Http "localhost" 8080 ""
spec = it "Query String API doesn't return 500" $
      serverSatisfies qsApi burl stdArgs
        (not500 <%> mempty)

instance Arbitrary SafeText
    -- Ensure it's not the random characters being the problem
    where arbitrary = return $ SafeText "value"

Output

Using a basic HTTP server (python -m SimpleHTTPServer 8080 here), I see lots of this in the logs:

127.0.0.1 - - [24/Feb/2017 18:01:06] "GET /path/?two=valueone=value HTTP/1.1" 404 -

Whilst I think the path/ is covered separately in #22, it's the missing & here that means I can't query the real API I was examining. Is this a bug?

Note: a client created with servant-client works as expected, with ampersands delimiting multiple (separate) QueryParams...

@declension

This comment has been minimized.

Show comment
Hide comment
@declension

declension Feb 24, 2017

Contributor

(for reference, from IRC): Line 95 (probably) needs to check whether the queryString is empty, and if it's not, use '&' before adding the param

Contributor

declension commented Feb 24, 2017

(for reference, from IRC): Line 95 (probably) needs to check whether the queryString is empty, and if it's not, use '&' before adding the param

declension added a commit to declension/servant-quickcheck that referenced this issue Mar 6, 2017

Fix multiple QueryParams
 * Add test API taking multiple `QueryParam`s
 * Add basic test using this API, generating an endpoint to ensure correct HTTP `one=foo&two=bar` query string generation is happening (that fails on `master`)
 * Fix (re)creation of query string to append `&` before the new parameter if there is already a built query string.

Fixes #23.
@declension

This comment has been minimized.

Show comment
Hide comment
@declension

declension Mar 7, 2017

Contributor

(Also realised this applies to QueryFlags)

Contributor

declension commented Mar 7, 2017

(Also realised this applies to QueryFlags)

declension added a commit to declension/servant-quickcheck that referenced this issue Mar 7, 2017

Fix QueryFlags too (#23)
 * Same logic / testing as for `QueryParam`
 * There's probably some de-duplication that could be done here one day...

@jkarni jkarni closed this in #24 Mar 10, 2017

@jkarni

This comment has been minimized.

Show comment
Hide comment
@jkarni

jkarni Mar 11, 2017

Member

Released as 0.0.2.3

Member

jkarni commented Mar 11, 2017

Released as 0.0.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment