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

[data views] Field editor endpoint versioning and schema validation #159626

Merged
merged 15 commits into from
Jun 30, 2023

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Jun 13, 2023

Summary

  • Move field preview to internal route
  • Add versioning to route
  • Endpoint is called with version
  • Response schema validation

Closes #159158

@mattkime mattkime changed the title partial progress [data views] Field editor response schema Jun 17, 2023
@mattkime mattkime changed the title [data views] Field editor response schema [data views] Field editor endpoint versioning and schema validation Jun 17, 2023
@mattkime mattkime marked this pull request as ready for review June 17, 2023 04:26
@mattkime mattkime requested a review from a team as a code owner June 17, 2023 04:26
@mattkime mattkime self-assigned this Jun 17, 2023
@mattkime mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 17, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! But I did run into one output validation issue when testing composite runtime fields.

This is the field I was trying to create (I disabled output validation to get this screenshot):
composite

With the validation enabled, I got this toast:
error

And this error response:

{
    "statusCode": 500,
    "error": "Internal Server Error",
    "message": "Failed output validation: [request body.values]: types that failed validation:\n- [request body.values.0.composite_field.test]: types that failed validation:\n - [request body.0]: expected value of type [boolean] but got [Array]\n - [request body.1]: expected value of type [number] but got [Array]\n - [request body.2]: expected value of type [string] but got [Array]\n - [request body.3]: expected a plain object value, but found [Array] instead.\n- [request body.values.1]: expected value of type [array] but got [Object]"
}

And for reference, the script I was using:

Map map = new HashMap();
map.put("first","Foo");
map.put("last", "Bar");

emit(map);
emit('map', map); 
emit('test', 'test');
emit('test2', 2555);
emit('test3', [99, 99]);

And the success response with validation disabled:

{
  "composite_field.test": ["test"],
  "composite_field.first": ["Foo"],
  "composite_field.last": ["Bar"],
  "composite_field.test2": [2555],
  "composite_field.test3": [[99, 99]],
  "composite_field.map": [
    {
      "last": "Bar",
      "first": "Foo"
    }
  ]
}

@mattkime
Copy link
Contributor Author

@davismcphee I need to talk to the painless engineers - either you've found an undocumented feature or a bug. Either way, passing maps and arrays to emit isn't documented - https://www.elastic.co/guide/en/elasticsearch/painless/8.8/painless-runtime-fields-context.html

@davismcphee
Copy link
Contributor

@mattkime I think calling emit with a map is supported for composite runtime fields, it's just very poorly documented. It looks like the feature was originally implemented here: elastic/elasticsearch#75108. And the documentation here shows a map being passed to emit as a result of grok().extract(), but that was the only (vague) mention of it that I could find in the documentation.

I'm not sure if emit('key', map) is considered valid since I couldn't find any references in the documentation, I just tried it in testing and it seemed to work.

The array in this case is actual a geo_point field, and it also seems to work as intended in the Discover grid with validation disabled:
composite

To be fair though, I couldn't find explicit mention of using geo_point values in composite fields in any of our documentation either. I just landed on the emit('key', [lat, lon]) syntax through trial and error. Presumably this is an alternate syntax for emit(lat, lon) reserved for composite fields since the usual syntax would conflict with the composite field emit('key', value) syntax, but maybe this isn't intentional and geo_point values shouldn't be supported in composite fields.

Either way though, composite fields seem to fail validation regardless of which values are used. I used complex types in my example to be comprehensive in testing, but even a composite field containing just primitive types currently fails validation. I believe it's because we're expecting Record<string, ValueType> here, but the painless API seems to return Record<string, ValueType[]> instead:

response: {
200: {
body: schema.object({
values: schema.oneOf([
// composite field
schema.recordOf(schema.string(), valueSchema),
// primitive field
schema.arrayOf(valueSchema),
]),
error: schema.maybe(schema.object({}, { unknowns: 'allow' })),
status: schema.maybe(schema.number()),
}),
},
},

@mattkime
Copy link
Contributor Author

@davismcphee I spoke to some ES engineers and the consensus was that passing an array or map to emit('fieldName', value) isn't valid. I've created a issue against the ES repo - elastic/elasticsearch#97126

Past that, everything should work now.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViewFieldEditor 23.3KB 23.3KB +13.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 492 496 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mattkime

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying which subfield types are supported! Since maps and arrays aren't valid, everything seems to be working as expected, LGTM 👍

@mattkime mattkime merged commit a915352 into elastic:main Jun 30, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IndexPatterns] Move internal subroutes to internal
5 participants