[Data View Field] Fix popularity score bugs (#211201)

- Fixes https://github.com/elastic/kibana/issues/211109

## Summary

This PR fixes a number of bugs in fields popularity logic:
- [x] If field popularity was customized via UI form, the value will be
saved now as a number instead of a string
- [x] Same for runtime fields in another part of the code
- [x] Since the data was polluted with string values, this PR makes sure
that the incrementing would still work and the result would be converted
to number.
- [x] If user opened the field flyout, when selected/deselected fields
as columns in the table, then opened the field flyout again, the data
shown as Popularity was outdated. Now it should be fixed.
- [x] Prevents reseting of Popularity scores in other fields.
- [x] Functional tests in
`test/functional/apps/discover/group6/_sidebar.ts` and
`test/functional/apps/management/data_views/_index_pattern_popularity.ts`.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

src/platform/packages/shared/kbn-unified-data-table/src/utils/popularize_field.test.ts

@@ -81,6 +81,31 @@ describe('Popularize field', () => {
     expect(field.count).toEqual(1);
   });
 
+  test('should increment', async () => {
+    const field = {
+      count: 5,
+    };
+    const dataView = {
+      id: 'id',
+      fields: {
+        getByName: () => field,
+      },
+      setFieldCount: jest.fn().mockImplementation((fieldName, count) => {
+        field.count = count;
+      }),
+      isPersisted: () => true,
+    } as unknown as DataView;
+    const fieldName = '@timestamp';
+    const updateSavedObjectMock = jest.fn();
+    const dataViewsService = {
+      updateSavedObject: updateSavedObjectMock,
+    } as unknown as DataViewsContract;
+    const result = await popularizeField(dataView, fieldName, dataViewsService, capabilities);
+    expect(result).toBeUndefined();
+    expect(updateSavedObjectMock).toHaveBeenCalled();
+    expect(field.count).toEqual(6);
+  });
+
   test('hides errors', async () => {
     const field = {
       count: 0,

src/platform/plugins/shared/data_view_field_editor/__jest__/client_integration/field_editor_flyout_content.test.ts

@@ -62,6 +62,7 @@ describe('<FieldEditorFlyoutContent />', () => {
       script: { source: 'emit(true)' },
       customLabel: 'cool demo test field',
       format: { id: 'boolean' },
+      popularity: 1,
     };
 
     const { find } = await setup({ fieldToCreate });
@@ -70,6 +71,7 @@ describe('<FieldEditorFlyoutContent />', () => {
     expect(find('nameField.input').props().value).toBe(fieldToCreate.name);
     expect(find('typeField').props().value).toBe(fieldToCreate.type);
     expect(find('scriptField').props().value).toBe(fieldToCreate.script.source);
+    expect(find('editorFieldCount').props().value).toBe(fieldToCreate.popularity.toString());
   });
 
   test('should accept an "onSave" prop', async () => {
@@ -172,6 +174,26 @@ describe('<FieldEditorFlyoutContent />', () => {
         script: { source: 'echo("hello")' },
         format: null,
       });
+
+      await toggleFormRow('popularity');
+      await fields.updatePopularity('5');
+
+      await waitForUpdates();
+
+      await act(async () => {
+        find('fieldSaveButton').simulate('click');
+        jest.advanceTimersByTime(0); // advance timers to allow the form to validate
+      });
+
+      fieldReturned = onSave.mock.calls[onSave.mock.calls.length - 1][0];
+
+      expect(fieldReturned).toEqual({
+        name: 'someName',
+        type: 'date',
+        script: { source: 'echo("hello")' },
+        format: null,
+        popularity: 5,
+      });
     });
 
     test('should not block validation if no documents could be fetched from server', async () => {

src/platform/plugins/shared/data_view_field_editor/__jest__/client_integration/helpers/common_actions.ts

@@ -42,7 +42,7 @@ export const waitForUpdates = async (testBed?: TestBed) => {
 
 export const getCommonActions = (testBed: TestBed) => {
   const toggleFormRow = async (
-    row: 'customLabel' | 'customDescription' | 'value' | 'format',
+    row: 'customLabel' | 'customDescription' | 'value' | 'format' | 'popularity',
     value: 'on' | 'off' = 'on'
   ) => {
     const testSubj = `${row}Row.toggle`;
@@ -102,6 +102,15 @@ export const getCommonActions = (testBed: TestBed) => {
     testBed.component.update();
   };
 
+  const updatePopularity = async (value: string) => {
+    await act(async () => {
+      testBed.form.setInputValue('editorFieldCount', value);
+      jest.advanceTimersByTime(0); // advance timers to allow the form to validate
+    });
+
+    testBed.component.update();
+  };
+
   const getScriptError = () => {
     const scriptError = testBed.component.find('#runtimeFieldScript-error-0');
 
@@ -123,6 +132,7 @@ export const getCommonActions = (testBed: TestBed) => {
       updateType,
       updateScript,
       updateFormat,
+      updatePopularity,
       getScriptError,
     },
   };

src/platform/plugins/shared/data_view_field_editor/public/components/field_editor/field_editor.tsx

@@ -43,9 +43,11 @@ export interface FieldEditorFormState {
   submit: FormHook<Field>['submit'];
 }
 
-export interface FieldFormInternal extends Omit<Field, 'type' | 'internalType' | 'fields'> {
+export interface FieldFormInternal
+  extends Omit<Field, 'type' | 'internalType' | 'fields' | 'popularity'> {
   fields?: Record<string, { type: RuntimePrimitiveTypes }>;
   type: TypeSelection;
+  popularity?: string;
   __meta__: {
     isCustomLabelVisible: boolean;
     isCustomDescriptionVisible: boolean;
@@ -84,6 +86,7 @@ const formDeserializer = (field: Field): FieldFormInternal => {
 
   return {
     ...field,
+    popularity: typeof field.popularity === 'number' ? String(field.popularity) : field.popularity,
     type: fieldType,
     format,
     __meta__: {
@@ -97,12 +100,15 @@ const formDeserializer = (field: Field): FieldFormInternal => {
 };
 
 const formSerializer = (field: FieldFormInternal): Field => {
-  const { __meta__, type, format, ...rest } = field;
+  const { __meta__, type, format, popularity, ...rest } = field;
+
   return {
     type: type && type[0].value!,
     // By passing "null" we are explicitly telling DataView to remove the
     // format if there is one defined for the field.
     format: format === undefined ? null : format,
+    // convert from the input string value into a number
+    popularity: typeof popularity === 'string' ? Number(popularity) || 0 : popularity,
     ...rest,
   };
 };

src/platform/plugins/shared/data_view_field_editor/public/open_editor.tsx

@@ -130,10 +130,17 @@ export const getFieldEditorOpener =
         };
       };
 
-      const dataViewLazy =
-        dataViewLazyOrNot instanceof DataViewLazy
-          ? dataViewLazyOrNot
-          : await dataViews.toDataViewLazy(dataViewLazyOrNot);
+      let dataViewLazy: DataViewLazy;
+
+      if (dataViewLazyOrNot instanceof DataViewLazy) {
+        dataViewLazy = dataViewLazyOrNot;
+      } else {
+        if (dataViewLazyOrNot.id) {
+          // force cache reset to have the latest field attributes
+          dataViews.clearDataViewLazyCache(dataViewLazyOrNot.id);
+        }
+        dataViewLazy = await dataViews.toDataViewLazy(dataViewLazyOrNot);
+      }
 
       const dataViewField = fieldNameToEdit
         ? (await dataViewLazy.getFieldByName(fieldNameToEdit, true)) ||

src/platform/plugins/shared/data_views/common/data_views/__snapshots__/data_views.test.ts.snap

@@ -21,6 +21,25 @@ FldList [
     "subType": undefined,
     "type": "string",
   },
+  Object {
+    "aggregatable": true,
+    "conflictDescriptions": undefined,
+    "count": 50,
+    "customDescription": undefined,
+    "customLabel": undefined,
+    "defaultFormatter": undefined,
+    "esTypes": Array [
+      "keyword",
+    ],
+    "lang": undefined,
+    "name": "wrongCountType",
+    "readFromDocValues": false,
+    "script": undefined,
+    "scripted": false,
+    "searchable": true,
+    "subType": undefined,
+    "type": "string",
+  },
 ]
 `;
 
@@ -39,6 +58,9 @@ Object {
       "count": 5,
       "customLabel": "A Runtime Field",
     },
+    "wrongCountType": Object {
+      "count": 50,
+    },
   },
   "fieldFormats": Object {
     "field": Object {},
@@ -54,6 +76,12 @@ Object {
       },
       "type": "keyword",
     },
+    "wrongCountType": Object {
+      "script": Object {
+        "source": "emit('test')",
+      },
+      "type": "keyword",
+    },
   },
   "sourceFilters": Array [
     Object {

src/platform/plugins/shared/data_views/common/data_views/abstract_data_views.ts

@@ -365,8 +365,15 @@ export abstract class AbstractDataView {
   getAsSavedObjectBody(): DataViewAttributes {
     const stringifyOrUndefined = (obj: any) => (obj ? JSON.stringify(obj) : undefined);
 
+    const fieldAttrsWithValues: Record<string, FieldAttrSet> = {};
+    this.fieldAttrs.forEach((attrs, fieldName) => {
+      if (Object.keys(attrs).length) {
+        fieldAttrsWithValues[fieldName] = attrs;
+      }
+    });
+
     return {
-      fieldAttrs: stringifyOrUndefined(Object.fromEntries(this.fieldAttrs.entries())),
+      fieldAttrs: stringifyOrUndefined(fieldAttrsWithValues),
       title: this.getIndexPattern(),
       timeFieldName: this.timeFieldName,
       sourceFilters: stringifyOrUndefined(this.sourceFilters),
@@ -382,8 +389,10 @@ export abstract class AbstractDataView {
   }
 
   protected toSpecShared(includeFields = true): DataViewSpec {
+    // `cloneDeep` is added to make sure that the original fieldAttrs map is not modified with the following `delete` operation.
+    const fieldAttrs = cloneDeep(Object.fromEntries(this.fieldAttrs.entries()));
+
     // if fields aren't included, don't include count
-    const fieldAttrs = Object.fromEntries(this.fieldAttrs.entries());
     if (!includeFields) {
       Object.keys(fieldAttrs).forEach((key) => {
         delete fieldAttrs[key].count;

src/platform/plugins/shared/data_views/common/data_views/data_view.test.ts

@@ -396,6 +396,22 @@ describe('IndexPattern', () => {
       indexPattern.removeRuntimeField(newField);
     });
 
+    test('add and remove a popularity score from a runtime field', () => {
+      const newField = 'new_field_test';
+      indexPattern.addRuntimeField(newField, {
+        ...runtimeWithAttrs,
+        popularity: 10,
+      });
+      expect(indexPattern.getFieldByName(newField)?.count).toEqual(10);
+      indexPattern.setFieldCount(newField, 20);
+      expect(indexPattern.getFieldByName(newField)?.count).toEqual(20);
+      indexPattern.setFieldCount(newField, null);
+      expect(indexPattern.getFieldByName(newField)?.count).toEqual(0);
+      indexPattern.setFieldCount(newField, undefined);
+      expect(indexPattern.getFieldByName(newField)?.count).toEqual(0);
+      indexPattern.removeRuntimeField(newField);
+    });
+
     test('add and remove composite runtime field as new fields', () => {
       const fieldCount = indexPattern.fields.length;
       indexPattern.addRuntimeField('new_field', runtimeCompositeWithAttrs);
@@ -505,6 +521,19 @@ describe('IndexPattern', () => {
       const dataView2 = create('test2', spec);
       expect(dataView1.sourceFilters).not.toBe(dataView2.sourceFilters);
     });
+
+    test('getting spec without fields does not modify fieldAttrs', () => {
+      const fieldAttrs = { bytes: { count: 5, customLabel: 'test_bytes' }, agent: { count: 1 } };
+      const dataView = new DataView({
+        fieldFormats: fieldFormatsMock,
+        spec: {
+          fieldAttrs,
+        },
+      });
+      const spec = dataView.toSpec(false);
+      expect(spec.fieldAttrs).toEqual({ bytes: { customLabel: fieldAttrs.bytes.customLabel } });
+      expect(JSON.parse(dataView.getAsSavedObjectBody().fieldAttrs!)).toEqual(fieldAttrs);
+    });
   });
 
   describe('should initialize from spec with field attributes', () => {

src/platform/plugins/shared/data_views/common/data_views/data_view_lazy.test.ts

@@ -480,6 +480,23 @@ describe('DataViewLazy', () => {
       dataViewLazy.removeRuntimeField(newField);
     });
 
+    test('add and remove a popularity score from a runtime field', async () => {
+      const newField = 'new_field_test';
+      fieldCapsResponse = [];
+      dataViewLazy.addRuntimeField(newField, {
+        ...runtimeWithAttrs,
+        popularity: 10,
+      });
+      expect((await dataViewLazy.getFieldByName(newField))?.count).toEqual(10);
+      dataViewLazy.setFieldCount(newField, 20);
+      expect((await dataViewLazy.getFieldByName(newField))?.count).toEqual(20);
+      dataViewLazy.setFieldCount(newField, null);
+      expect((await dataViewLazy.getFieldByName(newField))?.count).toEqual(0);
+      dataViewLazy.setFieldCount(newField, undefined);
+      expect((await dataViewLazy.getFieldByName(newField))?.count).toEqual(0);
+      dataViewLazy.removeRuntimeField(newField);
+    });
+
     test('add and remove composite runtime field as new fields', async () => {
       const fieldMap = (await dataViewLazy.getFields({ fieldName: ['*'] })).getFieldMap();
       const fieldCount = Object.values(fieldMap).length;

src/platform/plugins/shared/data_views/common/data_views/data_views.test.ts

@@ -51,8 +51,9 @@ const savedObject = {
     typeMeta: '{}',
     type: '',
     runtimeFieldMap:
-      '{"aRuntimeField": { "type": "keyword", "script": {"source": "emit(\'hello\')"}}}',
-    fieldAttrs: '{"aRuntimeField": { "count": 5, "customLabel": "A Runtime Field"}}',
+      '{"aRuntimeField": { "type": "keyword", "script": {"source": "emit(\'hello\')"}}, "wrongCountType": { "type": "keyword", "script": {"source": "emit(\'test\')"}}}',
+    fieldAttrs:
+      '{"aRuntimeField": { "count": 5, "customLabel": "A Runtime Field" }, "wrongCountType": { "count": "50" }}',
   },
   type: 'index-pattern',
   references: [],
@@ -682,6 +683,23 @@ describe('IndexPatterns', () => {
     expect(attrs.fieldFormatMap).toMatchInlineSnapshot(`"{}"`);
   });
 
+  test('gets the correct field attrs', async () => {
+    const id = 'id';
+    setDocsourcePayload(id, savedObject);
+    const dataView = await indexPatterns.get(id);
+    expect(dataView.getFieldByName('aRuntimeField')).toEqual(
+      expect.objectContaining({
+        count: 5,
+        customLabel: 'A Runtime Field',
+      })
+    );
+    expect(dataView.getFieldByName('wrongCountType')).toEqual(
+      expect.objectContaining({
+        count: 50,
+      })
+    );
+  });
+
   describe('defaultDataViewExists', () => {
     beforeEach(() => {
       indexPatterns.clearCache();

src/platform/plugins/shared/data_views/common/data_views/data_views.ts

@@ -147,6 +147,11 @@ export interface DataViewsServicePublicMethods {
    */
   clearInstanceCache: (id?: string) => void;
 
+  /**
+   * Clear the cache of lazy data view instances.
+   */
+  clearDataViewLazyCache: (id: string) => void;
+
   /**
    * Create data view based on the provided spec.
    * @param spec - Data view spec.
@@ -523,6 +528,13 @@ export class DataViewsService {
     }
   };
 
+  /**
+   * Clear instance in data view lazy cache
+   */
+  clearDataViewLazyCache = (id: string) => {
+    this.dataViewLazyCache.delete(id);
+  };
+
   /**
    * Get cache, contains data view saved objects.
    */
@@ -821,6 +833,16 @@ export class DataViewsService {
       ? JSON.parse(runtimeFieldMap)
       : {};
 
+    if (parsedFieldAttrs) {
+      Object.keys(parsedFieldAttrs).forEach((fieldName) => {
+        const parsedFieldAttr = parsedFieldAttrs?.[fieldName];
+        // Because of https://github.com/elastic/kibana/issues/211109 bug, the persisted "count" data can be polluted and have string type.
+        if (parsedFieldAttr && typeof parsedFieldAttr.count === 'string') {
+          parsedFieldAttr.count = Number(parsedFieldAttr.count) || 0;
+        }
+      });
+    }
+
     return {
       id,
       version,
@@ -893,9 +915,6 @@ export class DataViewsService {
     refreshFields: boolean = false
   ): Promise<DataView> => {
     const spec = this.savedObjectToSpec(savedObject);
-    spec.fieldAttrs = savedObject.attributes.fieldAttrs
-      ? JSON.parse(savedObject.attributes.fieldAttrs)
-      : {};
 
     let fields: Record<string, FieldSpec> = {};
     let indices: string[] = [];

test/functional/apps/discover/group6/_sidebar.ts

@@ -343,6 +343,32 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         await expectFieldListDescription(
           '3 selected fields. 3 popular fields. 48 available fields. 5 empty fields. 4 meta fields.'
         );
+
+        await unifiedFieldList.clickFieldListItemRemove('@message');
+        await discover.waitUntilSearchingHasFinished();
+        await unifiedFieldList.clickFieldListItemRemove('extension');
+        await discover.waitUntilSearchingHasFinished();
+        await discover.addRuntimeField('test', `emit('test')`, undefined, 30);
+        await discover.waitUntilSearchingHasFinished();
+
+        expect((await unifiedFieldList.getSidebarSectionFieldNames('popular')).join(', ')).to.be(
+          'test, @message, extension, _id'
+        );
+
+        await expectFieldListDescription(
+          '1 selected field. 4 popular fields. 49 available fields. 5 empty fields. 4 meta fields.'
+        );
+
+        await unifiedFieldList.clickFieldListItemAdd('bytes');
+        await discover.waitUntilSearchingHasFinished();
+
+        expect((await unifiedFieldList.getSidebarSectionFieldNames('popular')).join(', ')).to.be(
+          'test, @message, extension, _id, bytes'
+        );
+
+        await expectFieldListDescription(
+          '2 selected fields. 5 popular fields. 49 available fields. 5 empty fields. 4 meta fields.'
+        );
       });
 
       it('should show selected and available fields in ES|QL mode', async function () {

test/functional/apps/management/data_views/_index_pattern_popularity.ts

@@ -66,5 +66,25 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       log.debug('popularity = ' + popularity);
       expect(popularity).to.be('1');
     });
+
+    it('changing popularity for one field does not affect the other', async function () {
+      expect(await PageObjects.settings.getPopularity()).to.be('1');
+      await PageObjects.settings.setPopularity(5);
+      await PageObjects.settings.controlChangeSave();
+
+      await PageObjects.settings.openControlsByName('bytes');
+      expect(await PageObjects.settings.getPopularity()).to.be('0');
+      await testSubjects.click('toggleAdvancedSetting');
+      await PageObjects.settings.setPopularity(7);
+      await PageObjects.settings.controlChangeSave();
+
+      await browser.refresh();
+
+      await PageObjects.settings.openControlsByName('geo.coordinates');
+      expect(await PageObjects.settings.getPopularity()).to.be('5');
+      await PageObjects.settings.closeIndexPatternFieldEditor();
+      await PageObjects.settings.openControlsByName('bytes');
+      expect(await PageObjects.settings.getPopularity()).to.be('7');
+    });
   }); // end 'change popularity'
 }

test/functional/page_objects/discover_page.ts

@@ -690,7 +690,7 @@ export class DiscoverPageObject extends FtrService {
     }
   }
 
-  public async addRuntimeField(name: string, script: string, type?: string) {
+  public async addRuntimeField(name: string, script: string, type?: string, popularity?: number) {
     await this.dataViews.clickAddFieldFromSearchBar();
     await this.fieldEditor.setName(name);
     if (type) {
@@ -698,6 +698,9 @@ export class DiscoverPageObject extends FtrService {
     }
     await this.fieldEditor.enableValue();
     await this.fieldEditor.typeScript(script);
+    if (popularity) {
+      await this.fieldEditor.setPopularity(popularity);
+    }
     await this.fieldEditor.save();
     await this.header.waitUntilLoadingHasFinished();
   }

test/functional/page_objects/settings_page.ts

@@ -412,8 +412,14 @@ export class SettingsPageObject extends FtrService {
     });
   }
 
+  async setPopularity(value: number) {
+    await this.testSubjects.setValue('editorFieldCount', String(value), {
+      clearWithKeyboard: true,
+    });
+  }
+
   async increasePopularity() {
-    await this.testSubjects.setValue('editorFieldCount', '1', { clearWithKeyboard: true });
+    await this.setPopularity(Number(await this.getPopularity()) + 1);
   }
 
   async getPopularity() {

test/functional/services/field_editor.ts

@@ -34,6 +34,13 @@ export class FieldEditorService extends FtrService {
   public async setCustomDescription(description: string) {
     await this.testSubjects.setValue('customDescriptionRow > input', description);
   }
+  public async setPopularity(value: number) {
+    await this.testSubjects.click('toggleAdvancedSetting');
+    await this.testSubjects.setEuiSwitch('popularityRow > toggle', 'check');
+    await this.testSubjects.setValue('editorFieldCount', String(value), {
+      clearWithKeyboard: true,
+    });
+  }
   public async getFormError() {
     const alert = await this.find.byCssSelector(
       '[data-test-subj=indexPatternFieldEditorForm] > [role="alert"]'