fix(api) allow to delete APIs and Consumers by name

Cassandra from 3 to 2 during an update and a delete, it removed the
possibility of updating and deleting APIs and Consumers by their name,
because that change made it so the base_dao's underlying `delete()` and
`update()` were directly used instead of first querying the entity.
Querying the entity first allowed to retrieve its PRIMARY KEY fields,
and hence `delete()` and `update()` never complained. But by removing
this initial retrieving, we removed this feature, and **there were not
tests** for it!

- This adds test for PATCH/DELETE APIs and Consumers by name/username
- Adds an argument to the base_dao `update()` and `delete()` to select
  by any field rather than only the PRIMARY KEY fields.

kong/api/crud_helpers.lua

@@ -2,38 +2,39 @@ local responses = require "kong.tools.responses"
 local validations = require "kong.dao.schemas_validation"
 local app_helpers = require "lapis.application"
 local utils = require "kong.tools.utils"
+local is_uuid = validations.is_valid_uuid
 
 local _M = {}
 
 function _M.find_api_by_name_or_id(self, dao_factory, helpers)
   local fetch_keys = {
-    [validations.is_valid_uuid(self.params.name_or_id) and "id" or "name"] = self.params.name_or_id
+    [is_uuid(self.params.name_or_id) and "id" or "name"] = self.params.name_or_id
   }
   self.params.name_or_id = nil
 
-  local data, err = dao_factory.apis:find_by_keys(fetch_keys)
+  local rows, err = dao_factory.apis:find_by_keys(fetch_keys)
   if err then
     return helpers.yield_error(err)
   end
 
-  self.api = data[1]
+  self.api = rows[1]
   if not self.api then
     return helpers.responses.send_HTTP_NOT_FOUND()
   end
 end
 
 function _M.find_consumer_by_username_or_id(self, dao_factory, helpers)
   local fetch_keys = {
-    [validations.is_valid_uuid(self.params.username_or_id) and "id" or "username"] = self.params.username_or_id
+    [is_uuid(self.params.username_or_id) and "id" or "username"] = self.params.username_or_id
   }
   self.params.username_or_id = nil
 
-  local data, err = dao_factory.consumers:find_by_keys(fetch_keys)
+  local rows, err = dao_factory.consumers:find_by_keys(fetch_keys)
   if err then
     return helpers.yield_error(err)
   end
 
-  self.consumer = data[1]
+  self.consumer = rows[1]
   if not self.consumer then
     return helpers.responses.send_HTTP_NOT_FOUND()
   end
@@ -131,8 +132,8 @@ function _M.post(params, dao_collection, success)
   end
 end
 
-function _M.patch(params, dao_collection)
-  local updated_entity, err = dao_collection:update(params)
+function _M.patch(params, dao_collection, where_t)
+  local updated_entity, err = dao_collection:update(params, false, where_t)
   if err then
     return app_helpers.yield_error(err)
   elseif updated_entity == nil then
@@ -142,8 +143,8 @@ function _M.patch(params, dao_collection)
   end
 end
 
-function _M.delete(where_t, dao_collection)
-  local ok, err = dao_collection:delete(where_t)
+function _M.delete(primary_key_t, dao_collection, where_t)
+  local ok, err = dao_collection:delete(primary_key_t, where_t)
   if not ok then
     if err then
       return app_helpers.yield_error(err)

kong/api/routes/apis.lua

@@ -2,6 +2,7 @@ local crud = require "kong.api.crud_helpers"
 local syslog = require "kong.tools.syslog"
 local constants = require "kong.constants"
 local validations = require "kong.dao.schemas_validation"
