feat(dao) ngx.null — allows patch requests to remove opt. conf. params #2700

bungle · 2017-07-13T23:13:30Z

Summary

Better handling of JSON null in DAO layer and validations.

Also allows usage of JSON null to remove optional parameters using PATCH request, e.g. with HTTPie:

http patch :8001/apis/test hosts:=null uris:=null

p0pr0ck5 · 2017-07-14T18:36:06Z

Tests?

bungle · 2017-07-14T20:23:15Z

Yes, tests are missing, I just wanted first know (as I did this adhoc), if this makes any sense. If we agree that this is a good addition, I will add tests as well.

bungle · 2017-07-17T14:26:33Z

@p0pr0ck5 I added a couple of tests.

thibaultcha

Cool stuff! Un-setting fields via PUT/PATCH requests has been a long time flaw in this API...

Because of the legacy aspect of this part of the code base, and because of the associated tech-debt with it (many parts of the code rely on the schema validator to be working as it has been for the past ~2 years), I think we need to cover this with a few more tests, in order to make sure we don't break anything (that appears to be the case already) and are not introducing a broken behavior either. I left a few notes, but also a few comments of things we need to address I think, mainly related to tests:

fix the uris unset unit test to make it more atomic and not rely on the result of the previous test
add a test for default attribute and ngx.null
add a test for default attribute and ngx.null with sub-schemas
add a test for the custom func validator to be properly called with nil and not t[column]
a test for fields with the required attribute to error out when given ngx.null in both partial and full updates

thibaultcha · 2017-07-22T01:11:42Z

kong/dao/schemas_validation.lua

@@ -83,6 +83,9 @@ function _M.validate_entity(tbl, schema, options)

      if not partial_update then
        for column, v in pairs(schema.fields) do
+          if t[column] == ngx.null then
+            t[column] = nil
+          end


Note to self: so if we do a full update (PUT), and we set a field with a "default" attribute to NULL, we actually pull a new value from the configured "default" attribute (text or function). The behavior is fine (I think?), as most fields with a "default" values are so because they are not supposed to be empty. But it is still a somewhat unexpected behavior I think. Nothing else to add, just dropping a note here...

thibaultcha · 2017-07-22T01:23:43Z

kong/dao/schemas_validation.lua

@@ -185,7 +195,7 @@ function _M.validate_entity(tbl, schema, options)
              end
            end

-            if t[column] and type(t[column]) == "table" then
+            if t[column] and t[column] ~= ngx.null and type(t[column]) == "table" then


Since type(ngx.null) == "userdata", it seems we can stick to if t[column] and type(t[column]) == "table" then here? Explicitness is good though. Maybe a comment saying ngx.null will also get skipped?

I'm fine with both. Do we want to leave room open for mocking ngx.null = {}?

I don't think so, ngx.null should be ngx.null and nothing else imho

It doesn't hurt for such a bloated piece of code to stay explicit after all... Leaving this as-is :)

thibaultcha · 2017-07-22T01:29:47Z

kong/dao/schemas_validation.lua

      for column, v in pairs(schema.fields) do
+        if not partial_update then
+          if t[column] == ngx.null then
+            t[column] = nil


ditto: it seems the behavior we envision here is to treat fields set with NULL during a PUT as if they were initially created (run validation as if they were left blank, which means generates defaults and defaults for their sub-schema).

Could we have a couple of tests for this behavior?

A test asserting that during a full update, fields set to ngx.null are reset to their default

A test asserting that during a full update, fields with a sub-schema are reset to their default

You can create a fixture schema for this (the schema validation tests do it), in order to avoid depending on schemas from plugins or core entities :)

thibaultcha · 2017-07-22T01:39:43Z

spec/02-integration/03-dao/03-crud_spec.lua

@@ -604,6 +634,20 @@ helpers.for_each_dao(function(kong_config)
          assert.falsy(err)
          assert.same(api_fixture, api)
        end)
+        it("unset ngx.null fields", function()


Because the previous test unsets uris already, we don't know if this test succeeds because of the newly introduced code or because the previous test is already passing. Could we rework the logic a bit here? A suggestion would be to introduce the following in the previous test:

it("unset nil fields", function() finally(function() -- re add the `uris` field api_fixture.uris = "/mockbin" assert(api:update(api_fixture, api_fixture, { full = true})) end) -- rest of the test end) it("unset ngx.null fields", function() -- ... end)

thibaultcha · 2017-07-22T01:41:39Z

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

@@ -563,6 +563,31 @@ describe("Admin API " .. kong_config.database, function()
            assert.same(json, in_db)
          end
        end)
+        it_content_types("removes optional field with ngx.null", function(content_type)
+          return function()
+            -- TODO: how should ngx.null work with application/www-form-urlencoded?


It is probably out of the scope of this PR, which is just to make the Admin API/DAO accept the JSON null type (which of course does not exist in application/www-form-urlencoded), so nothing to worry about imo :)

thibaultcha · 2017-07-22T01:44:34Z

kong/dao/schemas_validation.lua

-            local ok, err, new_fields = v.func(t[column], t, column)
+            local ok, err, new_fields
+            if t[column] == ngx.null then
+              ok, err, new_fields = v.func(nil, t, column)


I think this should be tested too

Thanks to this change, we can now remove fields (if permitted by the entity schema) by passing a JSON NULL (`ngx.null` or `cjson.null`) to said field. A field updated with NULL during a PATCH (partial update) will retain its `ngx.null` value after validation, and the DAO must infer this value to the correct NULL representation of its interface (CQL/SQL). During a PUT (full update), fields with a JSON NULL will be treated as if they were not specified (like create). No such behavior is available when sending `application/x-www-form-urlencoded` MIME type requests, this is only to support `cjson.null`/`ngx.null`. Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha

Added all of the required tests in an edit to your commit!

bungle added the pr/status/please review label Jul 13, 2017

bungle added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jul 14, 2017

bungle force-pushed the feat/dao-patch-null branch from 8abbabc to 8a4a3f2 Compare July 17, 2017 14:26

bungle added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jul 17, 2017

thibaultcha requested changes Jul 22, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jul 22, 2017

bungle mentioned this pull request Jul 22, 2017

fix(plugins) consumer_id may be ngx.null when configuring the plugin #2724

Closed

thibaultcha force-pushed the feat/dao-patch-null branch from 8a4a3f2 to 0f0e432 Compare July 25, 2017 21:24

thibaultcha approved these changes Jul 25, 2017

View reviewed changes

thibaultcha merged commit fc82dd7 into next Jul 25, 2017

thibaultcha deleted the feat/dao-patch-null branch July 25, 2017 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dao) ngx.null — allows patch requests to remove opt. conf. params #2700

feat(dao) ngx.null — allows patch requests to remove opt. conf. params #2700

bungle commented Jul 13, 2017 •

edited

Loading

p0pr0ck5 commented Jul 14, 2017

bungle commented Jul 14, 2017 •

edited

Loading

bungle commented Jul 17, 2017 •

edited

Loading

thibaultcha left a comment

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

bungle Jul 22, 2017

thibaultcha Jul 24, 2017

thibaultcha Jul 25, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha left a comment

feat(dao) ngx.null — allows patch requests to remove opt. conf. params #2700

feat(dao) ngx.null — allows patch requests to remove opt. conf. params #2700

Conversation

bungle commented Jul 13, 2017 • edited Loading

Summary

p0pr0ck5 commented Jul 14, 2017

bungle commented Jul 14, 2017 • edited Loading

bungle commented Jul 17, 2017 • edited Loading

thibaultcha left a 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 left a comment

Choose a reason for hiding this comment

bungle commented Jul 13, 2017 •

edited

Loading

bungle commented Jul 14, 2017 •

edited

Loading

bungle commented Jul 17, 2017 •

edited

Loading