feat(dao) allows JSON NULL in CRUD operations (#2700)

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 <[email protected]>

kong/dao/dao.lua

@@ -264,6 +264,8 @@ local function fix(old, new, schema)
       for f_k in pairs(f_schema.fields) do
         if new[col][f_k] == nil and old[col][f_k] ~= nil then
           new[col][f_k] = old[col][f_k]
+        elseif new[col][f_k] == ngx.null then
+          new[col][f_k] = nil
         end
       end
 

kong/dao/db/cassandra.lua

@@ -24,8 +24,8 @@ _M.dao_insert_values = {
     return uuid()
   end,
   timestamp = function()
-    -- return time in UNIT millisecond, and PRECISION millisecond 
-    return math.floor(timestamp.get_utc_ms()) 
+    -- return time in UNIT millisecond, and PRECISION millisecond
+    return math.floor(timestamp.get_utc_ms())
   end
 }
 
@@ -282,7 +282,7 @@ end
 -- @section query_building
 
 local function serialize_arg(field, value)
-  if value == nil then
+  if value == nil or value == ngx.null then
     return cassandra.null
   elseif field.type == "id" then
     return cassandra.uuid(value)

kong/dao/db/postgres.lua

@@ -205,6 +205,10 @@ end
 
 -- @see pgmoon
 local function escape_literal(val, field)
+  if val == ngx.null then
+    return "NULL"
+  end
+
   local t_val = type(val)
   if t_val == "number" then
     return tostring(val)

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
           -- [DEFAULT] Set default value for the field if given
           if t[column] == nil and v.default ~= nil then
             if type(v.default) == "function" then
@@ -95,9 +98,16 @@ function _M.validate_entity(tbl, schema, options)
       end
 
       -- Check the given table against a given schema
+
       for column, v in pairs(schema.fields) do
+        if not partial_update then
+          if t[column] == ngx.null then
+            t[column] = nil
+          end
+        end
+
         -- [TYPE] Check if type is valid. Booleans and Numbers as strings are accepted and converted
-        if t[column] ~= nil and v.type ~= nil then
+        if t[column] ~= nil and t[column] ~= ngx.null and v.type ~= nil then
           local is_valid_type
           -- ALIASES: number, timestamp, boolean and array can be passed as strings and will be converted
           if type(t[column]) == "string" then
@@ -134,7 +144,7 @@ function _M.validate_entity(tbl, schema, options)
         end
 
         -- [ENUM] Check if the value is allowed in the enum.
-        if t[column] ~= nil and v.enum ~= nil then
+        if t[column] ~= nil and t[column] ~= ngx.null and v.enum ~= nil then
           local found = true
           local wrong_value = t[column]
           if v.type == "array" then
@@ -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
               -- Actually validating the sub-schema
               local s_ok, s_errors, s_self_check_err = _M.validate_entity(t[column], sub_schema, options)
               if not s_ok then
@@ -209,14 +219,24 @@ function _M.validate_entity(tbl, schema, options)
         if not partial_update or t[column] ~= nil then
           -- [REQUIRED] Check that required fields are set.
           -- Now that default and most other checks have been run.
-          if v.required and not v.dao_insert_value and (t[column] == nil or t[column] == "") then
+          if not options.full_update and v.required and not v.dao_insert_value and (t[column] == nil or t[column] == "" or t[column] == ngx.null) then
             errors = utils.add_error(errors, error_prefix .. column, column .. " is required")
           end
 
-          if type(v.func) == "function" and (errors == nil or errors[column] == nil) then
+          local callable = type(v.func) == "function"
+          if not callable then
+            local mt = getmetatable(v.func)
+            callable = mt and mt.__call ~= nil
+          end
+          if callable and (errors == nil or errors[column] == nil) then
             -- [FUNC] Check field against a custom function
             -- only if there is no error on that field already.
-            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)
+            else
+              ok, err, new_fields = v.func(t[column], t, column)
+            end
             if ok == false and err then
               errors = utils.add_error(errors, error_prefix .. column, err)
             elseif new_fields then

spec/01-unit/006-schema_validation_spec.lua

@@ -55,6 +55,13 @@ describe("Schemas", function()
         assert.truthy(err)
         assert.are.same("string is required", err.string)
       end)
