Skip to content

Untraceable error with Swagger UI for GET method that requires a body. #251

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

Closed
code-ape opened this issue Oct 3, 2016 · 7 comments
Closed

Comments

@code-ape
Copy link

code-ape commented Oct 3, 2016

Verified for:

  • [org.clojure/clojure "1.8.0"]
  • [compojure "1.5.1"]
  • [ring/ring-defaults "0.2.1"]
  • [metosin/compojure-api "1.1.8"] and [metosin/compojure-api "1.1.7"]
  • Running with lein ring server-headless

Issue

A method declared as GET which only takes a body fails, such as this one:

(GET "/foo" []
     :return {:foo s/Str}
     :body [f {:foo s/Str}]
     :summary "An example"
     (ok f))

fails without a traceable explanation on the server console output. The Swagger UI only says:

response Body
{
  "errors": "(not (map? nil))"
}

Response Code
400

Response Headers
{
  "date": "Mon, 03 Oct 2016 02:38:57 GMT",
  "server": "Jetty(7.6.13.v20130916)",
  "content-length": "29",
  "content-type": "application/json; charset=utf-8"
}

This appears to be related to the GET method as changing this to a POST fixes this problem.

Also, this issue doesn't occur when using curl, as the following succeeds with the appropriate response:

$ curl -X GET --header 'Content-Type: application/json' \ 
  --header 'Accept: application/json' -d '{ "foo": "string" }' \
  'http://192.168.50.50:3000/api/v1/foo'

Regardless though the failure from the Swagger UI still indicates an issue with the library and, with no other debug text, left me to twiddle around with all other aspects of my application for a few hours before finding out what I have reported here.

Let me know if any further information is needed!

Cheers,
codeape

@code-ape
Copy link
Author

code-ape commented Oct 3, 2016

Upon closer inspection this appears to be due to the fact that, for GET calls from the Swagger UI, the body is not actually sent in the request to the server. Which is why the error referring to something (presumably the body of the request) is nil.

@ricardojmendez
Copy link

This tripped me last night with compojure-api 1.1.8. I was using Swagger as the API prototype test and couldn't figure why I was getting the error. Works from curl/httpie.

@Deraen
Copy link
Member

Deraen commented Oct 3, 2016

Bug or "feature" is in Swagger UI, and they won't change this: swagger-api/swagger-ui#2136

Reason is that message-body on GET request is not supposed to have any effect on the response, so trying to use it is not a good idea:

A server SHOULD read and forward a message-body on any request; if the request method
does not include defined semantics for an entity-body, then the
message-body SHOULD be ignored when handling the request.

https://tools.ietf.org/html/rfc2616#section-4.3

The GET method means retrieve whatever information is identified by the Request-URI.

https://tools.ietf.org/html/rfc2616#section-9.3

@code-ape
Copy link
Author

code-ape commented Oct 5, 2016

@Deraen thank for you the explanation on this! Sorry to raise the issue here, I should have thought to check on Swagger's issues for this before posting.

As a passing thought, it could perhaps be beneficial to have some debug text on startup of compojure-api so that if there are GET's with body parameters so that users could be aware of this behavior.

Regardless I appreciate the explanation and have made the appropriate changes in my project.
Cheers!

@Deraen
Copy link
Member

Deraen commented Oct 5, 2016

Yeah, I agree that a warning should be a good idea.

@jconti
Copy link

jconti commented Mar 16, 2017

Actually while this no-body on GET behavior is recommended in the RFC for HTTP (I think), it is not universal, and I think a bit short sighted. Elastisearch uses the body on get request to good effect. They point out that REST gives poor granularity in selecting resources (individual item, or collection) and query params are space limited, poorly structured, and inherently part of the URL (sometimes intended and sometimes not).

By allowing a GET request to send a query as a body, a much more regular language is possible for specifying views over collections of resources. Many new technologies are heading this way too, graph-ql, falcor, etc. Clients, especially mobile clients need to be able to send a lot of info as part of get requests, and often the URL is not part of it, as it is not the data they want in a users bookmark. While there is a separate verb for this function (SEARCH) it is very hard to find implementations that service this correctly as a GET with a body.

The real issue here is the behavior of caching proxies. They do not generally allow this as an optimization. But I do not think API design should be limited by the implementation details of caching proxies. I will update the linked swagger case with my thoughts in case that has any effect.

@ikitommi
Copy link
Member

I believe compojure-api works correctly here as it allows bodys with GET. Not having a body and expecting a map gives a error than can be understood. Added note to add examples of this into the new documentation site (#329). Closing this. Please reopen is something was left undone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants