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(schema) add support for subschemas #3630

Merged
merged 2 commits into from Jul 24, 2018

Conversation

Projects
None yet
2 participants
@hishamhm
Copy link
Member

hishamhm commented Jul 18, 2018

This resolves three difficulties that would appear when porting the Plugins entity over to the new DAO: loading the plugin configuration schema dynamically, the fact that this schema depends on a value of the entity row itself, and the fact that the specific plugin type can impose further constraints on values.

The old DAO implemented this by allowing records to have a schema field that would be either a table or a function evaluated at validation time, and using an ad-hoc validation that checked for a no_consumer field to restrict the value of consumer_id to null.

The new DAO implements this by allowing an entity (such as Plugins) to declare that it is extensible via subschemas:

  • a schema may declare that it allows subschemas, tagged by one specific string key of the schema, with the top-level subschema_key property.
    • essentially, the set of all plugins taken as one works like a tagged union.
    • for Plugins, subschema_key will be "name"
  • a schema may declare where it gets subschemas from, with the subschema_module.
    • for Plugins, subschema_module will be "kong.plugins.?.schema"
  • the parent schema may declare fields as "abstract", which are meant to be specialized by the subschema. Abstract records don't need their fields property to be set, and they must be given in the specialized ones.
    • for Plugins, config will be abstract
  • the set of fields considered when validating an entity is that of the parent schema
    • subschemas cannot introduce new fields, those need to be declared in the parent schema ("abstract" if needed)
    • the data model of strategies can be inferred from parent schema alone
  • subschemas may override both abstract and non-abstract fields.
    • they cannot change the type of a field
    • a field declared in the subschema overrides the parent schema's field completely (constraints are not merged)
    • for Plugins, plugins will override config, and for the no_consumer feature, they can override consumer using the eq = null validator

@hishamhm hishamhm force-pushed the feat/subschemas branch from 07aea01 to 185b285 Jul 18, 2018

@hishamhm hishamhm requested review from bungle and kikito Jul 19, 2018

@kikito
Copy link
Member

kikito left a comment

I am not sure the parent needs to know where his subschemas are: the "kong.plugins.?.schema" part is not needed as far as I know - as long as we "keep track of the list of subschemas" when a new subschema is found.

Similarly, I don't think the abstract field abstraction is needed. It seems to me that the only reason to have them is to raise an error if someone wants to instantiate a Plugin without the correct subschema. But in that case we can still raise an error by detecting that the subschema_key doesn't correspond with any of the loaded subschemas.

@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Jul 20, 2018

I am not sure the parent needs to know where his subschemas are: the "kong.plugins.?.schema" part is not needed as far as I know - as long as we "keep track of the list of subschemas" when a new subschema is found.

We need that otherwise the knowledge of this path ends up hardcoded somewhere. This is already a problem with core entities because foreign relationships assume that other entities are core entities: https://github.com/Kong/kong/blob/master/kong/db/schema/init.lua#L1182-L1183 (@james-callahan has had problems with that line already). I don't want to repeat this. For foreign-key relationships we could have made it such that the reference field needs a full module path, but for subschemas we don't have this luxury because the "name" field of plugins needs to be backwards-compatible.

Unless you are proposing that we load every plugin beforehand, including their config schemas and eventual custom DAOs, and store them all in the same flat/single namespace. I think keeping these namespaces separate (at the schema level at least) is less prone to conflict.

For example, one possible (far off) future application of subschemas would be in a redesign of the Upstream entity into a more general Balancer entity where instead of a bunch of fields for hashing method, where we need to add fields each time we add a new method (e.g. hash_on_cookie), we could have a Balancer.config field and implement kong.runloop.balancer.methods.?.schema without worrying if there's another plugin called cookie out there when you create a cookie hashing method. (Of course one could fall into the age-old "but then let's create a convention that every X is prefixed by Y" which is always true for single-namespace propositions, but namespaces exist for a reason. :) )

Similarly, I don't think the abstract field abstraction is needed. It seems to me that the only reason to have them is to raise an error if someone wants to instantiate a Plugin without the correct subschema. But in that case we can still raise an error by detecting that the subschema_key doesn't correspond with any of the loaded subschemas.

The reason it is there is so that the parent schema alone can used to produce the DDL stuff for creating the database table in the strategy, without knowledge of the concrete ones, and for forcing certain fields to be specialized (config, in Plugins case).

@hishamhm

This comment has been minimized.

Copy link
Member Author

hishamhm commented Jul 23, 2018

@kikito I pushed a fixup commit (to be squashed when merged), based on the feedback you gave me today. It removes the subschema_module property, making the (pre-)loading of subschemas a separate concern from the schema library itself.

I think it reaches a nice middle-ground between the various approaches we suggested. Let me know what you think!

@kikito

kikito approved these changes Jul 24, 2018

hishamhm added some commits Jul 18, 2018

feat(schema) add eq validator
Does what it says: value must be equal to the given one:
`{ foo = { type = "integer", eq = 42 } }` will force the "foo"
field to be always 42.

This looks like a weird validator to have, but it's going to come in handy
when we define in a plugin subschema that the Consumer field must be `eq =
ngx.null`, properly wrapped in a `typedefs.no_consumer` to replace the
ad-hoc `no_consumer = true` feature of the old schema library.
feat(schema) add support for subschemas
This resolves three difficulties that would appear when porting the Plugins
entity over to the new DAO: loading the plugin configuration schema
dynamically, the fact that this schema depends on a value of the entity row itself,
and the fact that the specific plugin type can impose further constraints on
values.

The old DAO implemented this by allowing records to have a `schema` field that
would be either a table or a function evaluated at validation time, and using
an ad-hoc validation that checked for a `no_consumer` field to restrict the
value of `consumer_id` to `null`.

The new DAO implements this by allowing an entity (such as Plugins) to declare
that it is extensible via **subschemas**:

* a schema may declare that it allows subschemas, tagged by one specific
  string key of the schema, with the top-level `subschema_key` property.
  * essentially, the set of all plugins taken as one works like a tagged union.
  * for Plugins, `subschema_key` will be `"name"`
* a schema may declare where it gets subschemas from, with the
  `subschema_module`.
  * for Plugins, `subschema_module` will be `"kong.plugins.?.schema"`
* the parent schema may declare fields as "abstract", which are meant to be
  specialized by the subschema. Abstract records don't need their `fields`
  property to be set, and they must be given in the specialized ones.
  * for Plugins, `config` will be `abstract`
* the set of fields considered when validating an entity is that of the
  parent schema
  * subschemas cannot introduce new fields, those need to be
    declared in the parent schema ("abstract" if needed)
  * the data model of strategies can be inferred from parent schema alone
* subschemas may override both abstract and non-abstract fields.
  * they cannot change the type of a field
  * a field declared in the subschema overrides the parent schema's field
    completely (constraints are not merged)
  * for Plugins, plugins will override `config`, and for the `no_consumer`
    feature, they can override `consumer` using the `eq = null` validator

@hishamhm hishamhm force-pushed the feat/subschemas branch from 527b925 to 86179f9 Jul 24, 2018

@hishamhm hishamhm merged commit 6d4006c into next Jul 24, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@hishamhm hishamhm deleted the feat/subschemas branch Jul 24, 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.