fix(admin) unsupported media type (415) error reported on issue #1182 #3077

bungle · 2017-12-07T21:00:32Z

Summary

Fixes issue when using empty body with supported content-types on POST / PUT / PATCH.

Issues resolved

Fix #1182

bungle · 2017-12-07T21:54:49Z

One note:

when using application/json content type
empty body

This results:

{
    "message": "Cannot parse JSON body"
}

Not sure though, should we allow this to go through as we do with multipart/form-data and application/x-www-form-urlencoded (they don't error on parsing errors).

bungle · 2017-12-07T22:04:17Z

This also contains (apart from added tests) fixes to 02-apis_routes_spec.lua which was bit strangely organized and was also flaky on my machine. The admin client is not happy to be reused on errors (it sometimes closes itself).

thibaultcha · 2017-12-08T05:32:58Z

kong/api/init.lua

-    return responses.send_HTTP_UNSUPPORTED_MEDIA_TYPE()
+  if not NEEDS_BODY[ngx.req.get_method()] then
+    return
+  end


thibaultcha · 2017-12-08T05:38:39Z

kong/api/init.lua

+
+    if not content_length then
+      local _, err = ngx.req.socket()
+      if err == "no body" then


It seems to me like the check done by ngx_lua to return this error is the one we are already making above, L.100? See: https://github.com/openresty/lua-nginx-module/blob/master/src/ngx_http_lua_socket_tcp.c#L4371-L4375

Maybe we don't need this check?

I think the first one checks client header and the other checks if there is no body even when the client didn't add content-length header. I may be wrong and have to recheck this. I assume client can set that header to say 1000, but still doesn't send body.

I rechecked this and test is needed.

Hmmm, would you care to elaborate why? It seems to me that in our current code path (that is, not calling ngx.req.init_body(), not calling ngx_http_lua_create_fake_request, and not making a subrequest), the only place where content_length_n is set is: https://github.com/openresty/lua-nginx-module/blob/b122ed7ad2a6bf6f90d8d5c5939c0dd98a4ddbf7/src/ngx_http_lua_headers_in.c#L570 - in which case, the value is taken from the Content-Length header already. So when ngx.req.socket() returns "no body", it means it has did the same check as the one above, L.100 (actually, content_length_n <= 0).

Am I missing something?

This and the /apis endpoint concern are the last "blockers" for me. But feel free to dismiss if I just said non-sense above ^

Apart from those 2 points, this is GTM 👍

thibaultcha · 2017-12-08T05:57:01Z

spec/02-integration/04-admin_api/02-apis_routes_spec.lua

@@ -87,6 +88,54 @@ describe("Admin API #" .. kong_config.database, function()
      end)

      describe("errors", function()
+        it("returns 415 on invalid content-type", function()


Should we maybe not include those tests as part of the /apis route test suite, considering our recent work on the model and the uncertain future of this test suite?

Ping here as well - I would say this is concerning considering this endpoint (or its underlying implementation) may disappear soon?

thibaultcha · 2017-12-08T06:15:04Z

spec/02-integration/04-admin_api/02-apis_routes_spec.lua

@@ -259,6 +315,55 @@ describe("Admin API #" .. kong_config.database, function()
        end
      end)
      describe("errors", function()
+        it("returns 415 on invalid content-type", function()
+          local res = assert(client:request {
+            method = "POST",


Are those tests duplicates of the above ones? Maybe the intent was to re-run them with PUT?

Yes, a copy-paste-bug. Pushed a new version.

thibaultcha · 2017-12-08T06:18:25Z

spec/02-integration/04-admin_api/02-apis_routes_spec.lua

@@ -206,7 +260,7 @@ describe("Admin API #" .. kong_config.database, function()
        --
        -- Eventually, our Admin endpoint will follow a more appropriate
        -- behavior for PUT.
-        local res = assert(client:send {
+        local res = assert(helpers.admin_client():send {


Curious: why not using client here?

Because the test generates 404 and it makes the client really flaky as you see it runs this it_content_types loop, so first time it works, but subsequently you start getting:

spec/02-integration/04-admin_api/02-apis_routes_spec.lua:263: closed

The client is really problematic, because you cannot really call :send multiple times without getting into issues. That's why. To make tests pass reliably.

Ohh I see - hmmm

thibaultcha · 2017-12-08T06:20:26Z

spec/02-integration/04-admin_api/02-apis_routes_spec.lua

+            path = "/apis",
+            headers = {["Content-Type"] = "application/json"}
+          })
+          assert.res_status(400, res)


When we are returning a 400 error here, I think a second assertion for the body should be added for explicitness, ensuring that it is a schema error from our DAO (and not a Lapis error or some other sort of error).

thibaultcha · 2017-12-08T06:24:24Z

Not sure though, should we allow this to go through as we do with multipart/form-data and application/x-www-form-urlencoded (they don't error on parsing errors).

Good point. I would tend to say this behavior (reporting an invalid JSON payload) is fine. After all, a client would encounter the same error if they attempted to send an invalid JSON payload ({"hello":"world"), and it seems logical that the same error happen in both cases I think.

bungle force-pushed the fix/unsupported-media-type branch 4 times, most recently from 0f0c0ae to 8c51dda Compare December 7, 2017 21:27

bungle added the pr/status/please review label Dec 7, 2017

bungle force-pushed the fix/unsupported-media-type branch from 8c51dda to 1268c11 Compare December 7, 2017 22:02

bungle force-pushed the fix/unsupported-media-type branch from 1268c11 to 59d315a Compare December 7, 2017 22:30

thibaultcha reviewed Dec 8, 2017

View reviewed changes

thibaultcha added this to the 0.12.0 milestone Dec 8, 2017

bungle force-pushed the fix/unsupported-media-type branch 3 times, most recently from c4fb270 to 74e96f9 Compare December 14, 2017 18:38

fix(admin) unsupported media type (415) error reported on issue #1182

8411fab

bungle force-pushed the fix/unsupported-media-type branch from 74e96f9 to 8411fab Compare December 14, 2017 21:52

bungle merged commit 503dcc6 into next Dec 14, 2017

thibaultcha mentioned this pull request Dec 15, 2017

key-auth plugin error when creating new keys - "unsupported media type" - change of behavior #1182

Closed

thibaultcha deleted the fix/unsupported-media-type branch February 23, 2018 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(admin) unsupported media type (415) error reported on issue #1182 #3077

fix(admin) unsupported media type (415) error reported on issue #1182 #3077

bungle commented Dec 7, 2017

bungle commented Dec 7, 2017

bungle commented Dec 7, 2017 •

edited

Loading

thibaultcha Dec 8, 2017

thibaultcha Dec 8, 2017

bungle Dec 8, 2017

bungle Dec 12, 2017

thibaultcha Dec 12, 2017

thibaultcha Dec 13, 2017

thibaultcha Dec 8, 2017

thibaultcha Dec 13, 2017

thibaultcha Dec 8, 2017

bungle Dec 12, 2017

thibaultcha Dec 8, 2017

bungle Dec 12, 2017

thibaultcha Dec 12, 2017

thibaultcha Dec 8, 2017

thibaultcha commented Dec 8, 2017

fix(admin) unsupported media type (415) error reported on issue #1182 #3077

fix(admin) unsupported media type (415) error reported on issue #1182 #3077

Conversation

bungle commented Dec 7, 2017

Summary

Issues resolved

bungle commented Dec 7, 2017

bungle commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Dec 8, 2017

bungle commented Dec 7, 2017 •

edited

Loading