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/plugins schemas to new db #3778

Merged
merged 8 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@kikito
Copy link
Member

kikito commented Sep 16, 2018

No description provided.

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch 10 times, most recently from 64bb88b to 6675897 Sep 16, 2018

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch 6 times, most recently from aedbf57 to aa8742b Sep 21, 2018

@kikito kikito removed the pr/do not merge label Sep 21, 2018

@kikito kikito changed the title WIP - Feat/plugins schemas to new db Feat/plugins schemas to new db Sep 21, 2018

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch 2 times, most recently from 13954de to 6db701c Sep 21, 2018

@hishamhm

This comment has been minimized.

Copy link
Member

hishamhm commented Oct 2, 2018

Reviewing commit by commit!

Leaving a note here since there is no "general comment on commit": please rename fix(schema) default values for structs to fix(schema) default values for records to avoid confusion.

@@ -270,6 +270,9 @@ Schema.validators = {
end,

uuid = function(value)
if value == "" then
return true
end

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 2, 2018

Member

This does look odd, and the commit says "Check if this is needed"... which plugin test needs this to pass?

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 2, 2018

Member

Looks like this is related to config.anonymous. I see you added is_present in the plugins to support both "" and ngx.null, I guess to avoid a migration (always a good goal!).

I wonder if we could get rid of these empty strings via process_auto_fields instead. Perhaps with a legacy annotation?

-- in process_auto_fields (for field.auto == false case only)
if field.legacy and field.uuid and output[key] == "" then
  output[key] == ngx.null
end

This would always clean up the data once it comes from the DB (it's a bit of a "migration transformation"). The plugins wouldn't need the is_present tests and this change to the uuid validator wouldn't be necessary (ie, we'd avoid messing up all UUID fields because of anonymous). What do you think?

This comment has been minimized.

Copy link
@kikito

kikito Oct 3, 2018

Author Member

I like that! It didn't occur to me to do it that way.

Show resolved Hide resolved kong/db/schema/init.lua
Show resolved Hide resolved kong/db/schema/init.lua
output[key] = utils.random_string()
end
end

if context == "update" then
output[key] = adjust_field_for_update(field, output[key], nulls)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 2, 2018

Member

minor: the style guide says this is not necessary for one-liner if/then/else blocks

This comment has been minimized.

Copy link
@kikito

kikito Oct 3, 2018

Author Member

ok removed that one

Show resolved Hide resolved kong/db/schema/init.lua
fields = {
{ whitelist = {
type = "array",
elements = { type = "string", is_regex = true, match = ".*" },

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 2, 2018

Member

I understand this match rule was already present in the old schema, but it does nothing useful ("match zero or more of any character"), right?

Show resolved Hide resolved kong/plugins/datadog/schema.lua
Show resolved Hide resolved kong/plugins/request-termination/schema.lua
@hishamhm

This comment has been minimized.

Copy link
Member

hishamhm commented Oct 2, 2018

The "pr" build of Travis seems to be failing over and over in the C*2 integration job with a weird error in the datadog test suite. Currently the are conflicts with next, and @bungle has fixed a flakiness problem in the datadog tests. Perhaps this will be fixed after this PR merges cleanly again?

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch 3 times, most recently from d0f00b4 to a6880cd Oct 3, 2018

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch from a6880cd to 4cb6341 Oct 3, 2018

@hishamhm hishamhm force-pushed the feat/plugins-schemas-to-new-db branch from 4cb6341 to 131a8f1 Oct 3, 2018

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch from 131a8f1 to 23169b5 Oct 3, 2018

@hishamhm hishamhm force-pushed the feat/plugins-schemas-to-new-db branch from 23169b5 to 9f7bc77 Oct 3, 2018

@kikito kikito force-pushed the feat/plugins-schemas-to-new-db branch from 9f7bc77 to df89f46 Oct 4, 2018

@hishamhm hishamhm force-pushed the feat/plugins-schemas-to-new-db branch from 4fac07c to eb91de2 Oct 4, 2018

@hishamhm
Copy link
Member

hishamhm left a comment

All set, but waiting for master to be merged into next so we can remove the tests(helpers) workaround on this

kikito and others added some commits Sep 16, 2018

feat(schema) add `gt` validator
We cannot use `between' because sometimes the condition is non-inclusive (`gt = 0`
does not include 0)
fix(schema) fix propagation of default values for records
This calls `handle_missing_value` when propagating defaults
for records so that non-nullable records (as in Entities)
get their sub-tables created. Also refactors both
adjustment functions into one (the original version of
this patch missed the other function, so this avoids
it happening again).

Co-authored-by: Enrique García Cota <kikito@gmail.com>
fix(schema) make records non-nullable in plugin subschemas
Entities already had non-nullable records by default
since 30789e7. However, plugin subschemas did not go through
the same preprocessing. This adds a new function
`Entity.new_subschema` that performs that same preprocessing.
feat(schema) auto convert empty strings in legacy uuids to null
UUIDs created with the old schema stored an empty string to
mean "value not present". We now use an explicit `null` for that.
This `legacy` annotation allows the schema library to transform
existing rows stored in the old format to use the new format
transparently.

Co-authored-by: Hisham Muhammad <hisham@gobolinux.org>

@hishamhm hishamhm force-pushed the feat/plugins-schemas-to-new-db branch from eb91de2 to 9e0d1c4 Oct 4, 2018

@hishamhm hishamhm merged commit 9cda865 into next Oct 4, 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/plugins-schemas-to-new-db branch Oct 4, 2018

@hishamhm

This comment has been minimized.

Copy link
Member

hishamhm commented Oct 4, 2018

Merged with a merge commit, as we do for major changes

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.