+      it("errors if required property is set to ngx.null", function()
+        local values = { string = ngx.null }
+
+        local ok, err = validate_entity(values, schema)
+        assert.falsy(ok)
+        assert.equal("string is required", err.string)
+      end)
     end)
 
     describe("[type]", function()
@@ -325,6 +332,14 @@ describe("Schemas", function()
         assert.are.same("abcdef", values.default)
       end)
 
+      it("sets to default when a field is given ngx.null", function()
+        local values = { string = "foo", default = ngx.null }
+
+        local ok, err = validate_entity(values, schema)
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.equal("default", values.default)
+      end)
     end)
 
     describe("[regex]", function()
@@ -421,6 +436,17 @@ describe("Schemas", function()
         assert.truthy(err)
         assert.are.same("Nah", err.custom)
       end)
+      it("is called with arg1 'nil' when given ngx.null", function()
+        spy.on(schema.fields.custom, "func")
+
+        local values = { string = "foo", custom = ngx.null }
+
+        local ok, err = validate_entity(values, schema)
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.is_nil(values.custom)
+        assert.spy(schema.fields.custom.func).was_called_with(nil, values, "custom")
+      end)
     end)
 
     it("should return error when unexpected values are included in the schema", function()
@@ -753,6 +779,38 @@ describe("Schemas", function()
         assert.are.same("asd is an unknown field", err["flexi.somekey2.asd"])
       end)
 
+      it("errors if required sub-schema is given ngx.null", function()
+        local values = { some_required = "foo", sub_schema = ngx.null }
+
+        local ok, err = validate_entity(values, nested_schema)
+        assert.falsy(ok)
+        assert.same({
+          ["sub_schema"] = "sub_schema.sub_field_required is required",
+          ["sub_schema.sub_field_required"] = "sub_field_required is required",
+          ["sub_schema.sub_sub_schema"] = "sub_sub_schema.sub_sub_field_required is required",
+        }, err)
+      end)
+
+      it("gives NULL to sub-schema if given ngx.null in update", function()
+        local values = { some_required = "foo", sub_schema = ngx.null }
+
+        local ok, err = validate_entity(values, nested_schema, { update = true })
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.equal(ngx.null, values.sub_schema)
+      end)
+
+      it("errors if required sub-schema is given ngx.null in a full update", function()
+        local values = { some_required = "foo", sub_schema = ngx.null }
+
+        local ok, err = validate_entity(values, nested_schema, { update = true, full_update = true })
+        assert.falsy(ok)
+        assert.same({
+          ["sub_schema"] = "sub_schema.sub_field_required is required",
+          ["sub_schema.sub_field_required"] = "sub_field_required is required",
+          ["sub_schema.sub_sub_schema"] = "sub_sub_schema.sub_sub_field_required is required",
+        }, err)
+      end)
     end)
 
     describe("[update] (partial)", function()
@@ -796,6 +854,37 @@ describe("Schemas", function()
         assert.truthy(err)
         assert.equal("date cannot be updated", err.date)
       end)
+
+      it("passes NULL if a field with default is given ngx.null", function()
+        local values = { string = "foo", date = ngx.null }
+
+        local ok, err = validate_entity(values, schema, { update = true })
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.equal(ngx.null, values.date) -- DAO will handle ngx.null to 'NULL'
+      end)
+
+      it("calls 'func' with arg1 'nil' when given ngx.null", function()
+        spy.on(schema.fields.custom, "func")
+
+        local values = { string = "foo", custom = ngx.null }
+
+        local ok, err = validate_entity(values, schema, { update = true })
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.equal(ngx.null, values.custom)
+        assert.spy(schema.fields.custom.func).was_called_with(nil, values, "custom")
+      end)
+
+      it("errors when a required field is given ngx.null", function()
+        spy.on(schema.fields.custom, "func")
+
+        local values = { string = ngx.null }
+
+        local ok, err = validate_entity(values, schema, { update = true })
+        assert.falsy(ok)
+        assert.equal("string is required", err.string)
+      end)
     end)
 
     describe("[update] (full)", function()
@@ -805,7 +894,7 @@ describe("Schemas", function()
         local valid, err = validate_entity(values, schema, {update = true, full_update = true})
         assert.False(valid)
         assert.truthy(err)
-        assert.are.same({"string is required", "string is required"}, err.string)
+        assert.equal("string is required", err.string)
       end)
       it("should complete default fields", function()
         local values = {string = "foo", date = 123456}
@@ -815,6 +904,34 @@ describe("Schemas", function()
         assert.falsy(err)
         assert.equal("default", values.default)
       end)
+      it("sets a field to its default if given ngx.null", function()
+        local values = { string = "foo", date = ngx.null }
+
+        local ok, err = validate_entity(values, schema, {update = true, full_update = true})
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.is_number(values.date)
+      end)
+      it("calls 'func' with arg1 'nil' when given ngx.null", function()
+        spy.on(schema.fields.custom, "func")
+
+        local values = { string = "foo", custom = ngx.null }
+
+        local ok, err = validate_entity(values, schema, { update = true, full_update = true })
+        assert.falsy(err)
+        assert.is_true(ok)
+        assert.is_nil(values.custom)
+        assert.spy(schema.fields.custom.func).was_called_with(nil, values, "custom")
+      end)
+      it("errors when a required field is given ngx.null", function()
+        spy.on(schema.fields.custom, "func")
+
+        local values = { string = ngx.null }
+
+        local ok, err = validate_entity(values, schema, { update = true, full_update = true })
+        assert.falsy(ok)
+        assert.equal("string is required", err.string)
+      end)
     end)
   end)
 end)

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

@@ -119,6 +119,21 @@ helpers.for_each_dao(function(kong_config)
         assert.True(err.unique)
         assert.equal("already exists with value 'mockbin'", err.tbl.name)
       end)
+      it("ignores ngx.null fields", function()
+        local api, err = apis:insert {
+          name = "httpbin",
+          hosts = { "httpbin.org" },
+          upstream_url = "http://httpbin.org",
+          uris = ngx.null,
+          methods = ngx.null,
+        }
+        assert.falsy(err)
+        assert.is_table(api)
+        assert.equal("httpbin", api.name)
+        assert.is_nil(api.uris)
+        assert.is_nil(api.methods)
+        assert.raw_table(api)
+      end)
 
       describe("errors", function()
         it("refuse if invalid schema", function()
@@ -575,6 +590,21 @@ helpers.for_each_dao(function(kong_config)
         assert.not_same(api_fixture, api)
         assert.truthy(api.uris)
       end)
+      it("does unset ngx.null fields", function()
+        api_fixture.uris = ngx.null
+
+        local api, err = apis:update(api_fixture, {id = api_fixture.id})
+        assert.falsy(err)
+        assert.truthy(api)
+        assert.not_same(api_fixture, api)
+        assert.falsy(api.uris)
+        assert.raw_table(api)
+
+        api, err = apis:find(api_fixture)
+        assert.falsy(err)
+        assert.not_same(api_fixture, api)
+        assert.falsy(api.uris)
+      end)
 
       describe("full", function()
         it("update with nil fetch_keys", function()
@@ -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()
+          api_fixture.uris = ngx.null
+
+          local api, err = apis:update(api_fixture, api_fixture, {full = true})
+          api_fixture.uris = nil
+          assert.falsy(err)
+          assert.truthy(api)
+          assert.same(api_fixture, api)
+          assert.raw_table(api)
+
+          api, err = apis:find(api_fixture)
+          assert.falsy(err)
+          assert.same(api_fixture, api)
+        end)
         it("check schema", function()
           api_fixture.name = nil
 

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?
+            if content_type == "application/json" then
+              local res = assert(client:send {
+                method = "PATCH",
+                path = "/apis/" .. api.id,
+                body = {
+                  uris = ngx.null,
+                  hosts = ngx.null,
+                },
+                headers = {["Content-Type"] = content_type}
+              })
+              local body = assert.res_status(200, res)
+              local json = cjson.decode(body)
+              assert.is_nil(json.uris)
+              assert.is_nil(json.hosts)
+              assert.equal(api.id, json.id)
+
+              local in_db = assert(dao.apis:find {id = api.id})
+              assert.same(json, in_db)
+            end
+          end
+        end)
+
         describe("errors", function()
           it_content_types("returns 404 if not found", function(content_type)
             return function()