Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(db) move Consumers from kong.dao to kong.db using DAO wrapper #3437

Merged
merged 4 commits into from
May 8, 2018

Conversation

hishamhm
Copy link
Contributor

@hishamhm hishamhm commented May 4, 2018

No description provided.

@@ -740,6 +741,33 @@ return {
DROP TABLE ssl_certificates;
DROP TABLE ssl_servers_names;
]],
down = nil
down = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to remove this migration we need to have #3328 merged first.

@@ -251,7 +251,7 @@ for _, strategy in helpers.each_strategy() do
local credential
before_each(function()
dao:truncate_table("oauth2_credentials")
dao:truncate_table("consumers")
db:truncate("consumers")
db:truncate("services")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call db:truncate() here (it does not accept params, it removes everthing)

local old_dao = self.db.old_dao
local rows, err = old_dao[table_name]:find_all(fk)
if err then
ngx.log(ngx.ERR, "could not gather associated entities for delete cascade: ", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that err can be used like this (it is not a table without __tostring for example)

Namespace the error messages with consumers.delete_cascade

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked: find_all either hard-stops with error("foo") (when not given a table as parameter, for example) or executes ret_error, which makes sure that the second returned value (an old-dao-style error table) is always tostring-ed.


if self.new_db[constraint.table] then
-- new DAO
local res, err = self.new_db[constraint.table]:check_foreign_key({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the check_foreign_key function outside of the new DAO to the old DAO/strategy.

local function check_foreign_constraints(self, values, constraints)
local errors

for col, constraint in pairs(constraints.foreign) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this and see if the invalidation tests still run for postgres (maybe postgres migrations do this already)

@@ -527,7 +527,7 @@ function DAO:check_foreign_key(primary_key, human_name)
return entity
end

local msg = string.format("No such %s (%s)" ,
local msg = string.format("No such entry in %s (%s)" ,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never capitalize error messages.


constraints = constraints[name],

cache_key = function(_, a1, a2, a3, a4, a5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set to DEBUG level with a debug traceback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kikito
Copy link
Member

kikito commented May 4, 2018

Pending: /consumers?custom_id=xxx should return a list with a single item (add custom Admin api endpoint to detect the extra param)

@kikito
Copy link
Member

kikito commented May 7, 2018

Filtering via custom_id also done

kikito and others added 4 commits May 7, 2018 16:34
* On the old DAO side (`kong.dao`), this adds some utilities that are
  needed later for relationships between old-DAO and new-DAO entities,
  once Consumers are moved into the new DAO.
* On the new DAO side (`kong.db`), this adds old DAO access in new DAO.
  This is needed in order to perform cascade-deletes of old-DAO
  entities when a new-DAO entity is deleted.
Changes in other modules are to ensure that a DAOFactory instance
always gets a DB instance.
@hishamhm hishamhm force-pushed the feat/consumers-dao-wrapper branch from a7907aa to 82a4e43 Compare May 7, 2018 19:35
@kikito kikito force-pushed the feat/consumers-dao-wrapper branch from 82a4e43 to 1c354ce Compare May 8, 2018 10:33
@hishamhm hishamhm force-pushed the feat/consumers-dao-wrapper branch from 1c354ce to 82a4e43 Compare May 8, 2018 11:10
@kikito kikito merged commit 8395d6b into next May 8, 2018
@kikito kikito deleted the feat/consumers-dao-wrapper branch May 8, 2018 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants