[data views] data_views REST API

[mattkime]

Creation of data_views REST API which is largely a duplication and rename of the index_patterns REST API.

The PR is quite large but the changes are minor:

• Shared functionality has been abstracted so the route and serviceKey (index_pattern or data_view, used for submitting or returning info) are passed as arguments.
• In cases where runtime fields return a field, the data_view versions return a field array instead. This is because runtime fields will soon support returning multiple fields (object runtime fields)
• Tests have been similarly abstracted - they've been moved to a loop which will run both index_pattern and data_view versions

Closes #113422

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: e7df4ac
• Storybooks Preview
• Documentation Changes

Metrics [docs]

✅ unchanged

History

• 💔 Build #161807 failed 4a4338e
• 💛 Build #161260 was flaky 22485c3
• 💚 Build #155184 succeeded 6e2a575

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

[mattkime]

@elasticmachine merge upstream

[elasticmachine]

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

[Dosant]

I am not sure if we really want to rework this approach right now, but here are some things I am looking at:

1. Seems like because of dynamic validation schema building we lost a type inference on body
2. I find this a bit fragile, how we have to build a response object and keys in the response based on serviceKey. Also even ${serviceKey}_id in some cases
3. Same for res.ok(serviceKey === SERVICE_KEY_LEGACY ? legacyResponse : response);

I wonder if instead, we should add one more layer abstracting REST API logic:

Something like:

export const registerLegacyCreateRuntimeFieldRoute = (
   router: IRouter,
   getStartServices: StartServicesAccessor<
     DataViewsServerPluginStartDependencies,
     DataViewsServerPluginStart
   >
 ) => {
   router.post( {
    path: '/api/index_patterns/index_pattern/{id}/runtime_field',
    validate: {...}
}),
 (ctx, req, res) => {

       const id = req.params.id;
       const { name, runtimeField } = req.body;

const dataView = dataViewRestApiService.addRuntimeField(
  ctx,
  getStartServices,
  {id, name, runtimeField}
)

return  {// oldResponseStructure }

}) 


export const registerCreateRuntimeFieldRoute = (
   router: IRouter,
   getStartServices: StartServicesAccessor<
     DataViewsServerPluginStartDependencies,
     DataViewsServerPluginStart
   >
 ) => {
   router.post( {
    path: '/api/data_views/data_view/{id}/runtime_field',
    validate: {...}
}),
 (ctx, req, res) => {

       const id = req.params.id;
       const { name, runtimeField } = req.body;

const dataView = dataViewRestApiService.addRuntimeField(
  ctx,
  getStartServices,
  {id, name, runtimeField}
)

return  {// newResponseStructure }

}) 

Basically instead of wrapping abstraction around routes,
We will have routes separate with a bit of copy-paste there. We will keep validation schemas static. And we will have there only params extraction and response building.
Everything else will happen in a new service which will be shared between new and legacy.

I think this will also help us to build new versions of routes. We will keep request and response processing there but have business logic shared.

Wonder what are you thoughts on this and on the current approach in general

[Dosant]

Looks like type inference from schema got broken because of the dynamic schema approach we took here? :(

[mattkime]

@Dosant You're correct in your observation that type inference has been lost. That said, I think its possible to address it. I see this as a functional problem with the code.

            // [serviceKey]: indexPatternSpecSchema,
            data_view: serviceKey === SERVICE_KEY ? indexPatternSpecSchema : schema.never(),
            index_pattern:
              serviceKey === SERVICE_KEY_LEGACY ? indexPatternSpecSchema : schema.never(),

If I specify the two potential keys it no longer stomps on the other key types. as DataViewSpec is still needed in the indexPatternsService.createAndSave call but that was true before these changes as well.

When you say that the code is fragile, I think you mean that any changes to the one API and not the other will either entail the unwanted addition of further conditionals OR a wholesale separation of the code. I agree, but I think its an acceptable tradeoff. The differences are currently simple so further separation of the code would not be difficult. Further, the code may go untouched for a good long while before this is a concern.

Taking another look at the code, the business logic is often just one or two lines. This is mostly wrapper code, which makes sense considering what it is.

I need to look at this with fresh eyes tomorrow.

[mattkime]

@Dosant I've taken a closer look and haven't found further opportunities for improvement considering the tradeoffs mentioned above.

[mattkime]

@elasticmachine merge upstream

[Dosant]

LGTM,

when we update docs, need not forget to include the change about runtime fields fields

[mattkime]

@elasticmachine merge upstream

[mattkime]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: ad69069
• Storybooks Preview

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
dataViews	562	579	+17

Unknown metric groups

API count

id	before	after	diff
dataViews	704	721	+17

References to deprecated APIs

id	before	after	diff
dataViews	210	206	-4

History

• 💔 Build #16050 failed fab6638
• 💚 Build #15309 succeeded c3133c2
• 💚 Build #14807 succeeded 4b9110f
• 💚 Build #14695 succeeded bb0353e
• 💚 Build #14523 succeeded 8a9d5a2

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