-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[content management / maps] Content management / Saved object schema abstraction #155342
[content management / maps] Content management / Saved object schema abstraction #155342
Conversation
…om:mattkime/kibana into content_management_api_so_type_abstraction
…om:mattkime/kibana into content_management_api_so_type_abstraction
…t_management_code_abstraction
…kime/kibana into content_management_code_abstraction
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-only review, schemas LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Out of scope for the work of this PR but I wonder if we don't want to leave the mapAttributesSchema
(which is the main schema for a SO type) loose.
Only for the "result -> out" (so createResultSchema
and objectTypeToGetResultSchema
). Not for what comes "in" which should be strict.
Basically changing the { unknowns: 'forbid' }
to { unknowns: 'allow / ignore' }
. This would allow adding new fields without throwing when validating the schema. New fields are not breaking change and should also be fine in the UI. WDYT?
export const schemaAndOr = schema.oneOf([schema.literal('AND'), schema.literal('OR')]); | ||
|
||
// its recommended to create a subset of this schema for stricter validation | ||
export const searchOptionsSchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better then to expose the properties separately for composing the schema then?
export const searchOptionsSchemaProperties = {
page: schema.maybe(schema.number()),
perPage: schema.maybe(schema.number()),
sortField: schema.maybe(schema.string()),
...
}
// consumer side
const { sortField } = searchOptionsSchemaProperties;
const mySearchOptionsSchema = schema.object({
sortField
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a good idea, will do.
I agree that its not necessary to have strict validation regarding new fields however it seems to me that we should have well defined schemas if possible. I think its the same as having good types with typescript code. |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
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 @mattkime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana-gis changes LGTM
code review
Summary
Abstract schema definitions for using Saved Objects with the content management api. For most schema types, this will reduce creation to only the attributes specific to a saved object. For Option types (create options, update options, search options) the saved object api is more complex and its likely that most SO types will only need to use a portion of it. In these cases we recommend using the provided schema definitions as a pattern for creating simpler schemas.
Follow up to - #154985 - expresses types in schema form