search

haskell-servant / servant

Servant is a Haskell DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.83k stars 413 forks source link

Disallow ReqBody combinators for GET API endpoints #516

Closed tdietert closed 8 years ago

tdietert commented 8 years ago

In the HTTP protocol specification, (e.g., RFC 2616, section 4.3), GET and HEAD requests should not cause side effects on the server. By allowing the ReqBody combinator for GET and HEAD API endpoints in Servant, side effectful code is, arguably, easier to write when writing handlers of these request types. Furthermore, the xmlhttprequest specification states that GET and HEAD request bodies should be treated as null or discarded entirely before sending the final request to the server. Although servant does not use xmlhttprequests, it might be nice to mimic the way http requests operate in other forms of web applications apart from Servant and Haskell.

With this change, GET and HEAD can still contain QueryParam combinators, so users will still be able to send extra information to the server when using these two API endpoint types. These changes will also help the integration of servant-client-ghcjs into the master branch as ghcjs uses xmlhttprequests, and currently fails some tests that work when using servant-client, e.g. when using ReqBody in GET and HEAD API endpoints.

3noch commented 8 years ago

It's worth considering a way to disable this requirement in a pinch. Developers are not always designing an API but sometimes building one based on an existing specification/example. Sadly they are not always up to snuff in terms of RFC compliance.

jkarni commented 8 years ago

It'd be nice if #345 were merged for this.

I'm imagining something like

type family MethodOf a where
   MethodOf (Verb status method ...) = method
   MethodOf (a :> b) = MethodOf b

instance (...,  Not ( (GET `Elem` Map MethodOf (Endpoints b)) 
                      `Or` (HEAD `Elem` Map MethodOf (Endpoints b))
              ) HasServer (ReqBody ... :> b)
soenkehahn commented 8 years ago

I'm with @3noch on this. servant should allow to send bodies in GET requests. A bit of googling reveals that for example older versions of ElasticSearch require that.

So if we don't find an easy way to make this standard-compliance check optional, I would be against it.

jkarni commented 8 years ago

What about only making the check for servant-server?

soenkehahn commented 8 years ago

The same argument about other software that may not be standard-compliant can also be made here: What if I want to implement a server for clients that misbehave?

(Apart from that: the initial motivation for this issue is the lack of support for Get request bodies in servant-client. If we only disallow them in servant-server, that doesn't help there.)

tdietert commented 8 years ago

Thanks for everyone's input! So how can we get this working with GHCJS? Conditional compilation to disallow request bodies in GET and HEAD? Simply not have a test for ReqBody combinators for GET and HEAD when running tests for ghcjs? Trying to get servant-ghcjs merged with master is one of the main reasons I brought up this issue.

jkarni commented 8 years ago

@soenkehahn fair point.

@tdietert I don't think we should have ReqBody in GET and HEAD tests either for ghc or ghcjs.

Should we close this issue then? We could maybe just mention in the docs that you shouldn't use ReqBody with GET/HEAD.

soenkehahn commented 8 years ago

@jkarni: Yes, I'll close this. Please, discuss ghcjs issues in #51.

alexanderkjeldaas commented 6 years ago

I'd just like to add that Elastic Search supports queries in the request body, and that's the default mode used by Kibana on the ELK stack. See https://www.elastic.co/guide/en/elasticsearch/reference/6.3/search-request-body.html

There is no side effect implied by putting data in the request body.

soenkehahn commented 6 years ago

@alexanderkjeldaas: Thanks for pointing this out. http is such a mess...