Make config-schema extensible for handling of unknown fields #156214
Conversation
|
@elasticmachine merge upstream |
…ibana into config-schema/poc-extendsDeep
|
@elastic/kibana-core I would be curious to hear your thoughts about this approach. Would you benefit from this elsewhere? |
| public extendsDeep(options: ExtendsDeepOptions) { | ||
| const extendedProps = Object.entries(this.props).reduce((memo, [key, value]) => { | ||
| if (value !== null && value !== undefined) { | ||
| return { |
There was a problem hiding this comment.
(thinking out loud here) I'm overall fine with the recursive approach, I can understand that it would be complicated / tedious to manually redefine everything.
Now, if this extendsDeep approach looks fine to me as an internal API to perform the mutation, I wonder if that's what we want as the public API.
I was more thinking of something like:
const type = schema.object({ foo: schema.string() });
const savedObject = { foo: 'test', bar: 'test' };
// ... later
type.validate(savedObject, { unknowns: 'ignore' });Now, technically, internally we don't want to create a new schema everytime validate is called with this option. But if we know that we will only be exposing this single 'schema mutation' option to validate, we could store the mutated schema internally.
However, the validate signature is already crowded with 'useless' stuff as second and third parameters:
So I don't think it would be that easy to adapt it to have a good DX, as it would look like
type.validate(savedObject, {}, undefined, { unknowns: 'ignore' });So in summary, your approach is probably the most pragmatic one.
| private readonly keyType: Type<K>; | ||
| private readonly valueType: Type<V>; | ||
| private readonly options: RecordOfOptions<K, V>; |
There was a problem hiding this comment.
We're now storing all the constructor options on all of our type classes to be able to clone them during extendsDeep.
I'm thinking about GC / memory consumption here. I think it's fine, given the various props were referenced by lower schemas anyway, so we're probably not introducing any significant impact to the memory consumption of schemas here, but ideally someone else would also confirm that.
| const result = type | ||
| .extendsDeep({ unknowns: 'allow' }) |
There was a problem hiding this comment.
(unrelated to this line) I wonder if that extendsDeep will be sufficient covering our 'eviction schema' needs. Like, isn't there going to be scenarios were teams would need a more finely grained configuration, e.g ignore for some props, allow for some others and so on?
I guess in that case, they can still just manually redefine the schema (if such scenario even make sense)
There was a problem hiding this comment.
I can only explain from how I plan to use this but feel free to propose changes. In the example of Task Manager, we plan to evict unknown properties when reading from the task.state object while preventing tasks from running if task.params contains unknown properties (ex: properties created by a newer Kibana version).
We would build two different task schema objects to satisfy the needs:
- schema for read would ignore unknowns for
task.statewhile forbid unknowns fortask.params - schema for write would forbid unknowns for
task.stateandtask.params
Since we have control of the params schema by task type and the state schema by task type, we can intercept those and only apply .extendsDeep(...) as needed before building the task read/write schema for a given task type.
…-ref HEAD~1..HEAD --fix'
…ibana into config-schema/poc-extendsDeep
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @mikecote |
mikecote
changed the title
mikecote
added
Team:Core
…#156214) Related issue elastic#155764. In this POC, I'm adding an `extendsDeep` function to the schema object. This feature allows you to create a copy of an existing schema definition and recursively modify options without mutating them. With `extendsDeep`, you can specify whether unknown attributes on objects should be allowed, forbidden or ignored. This new function is particularly useful for alerting scenarios where we need to drop unknown fields when reading from Elasticsearch without modifying the schema object. Since we don't control the schema definition in some areas, `extendsDeep` provides a convenient way to set the `unknowns` option to all objects recursively. By doing so, we can validate and drop unknown properties using the same defined schema, just with `unknowns: forbid` extension. Usage: ``` // Single, shared type definition const type = schema.object({ foo: schema.string() }); // Drop unknown fields (bar in this case) const savedObject = { foo: 'test', bar: 'test' }; const ignoreSchema = type.extendsDeep({ unknowns: 'ignore' }); ignoreSchema.validate(savedObject); // Prevent unknown fields (bar in this case) const soToUpdate = { foo: 'test', bar: 'test' }; const forbidSchema = type.extendsDeep({ unknowns: 'forbid' }); forbidSchema.validate(soToUpdate); ``` --------- Co-authored-by: Kibana Machine <[email protected]>
Related issue #155764.
In this PR, I'm adding an
extendsDeepfunction to the schema object. This feature allows you to create a copy of an existing schema definition and recursively modify options without mutating them. WithextendsDeep, you can specify whether unknown attributes on objects should be allowed, forbidden or ignored.This new function is particularly useful for alerting scenarios where we need to drop unknown fields when reading from Elasticsearch without modifying the schema object. Since we don't control the schema definition in some areas,
extendsDeepprovides a convenient way to set theunknownsoption to all objects recursively. By doing so, we can validate and drop unknown properties using the same defined schema, just withunknowns: forbidextension.Usage: