Index pattern - refactor constructor by mattkime · Pull Request #77791 · elastic/kibana

mattkime · 2020-09-17T17:04:15Z

Summary

The index pattern factory and crud methods have been refactored -

Create new indexPattern instance (unsaved) -
indexPatternService.make() => indexPatternService.create(indexPatternSpec, skipFetchFields)

Save new index pattern -
indexPattern.create() => indexPatternService.createSavedObject(indexPattern)

Setting the default index pattern is done as part of indexPatternService.createSavedObject but can also be called individually-
uiSettings.set('defaultIndex', id) => indexPatternService.setDefault(indexPatternId, force)

Update index pattern -
indexPattern.save() => indexPatternService.updateSavedObject(indexPattern)

Unchanged
get index pattern - indexPatternService.get(id);

Other changes

indexPatternService.get(); no longer returns a new IndexPattern instance
IndexPatternSpec.fields now uses Record<string, FieldSpec> instead of FieldSpec[]
No longer throwing on unknown field (this was a bug)
indexPattern.fieldsFetcher is replaced by indexPatternService.getFieldsForWildcard and indexPatternService.getFieldsForIndexPattern
indexPattern.originalBody => indexPattern.originalSavedObjectBody updates via indexPattern.resetOriginalSavedObjectBody
indexPattern.refreshFields => indexPatternService.refreshFields(indexPattern)
indexPatternService.createAndSave(indexPatternSpec) convenience method added
indexPatternService.getFieldsForWildcard can be called directly. Previously a temp index pattern had to be created.

Addresses

Unknown types in field caps throw errors during index pattern creation - [Index patterns] Unknown types in field caps throw errors during index pattern creation #77125
(this is an easy to reproduce example of the issue above) - Unknown field type action.secrets in .kibana index pattern #77831
Make index pattern static - make IndexPattern class static #69379
Index pattern cleanup - Index Pattern API - clean up field index pattern and index patterns interfaces #68020
partially addresses type cleanup - Index Pattern API - Only export types, not classes #68012

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dev Docs

The index pattern factory and crud methods have been refactored -

Create new indexPattern instance (unsaved) -
indexPatternService.make() => indexPatternService.create(indexPatternSpec, skipFetchFields)

Save new index pattern -
indexPattern.create() => indexPatternService.createSavedObject(indexPattern)

Setting the default index pattern is done as part of indexPatternService.createSavedObject but can also be called individually-
uiSettings.set('defaultIndex', id) => indexPatternService.setDefault(indexPatternId, force)

Update index pattern -
indexPattern.save() => indexPatternService.updateSavedObject(indexPattern)

Additional changes

indexPatternService.get(); no longer returns a new IndexPattern instance
IndexPatternSpec.fields now uses Record<string, FieldSpec> instead of FieldSpec[]
indexPattern.fieldsFetcher is replaced by indexPatternService.getFieldsForWildcard and indexPatternService.getFieldsForIndexPattern
indexPattern.originalBody => indexPattern.originalSavedObjectBody updates via indexPattern.resetOriginalSavedObjectBody
indexPattern.refreshFields => indexPatternService.refreshFields(indexPattern)
indexPatternService.createAndSave(indexPatternSpec) convenience method added
indexPatternService.getFieldsForWildcard can be called directly. Previously a temp index pattern had to be created.

ppisljar

code LGTM

mshustov

approved since the current implementation reproduces the old logic

mshustov · 2020-09-21T13:38:31Z

src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

