Skip to content
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.

Sign up for GitHub

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

Jump to bottom

QueryParamForm Combinator #729

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

QueryParamForm Combinator #729

wants to merge 2 commits into from

Conversation

isairz
Copy link

@isairz isairz commented Apr 10, 2017

#728

type MyApi = "book" :> QueryParamForm BookSearchParams :> Get '[JSON] [Book]

This is my first pull request in servant.
Please feel free to review my commits 😃

  • Add API
  • instance and test for servant-server
  • instance and test for servant-client
  • instance and test for servant-doc
  • instance and test for servant-foreign

fizruk
fizruk reviewed Apr 12, 2017
View reviewed changes
servant-server/src/Servant/Server/Internal.hs
@@ -405,6 +406,48 @@ instance (KnownSymbol sym, HasServer api context)
examine v | v == "true" || v == "1" || v == "" = True
| otherwise = False

-- | If you use @'QueryParamForm' BookSearchParams@ in one of the endpoints for your API,
-- this automatically requires your server-side handler to be a function
-- that takes an argument of type @['BookSearchParams']@.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of type @'BookSearchParams'@, right?

servant-server/src/Servant/Server/Internal.hs
-- > { authors :: [Text]
-- > , page :: Maybe Int
-- > } deriving (Generic)
-- > instance FromForm BookSearchParams
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably mention that we default implementation uses FromHttpApiData for each field.

servant-server/src/Servant/Server/Internal.hs
subserver `addParameterCheck` withRequest paramsCheck
where
paramsCheck req =
case urlDecodeAsForm (BL.drop 1 . BL.fromStrict $ rawQueryString req) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess BL.drop 1 is about dropping leading ??
Can rawQueryString contain extra info (e.g. HTML anchors?), would it ruin parsing?

@fizruk
Copy link
Member

fizruk commented Apr 12, 2017

Also, I don't see any changes for servant-client, for that you should symmetrically use ToForm.

@alpmestan
Copy link
Contributor

@fizruk the servant-client interpretation is still not ticked in the first comment in this issue :) I agree with fizruk's feedback, lacking the time to do a proper review but this is a good start! I'll review when it's all ready.

@fizruk
Copy link
Member

fizruk commented Jul 7, 2017

@isairz are you going to work on this soon or should we take over?

@isairz
Copy link
Author

isairz commented Jul 10, 2017

@fizruk Sorry, I was so busy and did not care.
I will work on this soon.

@erewok erewok mentioned this pull request Oct 9, 2018
Open
2 tasks
@fishtreesugar fishtreesugar mentioned this pull request Jan 14, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fizruk fizruk fizruk left review comments

Assignees
No one assigned
Labels
enhancement 🚧 needs rebase 🚧 question
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@isairz @fizruk @alpmestan @phadej @tchoutri