Index pattern field class refactor #73180

mattkime · 2020-07-24T03:20:05Z

Summary

Better distinction and relationship between IndexPatternField and its spec
IndexPatternField class is no longer defined via object mutation
Reduction of dependencies
UI code moved into Index Pattern class (will be removed in next ticket)
IndexPattern field list was previously composed of IndexPatternFields or specs, now only IndexPatternFields
IndexPattern field list was previously redefined when loading fields, now only its contents are replaced.

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

elasticmachine · 2020-08-05T21:58:48Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

streamich

There seems to be a functional test failing related to this change. Other than that LGTM, checked on Mac/Ubuntu Chrome, all seems to be working.

streamich · 2020-08-06T08:33:08Z

src/plugins/data/common/index_patterns/fields/field_list.ts

-      this.groups.get(field.type)!.set(field.name, field);
-    };
-    private removeByGroup = (field: IFieldType) => this.groups.get(field.type)!.delete(field.name);
+export class FieldList extends Array<IndexPatternField> implements IIndexPatternFieldList {


I'm a bit concerned about extending the native Array here. Array has many public methods, are we sure we want to expose all of those as part of FieldList. If users use those, they can basically mutate the FieldList while by-passing the business logic of FieldList.

Maybe we could use composition instead?

class FieldList { private data: Array<IndexPatternField> = []; }

100% agree, but I'm avoiding changing the class interfaces in this PR. Will address in a follow up - #68020

streamich · 2020-08-06T08:34:51Z

src/plugins/data/common/index_patterns/fields/field_list.ts


-      specs.map((field) => this.add(field));
-    }
+  getByName = (name: IndexPatternField['name']) => this.byName.get(name);


All these could have readonly modifier.

Suggested change

getByName = (name: IndexPatternField['name']) => this.byName.get(name);

readonly getByName = (name: IndexPatternField['name']) => this.byName.get(name);

or

Suggested change

getByName = (name: IndexPatternField['name']) => this.byName.get(name);

public readonly getByName = (name: IndexPatternField['name']) => this.byName.get(name);

ppisljar · 2020-08-06T11:45:50Z

src/plugins/data/common/index_patterns/fields/index_pattern_field.ts