+local is_uuid = validations.is_valid_uuid
 
 return {
   ["/apis/"] = {
@@ -20,24 +21,22 @@ return {
 
   ["/apis/:name_or_id"] = {
     before = function(self, dao_factory)
-      if validations.is_valid_uuid(self.params.name_or_id) then
-        self.params.id = self.params.name_or_id
-      else
-        self.params.name = self.params.name_or_id
-      end
+      self.fetch_keys = {
+        [is_uuid(self.params.name_or_id) and "id" or "name"] = self.params.name_or_id
+      }
       self.params.name_or_id = nil
     end,
 
     GET = function(self, dao_factory, helpers)
-      crud.get(self.params, dao_factory.apis)
+      crud.get(self.fetch_keys, dao_factory.apis)
     end,
 
     PATCH = function(self, dao_factory)
-      crud.patch(self.params, dao_factory.apis)
+      crud.patch(self.params, dao_factory.apis, self.fetch_keys)
     end,
 
     DELETE = function(self, dao_factory)
-      crud.delete(self.params, dao_factory.apis)
+      crud.delete(nil, dao_factory.apis, self.fetch_keys)
     end
   },
 
@@ -51,7 +50,7 @@ return {
       crud.paginated_set(self, dao_factory.plugins)
     end,
 
-    POST = function(self, dao_factory, helpers)
+    POST = function(self, dao_factory)
       crud.post(self.params, dao_factory.plugins, function(data)
         if configuration.send_anonymous_reports then
           data.signal = constants.SYSLOG.API
@@ -60,7 +59,7 @@ return {
       end)
     end,
 
-    PUT = function(self, dao_factory, helpers)
+    PUT = function(self, dao_factory)
       crud.put(self.params, dao_factory.plugins)
     end
   },

kong/api/routes/consumers.lua

@@ -1,5 +1,6 @@
 local validations = require "kong.dao.schemas_validation"
 local crud = require "kong.api.crud_helpers"
+local is_uuid = validations.is_valid_uuid
 
 return {
   ["/consumers/"] = {
@@ -18,24 +19,22 @@ return {
 
   ["/consumers/:username_or_id"] = {
     before = function(self, dao_factory)
-      if validations.is_valid_uuid(self.params.username_or_id) then
-        self.params.id = self.params.username_or_id
-      else
-        self.params.username = self.params.username_or_id
-      end
+      self.fetch_keys = {
+        [is_uuid(self.params.username_or_id) and "id" or "username"] = self.params.username_or_id
+      }
       self.params.username_or_id = nil
     end,
 
-    GET = function(self, dao_factory, helpers)
-      crud.get(self.params, dao_factory.consumers)
+    GET = function(self, dao_factory)
+      crud.get(self.fetch_keys, dao_factory.consumers)
     end,
 
-    PATCH = function(self, dao_factory, helpers)
-      crud.patch(self.params, dao_factory.consumers)
+    PATCH = function(self, dao_factory)
+      crud.patch(self.params, dao_factory.consumers, self.fetch_keys)
     end,
 
-    DELETE = function(self, dao_factory, helpers)
-      crud.delete(self.params, dao_factory.consumers)
+    DELETE = function(self, dao_factory)
+      crud.delete(nil, dao_factory.consumers, self.fetch_keys)
     end
   }
 }

kong/dao/cassandra/base_dao.lua

@@ -211,28 +211,39 @@ end
 -- Performs schema validation, 'UNIQUE' and 'FOREIGN' checks.
 -- @see check_unique_fields
 -- @see check_foreign_fields
--- @param[type=table] t A table representing the entity to update. It must contain the entity's PRIMARY KEY (can be composite).
+-- @param[type=table] t A table representing the entity to update. It should contain the entity's PRIMARY KEY fields (can be composite). If not, then a selector can be passed as argument #3.
 -- @param[type=boolean] full  If true, set to CQL `null` any column not in the `t` argument, such as a PUT query would do for example.
+-- @param[type=table] where_t A table containing the PRIMARY KEY fields of the row to update.
 -- @treturn table `result`: Updated entity or nil.
 -- @treturn table `error`: Error if any during the execution.
-function BaseDao:update(t, full)
+function BaseDao:update(t, full, where_t)
   assert(t ~= nil, "Cannot update a nil element")
   assert(type(t) == "table", "Entity to update must be a table")
 
   -- Check if the entity exists to prevent upsert and retrieve its old values
-  local entity, err = self:find_by_primary_key(t)
-  if entity == nil or err then
-    return nil, err
+  local old_entity, err
+  if where_t ~= nil then
+    old_entity, err = self:find_by_keys(where_t)
+    if not old_entity or #old_entity ~= 1 or err then
+      return nil, err
+    else
+      old_entity = old_entity[1]
+    end
+  else
+    old_entity, err = self:find_by_primary_key(t)
+    if old_entity == nil or err then
+      return nil, err
+    end
   end
 
   if not full then
-    complete_partial_entity(t, entity, self._schema)
+    complete_partial_entity(t, old_entity, self._schema)
   end
 
   -- Validate schema
   local ok, errors, err = validations.validate_entity(t, self._schema, {
     update = true,
-    old_t = entity,
+    old_t = old_entity,
     full_update = full,
     dao = self._factory
   })
@@ -257,7 +268,7 @@ function BaseDao:update(t, full)
   end
 
   -- Extract primary key from the entity
-  local t_primary_key, t_no_primary_key = extract_primary_key(t, self._primary_key, self._clustering_key)
+  local t_primary_key, t_no_primary_key = extract_primary_key(old_entity, self._primary_key, self._clustering_key)
 
   -- If full, add CQL `null` to the SET part of the query for nil columns
   if full then
@@ -356,38 +367,41 @@ end
 
 ---
 -- Delete the row with PRIMARY KEY from the configured table (**_table** attribute).
--- @param[table=table] where_t A table containing the PRIMARY KEY (columns/values) of the row to delete
+-- @param[table=table] primary_key_t A table containing the PRIMARY KEY (columns/values) of the row to delete
 -- @treturn boolean True if deleted, false if otherwise or not found.
 -- @treturn table Error if any during the query execution or the cascade delete hook.
-function BaseDao:delete(where_t)
-  assert(self._primary_key ~= nil and type(self._primary_key) == "table" , "Entity does not have a primary_key")
-  assert(where_t ~= nil and type(where_t) == "table", "where_t must be a table")
-
+function BaseDao:delete(primary_key_t, where_t)
   -- Test if exists first
-  local res, err = self:find_by_primary_key(where_t)
-  if err then
-    return false, err
-  elseif not res then
-    return false
+  local row, err
+  if where_t ~= nil then
+    local rows, err = self:find_by_keys(where_t)
+    if err or rows and #rows ~= 1 then
+      return false, err
+    end
+    row = rows[1]
+  else
+    row, err = self:find_by_primary_key(primary_key_t)
+    if row == nil or err then
+      return false, err
+    end
   end
 
-  local t_primary_key = extract_primary_key(where_t, self._primary_key, self._clustering_key)
+  local t_primary_key = extract_primary_key(row, self._primary_key, self._clustering_key)
   local delete_q, where_columns = query_builder.delete(self._table, t_primary_key)
-  local results, err = self:build_args_and_execute(delete_q, where_columns, where_t)
-  if err then
+  local ok, err = self:build_args_and_execute(delete_q, where_columns, row)
+  if not ok then
     return false, err
   end
 
   -- Delete successful, trigger cascade delete hooks if any.
-  local foreign_err
   for _, hook in ipairs(self.cascade_delete_hooks) do
-    foreign_err = select(2, hook(t_primary_key))
+    local foreign_err = select(2, hook(t_primary_key))
     if foreign_err then
       return false, foreign_err
     end
   end
 
-  return results
+  return true
 end
 
 ---

spec/integration/admin_api/apis_routes_spec.lua

@@ -195,6 +195,16 @@ describe("Admin API", function()
             assert.equal("patch-updated", body.name)
           end
         end)
+        it_content_types("should update by name", function(content_type)
+          return function()
+            local response, status = http_client.patch(BASE_URL..api.name, {
+              name = "patch-updated"
+            }, {["content-type"] = content_type})
+            assert.equal(200, status)
+            local body = json.decode(response)
+            assert.equal("patch-updated", body.name)
+          end
+        end)
         describe("errors", function()
           it_content_types("should return 404 if not found", function(content_type)
             return function()
@@ -218,11 +228,24 @@ describe("Admin API", function()
       end)
 
       describe("DELETE", function()
-        it("delete an API", function()
+        before_each(function()
+          local _, err = dao_factory.apis:insert {
+            name = "to-delete",
+            request_host = "to-delete.com",
+            upstream_url = "http://mockbin.com"
+          }
+          assert.falsy(err)
+        end)
+        it("delete an API by id", function()
           local response, status = http_client.delete(BASE_URL..api.id)
           assert.equal(204, status)
           assert.falsy(response)
         end)
+        it("delete an API by name #only", function()
+          local response, status = http_client.delete(BASE_URL.."to-delete")
+          assert.equal(204, status)
+          assert.falsy(response)
+        end)
         describe("error", function()
           it("should return HTTP 404 if not found", function()
             local _, status = http_client.delete(BASE_URL.."hello")

spec/integration/admin_api/consumers_routes_spec.lua

@@ -202,6 +202,16 @@ describe("Admin API", function()
             assert.equal("patch-updated", body.username)
           end
         end)
+        it_content_types("should update by username", function(content_type)
+          return function()
+            local response, status = http_client.patch(BASE_URL..consumer.username, {
+              username = "patch-updated"
+            }, {["content-type"] = content_type})
+            assert.equal(200, status)
+            local body = json.decode(response)
+            assert.equal("patch-updated", body.username)
+          end
+        end)
         describe("errors", function()
           it_content_types("should return 404 if not found", function(content_type)
             return function()
@@ -229,11 +239,23 @@ describe("Admin API", function()
       end)
 
       describe("DELETE", function()
-        it("delete a Consumer", function()
+        local dao_factory = spec_helper.get_env().dao_factory
+        setup(function()
+          local _, err = dao_factory.consumers:insert {
+            username = "to-delete"
+          }
+          assert.falsy(err)
+        end)
+        it("delete a Consumer by id", function()
           local response, status = http_client.delete(BASE_URL..consumer.id)
           assert.equal(204, status)
           assert.falsy(response)
         end)
+        it("delete a Consumer by name", function()
+          local response, status = http_client.delete(BASE_URL..consumer.username)
+          assert.equal(204, status)
+          assert.falsy(response)
+        end)
         describe("error", function()
           it("should return HTTP 404 if not found", function()
             local _, status = http_client.delete(BASE_URL.."hello")