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/oauth2 daos.lua & api.lua to new db #3774

Merged
merged 4 commits into from Sep 17, 2018

Conversation

Projects
None yet
2 participants
@kikito
Copy link
Member

commented Sep 15, 2018

This change has some previous groundwork, then the commit itself which deals with oauth2, and then some cleanup. Each part has its own separate commit, with explanations. I recommend reviewing the PR commit-by-commit.

@kikito kikito changed the base branch from master to next Sep 15, 2018

@kikito kikito force-pushed the feat/oauth2-daos-api-to-new-db branch from f79d2d4 to c20ae1b Sep 15, 2018

Show resolved Hide resolved kong/plugins/oauth2/access.lua Outdated
Show resolved Hide resolved kong/plugins/oauth2/access.lua
Show resolved Hide resolved kong/plugins/oauth2/migrations/cassandra.lua Outdated
@@ -268,16 +268,16 @@ function Plugins:load_plugin_schemas(plugin_set)
local Strategy = require(string.format("kong.db.strategies.%s", db.strategy))
for name, schema_def in pairs(daos_schemas) do
if name ~= "tables" and schema_def.name then
ngx_log(ngx_DEBUG, string.format("Loading custom plugin entity: '%s.%s'", plugin, name))
ngx_log(ngx_DEBUG, string.format("Loading custom plugin entity: '%s.%s'", plugin, schema_def.name))

This comment has been minimized.

Copy link
@hishamhm

hishamhm Sep 15, 2018

Member

Took me a while to see how this added support for arrays (namely, that then the name variable becomes the numeric index returned in arbitrary order, and that it goes unused in this loop except for comparing against "tables"). I just wonder if keeping this around for backwards compatibility sake is worth the "sneaky" code — plus, this code assumes that pairs always traverses the numeric keys of a table in order, right? I believe this is true in practice in LuaJIT for tables specified in literals as is usually the case in daos.lua, but IIRC it is not a formal guarantee.

And AFAICT the backwards-compat will be partial anyway, since plugins with dependencies among custom daos like oauth2 does will require using ordered arrays.

BTW, thanks for the explanation in the commit message, otherwise I would have totally missed the intent!

This comment has been minimized.

Copy link
@kikito

kikito Sep 16, 2018

Author Member

Removing the backward-compatibility and using ipairs instead of pairs would involve changing all the plugins' daos.lua files to return an array instead of a hash. That is the main reason for keeping the "sneaky" code here.

If you don't think that's a good enough reason I will change all the daos.lua to arrays and use ipairs here.

@hishamhm

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

Reviewed commit-by-commit. I think the overall port of Oauth2 looks good! I think the fix for the Cassandra migrations is the only blocker.

@kikito kikito force-pushed the feat/oauth2-daos-api-to-new-db branch 2 times, most recently from 6f9bc0b to a563471 Sep 16, 2018

feat(schema) support foreign relationships with non-core entities
Our schemas didn't support referencing non-core entities on foreign keys,
but this plugin needed that, so that's bundled in a separate commit. It
means that the order in which the schemas are loaded matters now
(if you want to have a fk to a schema A in schema B, you must do
`Schema.new(A)` before you can do `Schema.new(B)`

@kikito kikito force-pushed the feat/oauth2-daos-api-to-new-db branch from a563471 to 4186825 Sep 16, 2018

@hishamhm

This comment has been minimized.

Copy link
Member

commented Sep 16, 2018

@kikito Regarding the "sneaky" loop, my suggestion then is to add a comment above the pairs() loop with something like

-- If the daos list is declared as an array, we assume below that pairs()
-- will traverse them in order and initialize the schemas
-- such that foreign field relationships are resolved correctly

...or use this order-safe iterator instead? :) (couldn't resist! (untested of course) )

local function each_dao(daos)
  local phase = 1
  local ak = 1
  local hk = next(daos)
  return function()
    if phase == 1 then
      local v = daos[ak]
      ak = ak + 1
      if v == nil then
        phase = 2
      else
        return v
      end
    end
    --- phase == 2:
    while type(hk) == "number" then
      hk = next(daos, hk)
    end
    if hk == "tables" then
      hk = next(daos, hk)
    end
    v = daos[hk]
    hk = next(daos, hk)
    return v
  end
end

kikito added some commits Sep 13, 2018

refactor(runloop) accept arrays in addition to hashes in plugins/daos…
….lua

This change also allows returning an array from a plugin's daos.lua file,
instead of a hash. This way the loading order is preserved. In the core this
feature is only needed for loading oauth2 plugin's daos.lua entities in
the correct order (credentials are loaded first so that tokens and codes
can have foreign keys to them).
refactor(db) remove custom dao from consumers
Our consumers' custom DAO object was there to deal with delete-cascading
objects in the old DAO pointing to a consumer. After I moved the oauth2
objects to the new DB there isn't anly old-DAO object with foreign keys
to consumers any more, so that code is now redundant.

@kikito kikito force-pushed the feat/oauth2-daos-api-to-new-db branch from 4186825 to f4e8be1 Sep 16, 2018

@hishamhm hishamhm merged commit d4ffab1 into next Sep 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hishamhm hishamhm deleted the feat/oauth2-daos-api-to-new-db branch Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.