+    displayName: string,
+    onNotification: OnNotification
+  ) {
+    this.indexPattern = indexPattern;


i don't like the fact that we need to pass indexPattern in just to be able to get to formatter, could we:

remove format function and have consumers always use indexPattern.getFormatterForField(field) ?

or actually push that to the service, so we would do something like indexPatterns.getFormatter(indexPattern, field) and both IndexPattern and IndexPatternField get rid of that dependency (and can be static)

i am fine with doing this later, i am just wondering how reasonable you think that is ?

100% This PR keeps the class interfaces largely the same. My next PR will address this -#71909

walterra · 2020-08-06T12:49:03Z

I investigated the failing functional test in the transform plugin.

We use the Kibana index pattern field information to determine whether that field is suitable to be used for a preview visualization. Among other things we check against the aggregatable attribute.

On master, for the fields of the kibana e-commerce sample dataset we get the following:

name: category 	type: string 	aggregatable: false
name: currency 	type: string 	aggregatable: true
name: customer_birth_date 	type: date 	aggregatable: true
name: customer_first_name 	type: string 	aggregatable: false

When running this PR, we get:

name: category 	type: string 	aggregatable: undefined
name: currency 	type: string 	aggregatable: true
name: customer_birth_date 	type: date 	aggregatable: true
name: customer_first_name 	type: string 	aggregatable: undefined

Since in our code we do a strict equal check (field?.aggregatable === false) the code no longer works as expected.

The code causing this can be found here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/public/application/components/data_grid/common.ts#L157

Hope this helps, let me know if I can help out with more investigations.

mattkime · 2020-08-06T14:45:46Z

@walterra

Thanks! Extremely helpful!

ppisljar

code LGTM

kibanamachine · 2020-08-07T18:16:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 651fe8f

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	537	-1	538

async chunks size

id	value	diff	baseline
discover	431.6KB	+1.3KB	430.3KB
indexPatternManagement	698.1KB	+191.0B	697.9KB
total		+1.5KB

page load bundle size

id	value	diff	baseline
data	1.4MB	+609.0B	1.4MB
indexPatternManagement	133.3KB	+6.0B	133.3KB
total		+615.0B

History

💚 Build #66914 succeeded fbf3636

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

mattkime · 2020-08-09T16:43:46Z

@elasticmachine merge upstream

- Better distinction and relationship between IndexPatternField and its spec - IndexPatternField class is no longer defined via object mutation - Reduction of dependencies - UI code moved into Index Pattern class (will be removed in next ticket) - IndexPattern field list was previously composed of IndexPatternFields or specs, now only IndexPatternFields - IndexPattern field list was previously redefined when loading fields, now only its contents are replaced. # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.indexpattern.md # src/plugins/data/common/index_patterns/index_patterns/index_pattern.ts # src/plugins/data/public/index.ts

- Better distinction and relationship between IndexPatternField and its spec - IndexPatternField class is no longer defined via object mutation - Reduction of dependencies - UI code moved into Index Pattern class (will be removed in next ticket) - IndexPattern field list was previously composed of IndexPatternFields or specs, now only IndexPatternFields - IndexPattern field list was previously redefined when loading fields, now only its contents are replaced.

…-task * master: (42 commits) Allow any hostname for chromium proxy bypass (elastic#74693) [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533) [Metrics UI] Fix No Data preview pluralization (elastic#74399) [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688) Remove karma tests from legacy maps (elastic#74668) [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683) [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392) [visualizations] Add i18n translation for 'No results found' (elastic#74619) [maps] convert vector style properties to TS (elastic#74553) bump geckodriver binary to 0.27 (elastic#74638) fix: update apm agents to catch abort requests (elastic#74658) [Security Solution] Resolver children pagination (elastic#74603) add memoryStatus to df analytics page and analytics table in management (elastic#74570) [Ingest Manager] Allow prerelease in package version (elastic#74452) [App Arch]: remove legacy karma tests (elastic#74599) [i18n] revert reverted changes (elastic#74633) [Lens] Clear out all attribute properties before updating (elastic#74483) [Uptime] Fix full reloads while navigating to alert/ml (elastic#73796) Index pattern field class refactor (elastic#73180) [ML] Functional tests - stabilize DFA job type check (elastic#74631) ...

mattkime added 2 commits July 23, 2020 22:18

first pass at new field class

3d2f8cd

fix index_pattern class

ffd0372

elastic deleted a comment from kibanamachine Jul 24, 2020

fix index pattern stub

961b85a

elastic deleted a comment from kibanamachine Jul 24, 2020

mattkime added 8 commits July 24, 2020 07:01

update docs

da420e8

partial progress on tests

c05cffa

fix indexed_fields_table_test, field formatters management

b6a28b8

fix index pattern functional test

c265379

Merge branch 'master' into refactor_index_pattern_field_v2

e43572a

Merge branch 'master' into refactor_index_pattern_field_v2

244a6af

disable test for unused functionality

b82d623

Merge branch 'master' into refactor_index_pattern_field_v2

fdc4262

mattkime changed the title ~~first pass at new field class~~ Index pattern field class refactor Aug 4, 2020

mattkime mentioned this pull request Aug 4, 2020

[Discover] Allow custom label for fields via index pattern field management #70039

Merged

6 tasks

mattkime added 6 commits August 4, 2020 22:12

fix jest test, remove some comments

db0c0f2

Merge branch 'master' into refactor_index_pattern_field_v2

f8d78d6

fix jest test

60b6eca

Merge branch 'master' into refactor_index_pattern_field_v2

2d73d78

update docs

b787266

Merge branch 'master' into refactor_index_pattern_field_v2

e7d279b

elastic deleted a comment from kibanamachine Aug 5, 2020

mattkime added v8.0.0 v7.10.0 Team:AppArch Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 5, 2020

mattkime added this to In progress in kibana-app-arch via automation Aug 5, 2020

mattkime marked this pull request as ready for review August 5, 2020 21:58

mattkime requested a review from a team August 5, 2020 21:58

mattkime requested a review from a team as a code owner August 5, 2020 21:58

mattkime moved this from In progress to Review in progress in kibana-app-arch Aug 5, 2020

streamich approved these changes Aug 6, 2020

View reviewed changes

ppisljar reviewed Aug 6, 2020

View reviewed changes

only return boolean, not boolean or undefined

fbf3636

elastic deleted a comment from kibanamachine Aug 6, 2020

ppisljar approved these changes Aug 6, 2020

View reviewed changes

mattkime added 2 commits August 6, 2020 14:52

readonly for field list attrs

d6eb0a6

Merge branch 'master' into refactor_index_pattern_field_v2

651fe8f

mattkime merged commit cccf15a into elastic:master Aug 10, 2020

kibana-app-arch automation moved this from Review in progress to Done in current release Aug 10, 2020

mattkime mentioned this pull request Aug 10, 2020

[7.x] Index pattern field class refactor (#73180) #74653

Merged

This was referenced Aug 17, 2020

Fieldlist in Discover can break on unknown fields #75025

Closed

Remove dependencies from index pattern field and index pattern list #75185

Merged

mattkime mentioned this pull request Aug 24, 2020

Index Pattern API - clean up field and field list interfaces #68010

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index pattern field class refactor #73180

Index pattern field class refactor #73180

mattkime commented Jul 24, 2020 •

edited

Loading

elasticmachine commented Aug 5, 2020

streamich left a comment

streamich Aug 6, 2020

ppisljar Aug 6, 2020

mattkime Aug 6, 2020 •

edited

Loading

streamich Aug 6, 2020

ppisljar Aug 6, 2020

mattkime Aug 6, 2020 •

edited

Loading

walterra commented Aug 6, 2020

mattkime commented Aug 6, 2020

ppisljar left a comment

kibanamachine commented Aug 7, 2020

mattkime commented Aug 9, 2020

	getByName = (name: IndexPatternField['name']) => this.byName.get(name);
	readonly getByName = (name: IndexPatternField['name']) => this.byName.get(name);

	getByName = (name: IndexPatternField['name']) => this.byName.get(name);
	public readonly getByName = (name: IndexPatternField['name']) => this.byName.get(name);

Index pattern field class refactor #73180

Index pattern field class refactor #73180

Conversation

mattkime commented Jul 24, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 5, 2020

streamich left a comment

Choose a reason for hiding this comment

streamich Aug 6, 2020

Choose a reason for hiding this comment

ppisljar Aug 6, 2020

Choose a reason for hiding this comment

mattkime Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

streamich Aug 6, 2020

Choose a reason for hiding this comment

ppisljar Aug 6, 2020

Choose a reason for hiding this comment

mattkime Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

walterra commented Aug 6, 2020

mattkime commented Aug 6, 2020

ppisljar left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 7, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

mattkime commented Aug 9, 2020

mattkime commented Jul 24, 2020 •

edited

Loading

mattkime Aug 6, 2020 •

edited

Loading

mattkime Aug 6, 2020 •

edited

Loading