devicehive / devicehive-java-server

DeviceHive Java Server
http://www.devicehive.com
Apache License 2.0
371 stars 139 forks source link

DH returns 415 when no Content-Type header defined #90

Closed sorjef closed 8 years ago

sorjef commented 9 years ago

Please make Content-Type header non-required and use JSON request parser by default. Right now if you supply a valid JSON and no Content-Type the API will return HTTP 415.

This seems to be redundant on this stage jsut because JSON media type is used everywhere and there is no alternative media types implemented in DH. This will also help users who like accessing DeviceHive API directly (using curl or Postman or whatever)

This issue (or you may call this a feature :smiley:) is reproducible at least for Network POST method.

Error example:

{
  "error": 415,
  "message": "HTTP 415 Unsupported Media Type"
}
mdolbin commented 9 years ago

I think we should not make Content-Type header optional for the number of reasons.

First of all it's not about having a some header string in a request. We need to explicitly check that sender wants to send exactly JSON string before we even try to parse it.

For example, default Content-Type for POST request is application/x-www-form-urlencoded which is used to send serialized HTML forms data. This means that the following code will produce the following payload: command=command&parameters[prop1]=val1&parameters[prop2]=val2&parameters[prop3]=etc which will cause JSON parsing error (assuming we are accepting all requests) on server-side and unexpected 500 Internal Server Error instead of code 415 with user-friendly error message:

$.ajax({
    type: "POST",
    url: "http://127.0.0.1:8080/dh/rest/device/e50d6085-2aba-48e9-b1c3-73c673e414be/command",
    headers: {"Authorization": "Bearer 1jwKgLYi/CdfBTI9KByfYxwyQ6HUIEfnGSgakdpFjgk="},
    data: {
        "command": "command",
        "parameters": {
            "prop1": "val1",
            "prop2": "val2",
            "prop3": "etc"
        }
    }
});

Sticking to standards also makes it easier for us to integrate with new tools, libraries and applications.

On the other hand, it's a good idea to transfer less bandwidth. For example, we can return 415 code on parsing error.

@demon-xxi @zubrabubra @SorJEF What do you think? Let's discuss and make some decision regarding this issue.

zubrabubra commented 9 years ago

I would go with defaulting content type to application json and then making it the only choice for now. What are the reasons device would decide to send us data form encoded?

As from device perspective both json and form encoded are not native, while json accepted by human beings easily, and when someone looks on command=command&parameters[prop1]=val1&parameters[prop2]=val2&parameters[prop3]=etc he become disappointed with a fact that he needs to catch symbols in this spagetti :)

mdolbin commented 9 years ago

@zubrabubra What are the reasons device would decide to send us data form encoded? I was talking about

This will also help users who like accessing DeviceHive API directly (using curl or Postman or whatever) Majority of tools will use default Content-Type if it's not omitted.

But yeah, from the device perspective it makes perfect sense to avoid sending redundant data.

YermakovSergiy commented 9 years ago

Checked situation with curl and with Postman (from chrome). Check in debug mode, showed that curl send header Content-Type: application/x-www-form-urlencoded and Postman send header Content-Type: text/plain;charset=UTF-8. I assume that the client generates a default Content-Type. And it can cause problems. Therefore, the header must be specified certainly. I propose to close issue.

zubrabubra commented 9 years ago

@YermakovSergiy i think this is specific behaviour only of curl, postman etc, and it is not available in low level http libraries. So it's better to go with Content-type: application/json or text/json if no header specified.

YermakovSergiy commented 9 years ago

Added filter that checks request headers. If there is no Content-Type header then will be added Content-Type : application/json. Can be cheked at branch: https://github.com/devicehive/devicehive-java-server/tree/defect/90