+  try {
+    emptyPattern = await indexPatterns.createAndSave(indexPatternSpec, overwriteAll, true);
+  } catch (err) {
+    if (err instanceof DuplicateIndexPatternError) {


optional: does it make sense to provide an error helper instead to hide an implementation details?

dataPluginErrors.indexPatterns.isDuplicate(err) ?

I think this is a code style question. In this case DuplicateIndexPatternError is being treated as a public interface. The code is trivial. I think code consistency is more important than which path we choose.

mshustov · 2020-09-21T13:40:45Z

src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

-    } else {
-      return;
+      if (isConfirmed) {
+        emptyPattern = await indexPatterns.createAndSave(indexPatternSpec, true, true);


So we assume it couldn't throw here?

In this case we're overriding any existing index patterns so DuplicateIndexPatternError won't throw.

As best I can tell the handling of errors (or lack thereof) has been maintained.

...lugins/ml/public/application/datavisualizer/file_based/components/import_view/import_view.js

...tics/pages/analytics_management/hooks/use_create_analytics_form/use_create_analytics_form.ts

...lugins/ml/public/application/datavisualizer/file_based/components/import_view/import_view.js

kertal

KibanaApp owned code LGTM, there were just tests changed.

alvarezmelissa87

LGTM ⚡

lukeelmers

Code LGTM. A few nits/questions but nothing critical.

lukeelmers · 2020-09-22T21:26:55Z

src/plugins/data/public/index.ts

  formatHitProvider,
 } from './index_patterns';

+export type { IndexPatternsService } from './index_patterns';


does this need to be exported?

Exported for docs

lukeelmers · 2020-09-22T21:27:35Z

src/plugins/data/server/index.ts

  schema: configSchema,
 };
+
+export type { IndexPatternsService } from './index_patterns';


does this need to be exported?

Exported for docs

src/plugins/data/public/index.ts

lukeelmers · 2020-09-22T21:29:30Z

...plugins/index_pattern_management/public/components/edit_index_pattern/edit_index_pattern.tsx

      overlays.openConfirm(confirmMessage, confirmModalOptionsRefresh).then(async (isConfirmed) => {
        if (isConfirmed) {
-          await indexPattern.refreshFields();
+          // todo catch error as in index_pattern


Is this TODO a leftover task from this PR, or for future reference?

ah, thought I got that one - addressed.

kibanamachine · 2020-09-23T00:56:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 569e00f

Build metrics

async chunks size

id	value	diff	baseline
fileUpload	719.6KB	-1.2KB	720.7KB
indexPatternManagement	807.2KB	+963.0B	806.2KB
ml	8.1MB	-1.6KB	8.1MB
transform	660.9KB	-264.0B	661.1KB
total		-2.0KB

page load bundle size

id	value	diff	baseline
data	1.5MB	+2.0KB	1.5MB
ml	873.6KB	+53.0B	873.5KB
savedObjectsManagement	226.2KB	+263.0B	225.9KB
total		+2.3KB

History

💚 Build #76377 succeeded 305f3f8
💔 Build #76360 failed ae0d9ee
💚 Build #76268 succeeded 498637b
💚 Build #76217 succeeded 59ec324
💚 Build #76021 succeeded 5b3d104

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

* index pattern - refactor constructor # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern._constructor_.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.indexpattern._constructor_.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.indexpattern.md # src/plugins/data/common/index_patterns/index_patterns/_fields_fetcher.ts # src/plugins/data/common/index_patterns/index_patterns/index_pattern.test.ts # src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts # src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts # src/plugins/data/public/public.api.md # src/plugins/data/server/server.api.md # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

* master: (31 commits) skip tests for old pacakge (elastic#78194) [Ingest Pipelines] Add url generator for ingest pipelines app (elastic#77872) [Lens] Rename "telemetry" to "stats" (elastic#78125) [CSM] Url search (elastic#77516) [Drilldowns] Config to disable URL Drilldown (elastic#77887) [Lens] Combined histogram/range aggregation for numbers (elastic#76121) Remove legacy plugins support (elastic#77599) 'Auto' interval must be correctly calculated for natural numbers (elastic#77995) [CSM] fix ingest data retry order messed up (elastic#78163) Add response status helpers (elastic#78006) Bump react-beautiful-dnd (elastic#78028) [Security Solution][Detection Engine] Bubbles up more error messages from ES queries to the UI (elastic#78004) Index pattern - refactor constructor (elastic#77791) Add `xpack.security.sameSiteCookies` to docker allow list (elastic#78192) Remove [key: string]: any; from IIndexPattern (elastic#77968) Remove requirement for manage_index_templates privilege for Index Management (elastic#77377) [Ingest Manager] Agent bulk actions UI (elastic#77690) [Metrics UI] Add inventory view timeline (elastic#77804) Reporting/Docs: Updates for setting to enable CSV Download (elastic#78101) Update to latest rum-react (elastic#78193) ...

* Index pattern - refactor constructor (#77791)

mattkime added 30 commits August 29, 2020 22:22

index pattern creator take spec

e679933

partial progress

cc5884a

simpler IndexPatterns.get

76af115

partial progress

dd611ed

Merge branch 'master' into index_pattern_class_constructor_refactor

ae40414

fix field spec

0b6e64d

fix field spec

262e183

fix index pattern creation

be88df3

Merge branch 'master' into index_pattern_class_constructor_refactor

574fce6

fix types and update docs

ff971a0

update docs

5084496

refresh fields before creating index pattern

010a70f

update docs

9c0f0f7

Merge branch 'master' into index_pattern_class_constructor_refactor

c4f5e67

update docs

00dfeff

Merge branch 'master' into index_pattern_class_constructor_refactor

33d7860

functional test fixes

40bfe83

Merge branch 'master' into index_pattern_class_constructor_refactor

b8c79f9

separate create and save

a46205a

update docs, fix a couple of tests

ec00ce5

update docs

275b152

fix file upload indexing service

6bec815

attempt to fix ml and maps tests

e7920b3

fix index pattern create

fc57286

refactor index patterns methods

c4d9b70

fix tests

a8d0c36

remove comment

b4bcfa8

Merge branch 'master' into index_pattern_class_constructor_refactor

317cfb7

fix saved object import

f046894

fixed jest test

7dc3e8c

mattkime requested a review from a team September 21, 2020 04:14

update docs

3f9e4e5

ppisljar approved these changes Sep 21, 2020

View reviewed changes

mshustov approved these changes Sep 21, 2020

View reviewed changes

peteharverson reviewed Sep 21, 2020

View reviewed changes

...lugins/ml/public/application/datavisualizer/file_based/components/import_view/import_view.js Show resolved Hide resolved

peteharverson reviewed Sep 21, 2020

View reviewed changes

...tics/pages/analytics_management/hooks/use_create_analytics_form/use_create_analytics_form.ts Outdated Show resolved Hide resolved

mattkime added 2 commits September 21, 2020 11:12

Merge branch 'master' into index_pattern_refactor_constructor

63838e1

fix file import

dd2caff

mattkime commented Sep 21, 2020

View reviewed changes

...lugins/ml/public/application/datavisualizer/file_based/components/import_view/import_view.js Show resolved Hide resolved

add docs

5b3d104

kertal approved these changes Sep 22, 2020

View reviewed changes

mattkime added 2 commits September 22, 2020 10:15

Merge branch 'master' into index_pattern_refactor_constructor

28ff17b

don't load field list for data frame analytics

59ec324

alvarezmelissa87 approved these changes Sep 22, 2020

View reviewed changes

alvarezmelissa87 mentioned this pull request Sep 22, 2020

[ML] DF Analytics creation wizard: ensure data available before creation index pattern for job #78172

Closed

remove todo

498637b

lukeelmers approved these changes Sep 22, 2020

View reviewed changes

mattkime added 4 commits September 22, 2020 16:44

catch on field refresh

e3b5407

remove comment

ae0d9ee

fix return type

305f3f8

Merge branch 'master' into index_pattern_refactor_constructor

569e00f

mattkime merged commit 9450248 into elastic:master Sep 23, 2020

mattkime mentioned this pull request Sep 23, 2020

[7.x] Index pattern - refactor constructor (#77791) #78246

Merged

mattkime added a commit that referenced this pull request Sep 23, 2020

[7.x] Index pattern - refactor constructor (#77791) (#78246)

5d665a9

* Index pattern - refactor constructor (#77791)

Conversation

mattkime commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Other changes

Checklist

For maintainers

Dev Docs

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

mshustov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 23, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mattkime commented Sep 17, 2020 •

edited

Loading