fix(plugins) add unique constraint to id field

The compound primary key, coupled with a lack of unique constraint
on id, allows for duplicate plugin UUIDs, breaking expected behavior.
This commit adds a unique constraint in the Kong schema, and directly
to the Postgres table definition via a migration.

kong/dao/migrations/postgres.lua

@@ -401,4 +401,54 @@ return {
     end,
     down = function(_, _, dao) end
   },
+  {
+    name = "2017-04-18-153000_unique_plugins_id",
+    up = function(_, _, dao)
+      local duplicates, err = dao.db:query([[
+        SELECT plugins.*
+        FROM plugins
+        JOIN (
+          SELECT id
+          FROM plugins
+          GROUP BY id
+          HAVING COUNT(1) > 1)
+        AS x
+        USING (id)
+        ORDER BY id, name;
+      ]])
+      if err then
+        return err
+      end
+
+      -- we didnt find any duplicates; we're golden!
+      if #duplicates == 0 then
+        return
+      end
+
+      -- print a human-readable output of all the plugins with conflicting ids
+      local t = {}
+      t[#t + 1] = "\n\nPlease correct the following duplicate plugin entries and re-run this migration:\n"
+      for i = 1, #duplicates do
+        local d = duplicates[i]
+        local p = {}
+        for k, v in pairs(d) do
+          p[#p + 1] = k .. ": " .. tostring(v)
+        end
+        t[#t + 1] = table.concat(p, "\n")
+        t[#t + 1] = "\n"
+      end
+
+      return table.concat(t, "\n")
+    end,
+    down = function(_, _, dao) return end
+  },
+  {
+    name = "2017-04-18-153000_unique_plugins_id_2",
+    up = [[
+      ALTER TABLE plugins ADD CONSTRAINT plugins_id_key UNIQUE(id);
+    ]],
+    down = [[
+      ALTER TABLE plugins DROP CONSTRAINT plugins_id_key;
+    ]],
+  },
 }

kong/dao/schemas/plugins.lua

@@ -19,7 +19,8 @@ return {
     id = {
       type = "id",
       dao_insert_value = true,
-      required = true
+      required = true,
+      unique = true,
     },
     created_at = {
       type = "timestamp",

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

@@ -1,17 +1,27 @@
 local helpers = require "spec.helpers"
 local cjson = require "cjson"
 
+local dao_helpers = require "spec.02-integration.02-dao.helpers"
+local DAOFactory = require "kong.dao.factory"
+
 local function it_content_types(title, fn)
   local test_form_encoded = fn("application/x-www-form-urlencoded")
   local test_json = fn("application/json")
   it(title.." with application/www-form-urlencoded", test_form_encoded)
   it(title.." with application/json", test_json)
 end
 
-describe("Admin API", function()
+dao_helpers.for_each_dao(function(kong_config)
+
+describe("Admin API " .. kong_config.database, function()
   local client
+  local dao
   setup(function()
-    assert(helpers.start_kong())
+    dao = assert(DAOFactory.new(kong_config))
+
+    assert(helpers.start_kong{
+      database = kong_config.database
+    })
     client = assert(helpers.admin_client())
   end)
   teardown(function()
@@ -22,7 +32,7 @@ describe("Admin API", function()
   describe("/apis", function()
     describe("POST", function()
       before_each(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
       end)
       it_content_types("creates an API", function(content_type)
         return function()
@@ -150,7 +160,7 @@ describe("Admin API", function()
 
     describe("PUT", function()
       before_each(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
       end)
 
       it_content_types("creates if not exists", function(content_type)
@@ -289,18 +299,18 @@ describe("Admin API", function()
 
     describe("GET", function()
       setup(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
 
         for i = 1, 10 do
-          assert(helpers.dao.apis:insert {
+          assert(dao.apis:insert {
             name = "api-"..i,
             uris = "/api-"..i,
             upstream_url = "http://my-api.com"
           })
         end
       end)
       teardown(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
       end)
 
       it("retrieves the first page", function()
@@ -365,7 +375,7 @@ describe("Admin API", function()
 
       describe("empty results", function()
         setup(function()
-          helpers.dao:truncate_tables()
+          dao:truncate_tables()
         end)
 
         it("data property is an empty array", function()
@@ -396,17 +406,17 @@ describe("Admin API", function()
     describe("/apis/{api}", function()
       local api
       setup(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
       end)
       before_each(function()
-        api = assert(helpers.dao.apis:insert {
+        api = assert(dao.apis:insert {
           name = "my-api",
           uris = "/my-api",
           upstream_url = "http://my-api.com"
         })
       end)
       after_each(function()
-        helpers.dao:truncate_tables()
+        dao:truncate_tables()
       end)
 
       describe("GET", function()
@@ -464,7 +474,7 @@ describe("Admin API", function()
             assert.equal("my-updated-api", json.name)
             assert.equal(api.id, json.id)
 
-            local in_db = assert(helpers.dao.apis:find {id = api.id})
+            local in_db = assert(dao.apis:find {id = api.id})
             assert.same(json, in_db)
           end
         end)
@@ -483,7 +493,7 @@ describe("Admin API", function()
             assert.equal("my-updated-api", json.name)
             assert.equal(api.id, json.id)
 
-            local in_db = assert(helpers.dao.apis:find {id = api.id})
+            local in_db = assert(dao.apis:find {id = api.id})
             assert.same(json, in_db)
           end
         end)
@@ -502,7 +512,7 @@ describe("Admin API", function()
             assert.same({ "/my-updated-api", "/my-new-uri" }, json.uris)
             assert.equal(api.id, json.id)
 
-            local in_db = assert(helpers.dao.apis:find {id = api.id})
+            local in_db = assert(dao.apis:find {id = api.id})
             assert.same(json, in_db)
           end
         end)
@@ -521,7 +531,7 @@ describe("Admin API", function()
             assert.True(json.strip_uri)
             assert.equal(api.id, json.id)
 
-            local in_db = assert(helpers.dao.apis:find {id = api.id})
+            local in_db = assert(dao.apis:find {id = api.id})
             assert.same(json, in_db)
           end
         end)
@@ -542,7 +552,7 @@ describe("Admin API", function()
             assert.same({ "my-updated.tld" }, json.hosts)
             assert.equal(api.id, json.id)
 
-            local in_db = assert(helpers.dao.apis:find {id = api.id})
+            local in_db = assert(dao.apis:find {id = api.id})
             assert.same(json, in_db)
           end
         end)
@@ -609,16 +619,16 @@ describe("Admin API", function()
   describe("/apis/{api}/plugins", function()
     local api
     setup(function()
-      helpers.dao:truncate_tables()
+      dao:truncate_tables()
 
-      api = assert(helpers.dao.apis:insert {
+      api = assert(dao.apis:insert {
         name = "my-api",
         uris = "/my-api",
         upstream_url = "http://my-api.com"
       })
     end)
     before_each(function()
-      helpers.dao.plugins:truncate()
+      dao.plugins:truncate()
     end)
 
     describe("POST", function()
@@ -657,6 +667,15 @@ describe("Admin API", function()
         end
       end)
       describe("errors", function()
+        -- TODO fix the weird nesting issues in this file that
+        -- require us to rescope client
+        local client
+        before_each(function()
+          client = assert(helpers.admin_client())
+        end)
+        after_each(function()
+          if client then client:close() end
+        end)
         it_content_types("handles invalid input", function(content_type)
           return function()
             local res = assert(client:send {
@@ -697,6 +716,35 @@ describe("Admin API", function()
             assert.same({ name = "already exists with value 'basic-auth'"}, json)
           end
         end)
+        it_content_types("returns 409 on id conflict #xxx", function(content_type)
+          return function()
+            -- insert initial plugin
+            local res = assert(client:send {
+              method = "POST",
+              path = "/apis/"..api.id.."/plugins",
+              body = {
+                name="basic-auth",
+              },
+              headers = {["Content-Type"] = content_type}
+            })
+            local body = assert.res_status(201, res)
+            local plugin = cjson.decode(body)
+
+            -- do it again, to provoke the error
+            local conflict_res = assert(client:send {
+              method = "POST",
+              path = "/apis/"..api.id.."/plugins",
+              body = {
+                name="key-auth",
+                id = plugin.id,
+              },
+              headers = {["Content-Type"] = content_type}
+            })
+            local conflict_body = assert.res_status(409, conflict_res)
+            local json = cjson.decode(conflict_body)
+            assert.same({ id = "already exists with value '" .. plugin.id .. "'"}, json)
+          end
+        end)
       end)
     end)
 
@@ -753,7 +801,7 @@ describe("Admin API", function()
       end)
       it_content_types("perfers default values when replacing", function(content_type)
         return function()
-          local plugin = assert(helpers.dao.plugins:insert {
+          local plugin = assert(dao.plugins:insert {
             name = "key-auth",
             api_id = api.id,
             config = {hide_credentials = true}
@@ -776,7 +824,7 @@ describe("Admin API", function()
           local json = cjson.decode(body)
           assert.False(json.config.hide_credentials) -- not true anymore
 
-          plugin = assert(helpers.dao.plugins:find {
+          plugin = assert(dao.plugins:find {
             id = plugin.id,
             name = plugin.name
           })
@@ -786,7 +834,7 @@ describe("Admin API", function()
       end)
       it_content_types("overrides a plugin previous config if partial", function(content_type)
         return function()
-          local plugin = assert(helpers.dao.plugins:insert {
+          local plugin = assert(dao.plugins:insert {
             name = "key-auth",
             api_id = api.id
           })
@@ -810,7 +858,7 @@ describe("Admin API", function()
       end)
       it_content_types("updates the enabled property", function(content_type)
         return function()
-          local plugin = assert(helpers.dao.plugins:insert {
+          local plugin = assert(dao.plugins:insert {
             name = "key-auth",
             api_id = api.id
           })
@@ -831,7 +879,7 @@ describe("Admin API", function()
           local json = cjson.decode(body)
           assert.False(json.enabled)
 
-          plugin = assert(helpers.dao.plugins:find {
+          plugin = assert(dao.plugins:find {
             id = plugin.id,
             name = plugin.name
           })
@@ -856,7 +904,7 @@ describe("Admin API", function()
 
     describe("GET", function()
       it("retrieves the first page", function()
-        assert(helpers.dao.plugins:insert {
+        assert(dao.plugins:insert {
           name = "key-auth",
           api_id = api.id
         })
@@ -884,7 +932,7 @@ describe("Admin API", function()
     describe("/apis/{api}/plugins/{plugin}", function()
       local plugin
       before_each(function()
-        plugin = assert(helpers.dao.plugins:insert {
+        plugin = assert(dao.plugins:insert {
           name = "key-auth",
           api_id = api.id
         })
@@ -911,7 +959,7 @@ describe("Admin API", function()
         end)
         it("only retrieves if associated to the correct API", function()
           -- Create an API and try to query our plugin through it
-          local w_api = assert(helpers.dao.apis:insert {
+          local w_api = assert(dao.apis:insert {
             name = "wrong-api",
             uris = "/wrong-api",
             upstream_url = "http://wrong-api.com"
@@ -953,7 +1001,7 @@ describe("Admin API", function()
             assert.same({"key-updated"}, json.config.key_names)
             assert.equal(plugin.id, json.id)
 
-            local in_db = assert(helpers.dao.plugins:find {
+            local in_db = assert(dao.plugins:find {
               id = plugin.id,
               name = plugin.name
             })
@@ -963,7 +1011,7 @@ describe("Admin API", function()
         it_content_types("doesn't override a plugin config if partial", function(content_type)
           -- This is delicate since a plugin config is a text field in a DB like Cassandra
           return function()
-            plugin = assert(helpers.dao.plugins:update({
+            plugin = assert(dao.plugins:update({
               config = {hide_credentials = true}
             }, {id = plugin.id, name = plugin.name}))
             assert.True(plugin.config.hide_credentials)
@@ -982,7 +1030,7 @@ describe("Admin API", function()
             assert.True(json.config.hide_credentials) -- still true
             assert.same({"my-new-key"}, json.config.key_names)
 
-            plugin = assert(helpers.dao.plugins:find {
+            plugin = assert(dao.plugins:find {
               id = plugin.id,
               name = plugin.name
             })
@@ -1005,7 +1053,7 @@ describe("Admin API", function()
             local json = cjson.decode(body)
             assert.False(json.enabled)
 
-            plugin = assert(helpers.dao.plugins:find {
+            plugin = assert(dao.plugins:find {
               id = plugin.id,
               name = plugin.name
             })
@@ -1063,6 +1111,8 @@ describe("Admin API", function()
   end)
 end)
 
+end)
+
 describe("Admin API request size", function()
   local client
   setup(function()