Disallow ReqBody combinators for GET API endpoints

[tdietert]

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]

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]

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]

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]

What about only making the check for servant-server?

[soenkehahn]

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]

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]

@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]

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

[alexanderkjeldaas]

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]

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