Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ export class DataView extends AbstractDataView implements DataViewBase {
if (existingField && existingField.isMapped) {
// mapped field, remove runtimeField def
existingField.runtimeField = undefined;
this.fields.clearSpecCache();
Comment on lines 342 to +343
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It sucks that we need to expose a public clearSpecCache method just for this case... But it also sucks that we do this by mutating the field (and that we expose mutable props on the field class in the first place). Kinda makes me feel like we should try sticking readonly on all the public field class props and run a CI build to see if we catch anywhere else in the codebase where we do this, since it could corrupt the cache value.

Thinking about it a bit more, wouldn't we need to invalidate the entire field list cache whenever any property of any field is modified? Most of them look to be mutable.

} else {
Object.values(this.getFieldsByRuntimeFieldName(name) || {}).forEach((field) => {
this.fields.remove(field);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,25 @@ describe('IndexPatterns', () => {
expect(savedObjectsClient.find).toHaveBeenCalledTimes(2);
});

test('caches fields toSpec calls', async () => {
const indexPatternSpec: DataViewSpec = {
runtimeFieldMap: {
a: {
type: 'keyword',
script: {
source: "emit('a');",
},
},
},
title: 'test',
};

const indexPattern = await indexPatterns.create(indexPatternSpec);
const firstSpec = indexPattern.fields.toSpec();
const secondSpec = indexPattern.fields.toSpec();
expect(firstSpec).toEqual(secondSpec);
Comment on lines +451 to +453
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One risk here is if there's anywhere in the codebase that retrieves and modifies a spec, it could corrupt the cache value. Maybe we should use defaultFreeze on it like what's done on responses in this PR, which will freeze it in dev and help catch these issues.

Do you know if CI runs in dev? I'd feel a bit more confident if we had a full CI run with freezing enabled in case it reveals any issues. Otherwise we might want to let it go through CI once with deepFreeze explicitly to see if it passes.

});

test('deletes the index pattern', async () => {
const id = '1';
const indexPattern = await indexPatterns.get(id);
Expand Down Expand Up @@ -936,5 +955,25 @@ describe('IndexPatterns', () => {
// @ts-expect-error
expect(apiClient.getFieldsForWildcard.mock.calls[0][0].allowNoIndex).toBe(true);
});

test('refreshFields should return a new fields spec instance', async () => {
const indexPatternSpec: DataViewSpec = {
runtimeFieldMap: {
a: {
type: 'keyword',
script: {
source: "emit('a');",
},
},
},
title: 'test',
};

const indexPattern = await indexPatterns.create(indexPatternSpec);
const originalFieldsSpec = indexPattern.fields.toSpec();

await indexPatterns.refreshFields(indexPattern);
expect(indexPattern.fields.toSpec()).not.toBe(originalFieldsSpec);
});
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice to actually have some tests for this now 👍

Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { fieldList, IIndexPatternFieldList } from './field_list';
import { DataViewField } from './data_view_field';
import type { FieldSpec } from '../types';

const baseField: FieldSpec = {
name: 'foo',
type: 'string',
esTypes: ['keyword'],
aggregatable: true,
searchable: true,
scripted: false,
};

const anotherField: FieldSpec = {
name: 'bar',
type: 'number',
esTypes: ['long'],
aggregatable: false,
searchable: false,
scripted: false,
count: 5,
customLabel: 'Bar Label',
customDescription: 'Bar Description',
conflictDescriptions: { idx1: ['type1', 'type2'] },
readFromDocValues: true,
indexed: true,
isMapped: true,
};

const scriptedField: FieldSpec = {
name: 'scripted',
type: 'number',
esTypes: ['long'],
aggregatable: false,
searchable: false,
scripted: true,
script: 'doc["bar"].value + 1',
lang: 'painless',
};

describe('fieldList', () => {
let list: IIndexPatternFieldList;

beforeEach(() => {
list = fieldList([baseField], false);
});

it('creates a field list with initial fields', () => {
expect(list.length).toBe(1);
expect(list[0]).toBeInstanceOf(DataViewField);
expect(list[0].name).toBe('foo');
expect(list[0].type).toBe('string');
expect(list[0].esTypes).toEqual(['keyword']);
expect(list[0].aggregatable).toBe(true);
expect(list[0].searchable).toBe(true);
expect(list[0].scripted).toBe(false);
});

it('creates a DataViewField instance with create()', () => {
const field = list.create(anotherField);
expect(field).toBeInstanceOf(DataViewField);
expect(field.name).toBe('bar');
expect(field.type).toBe('number');
expect(field.customLabel).toBe('Bar Label');
expect(field.customDescription).toBe('Bar Description');
expect(field.conflictDescriptions).toEqual({ idx1: ['type1', 'type2'] });
expect(field.count).toBe(5);
expect(field.readFromDocValues).toBe(true);
expect(field.isMapped).toBe(true);
});

it('adds a field with add()', () => {
const added = list.add(anotherField);
expect(list.length).toBe(2);
expect(list.getByName('bar')).toBe(added);
expect(list.getByType('number')).toContain(added);
expect(list.getByType('string')[0].name).toBe('foo');
});

it('gets all fields with getAll()', () => {
list.add(anotherField);
const all = list.getAll();
expect(all.length).toBe(2);
expect(all[0]).toBeInstanceOf(DataViewField);
expect(all[1]).toBeInstanceOf(DataViewField);
expect(all.map((f) => f.name)).toEqual(['foo', 'bar']);
});

it('gets a field by name with getByName()', () => {
expect(list.getByName('foo')).toBeInstanceOf(DataViewField);
expect(list.getByName('foo')?.name).toBe('foo');
expect(list.getByName('notfound')).toBeUndefined();
});

it('gets fields by type with getByType()', () => {
list.add(anotherField);
const stringFields = list.getByType('string');
expect(stringFields.length).toBe(1);
expect(stringFields[0].name).toBe('foo');
const numberFields = list.getByType('number');
expect(numberFields.length).toBe(1);
expect(numberFields[0].name).toBe('bar');
const notFound = list.getByType('boolean');
expect(notFound.length).toBe(0);
});

it('removes a field with remove()', () => {
const field = list.getByName('foo');
if (field) {
list.remove(field);
}
expect(list.length).toBe(0);
expect(list.getByName('foo')).toBeUndefined();
expect(list.getByType('string').length).toBe(0);
});

it('updates a field with update()', () => {
list.add(anotherField);
const updatedField: FieldSpec = {
...anotherField,
aggregatable: true,
name: 'bar',
customLabel: 'Updated Label',
};
list.update(updatedField);
const field = list.getByName('bar');
expect(field).toBeInstanceOf(DataViewField);
expect(field?.aggregatable).toBe(true);
expect(field?.customLabel).toBe('Updated Label');
});

it('removes all fields with removeAll()', () => {
list.add(anotherField);
list.removeAll();
expect(list.length).toBe(0);
expect(list.getAll().length).toBe(0);
expect(list.getByName('foo')).toBeUndefined();
expect(list.getByType('string').length).toBe(0);
});

it('replaces all fields with replaceAll()', () => {
list.replaceAll([anotherField, scriptedField]);
expect(list.length).toBe(2);
expect(list[0].name).toBe('bar');
expect(list[1].name).toBe('scripted');
expect(list.getByName('foo')).toBeUndefined();
expect(list.getByName('bar')).toBeInstanceOf(DataViewField);
expect(list.getByName('scripted')).toBeInstanceOf(DataViewField);
expect(list.getByName('scripted')?.scripted).toBe(true);
expect(list.getByName('scripted')?.script).toBe('doc["bar"].value + 1');
expect(list.getByName('scripted')?.lang).toBe('painless');
});

it('caches and clears toSpec()', () => {
const spy = jest.spyOn(list[0], 'toSpec');
// First call, not cached
const spec1 = list.toSpec();
expect(spec1.foo).toBeDefined();
expect(spy).toHaveBeenCalled();

// Second call, should be cached (no new calls to toSpec)
spy.mockClear();
const spec2 = list.toSpec();
expect(spec2.foo).toBeDefined();
expect(spy).not.toHaveBeenCalled();

// After add, cache should be cleared
list.add(anotherField);
const spec3 = list.toSpec();
expect(spec3.bar).toBeDefined();
});

it('calls toSpec with getFormatterForField', () => {
const getFormatterForField = jest.fn().mockReturnValue({ toJSON: () => ({ id: 'test' }) });
list.toSpec({ getFormatterForField });
expect(getFormatterForField).toHaveBeenCalledWith(list[0]);
});

it('handles remove on non-existent field gracefully', () => {
expect(list.length).toBe(1);
const fakeField = new DataViewField({ ...baseField, name: 'notfound' });
expect(() => list.remove(fakeField)).not.toThrow();
expect(list.length).toBe(1);
});

it('handles update on non-existent field gracefully', () => {
const fakeField: FieldSpec = { ...baseField, name: 'notfound' };
expect(() => list.update(fakeField)).not.toThrow();
expect(list.length).toBe(1);
});

it('handles replaceAll with empty array', () => {
list.replaceAll([]);
expect(list.length).toBe(0);
});

it('supports adding and updating a scripted field', () => {
list.add(scriptedField);
const field = list.getByName('scripted');
expect(field).toBeInstanceOf(DataViewField);
expect(field?.scripted).toBe(true);
expect(field?.script).toBe('doc["bar"].value + 1');
expect(field?.lang).toBe('painless');

// Update scripted field
const updatedScripted: FieldSpec = {
...scriptedField,
script: 'doc["bar"].value + 2',
lang: 'expression',
};
list.update(updatedScripted);
const updated = list.getByName('scripted');
expect(updated?.script).toBe('doc["bar"].value + 2');
expect(updated?.lang).toBe('expression');
});

it('does not throw if remove is called on a field not in the list', () => {
const field = new DataViewField({ ...baseField, name: 'notfound' });
expect(() => list.remove(field)).not.toThrow();
});

it('does not throw if update is called on a field not in the list', () => {
const field: FieldSpec = { ...baseField, name: 'notfound' };
expect(() => list.update(field)).not.toThrow();
});
Comment on lines +226 to +234
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are these duplicates of the above tests with "handles _ on non-existent field gracefully"?


it('preserves shortDotsEnable option', () => {
const shortDotsList = fieldList([baseField], true);
expect(shortDotsList[0].spec.shortDotsEnable).toBe(true);
});

it('does not fail if add is called with a field with the same name', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added, but not sure if this should be valid?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm hard to say, but I'd be hesitant to change it now 😅

list.add(baseField);
expect(list.length).toBe(2);
expect(list[0].name).toBe('foo');
expect(list[1].name).toBe('foo');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of questions to better understand the security use case:

  • Why does security use data view specs so heavily? Would it not work better to use actual data view instances instead, which are already cached by the data views service?
  • Would using specs that don't include the fields, e.g. toSpec(false) or toMinimalSpec() help at all here? I'm guessing not since presumably you want to keep the fields around?

Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ export interface IIndexPatternFieldList extends Array<DataViewField> {
* @returns a new data view field instance
*/
create(field: FieldSpec): DataViewField;

/**
* Clear the cached field spec map.
* This is used to avoid recalculating the spec every time `toSpec` is called.
*/
clearSpecCache(): void;
/**
* Add field to field list.
* @param field field spec to add field to list
Expand Down Expand Up @@ -97,6 +103,16 @@ export const fieldList = (
private removeByGroup = (field: DataViewField) =>
this.groups.get(field.type)?.delete(field.name);

/**
* @private
* @description
* This cache is used to store the result of the `toSpec` method, which converts the field list
* to a map of field specs by name. It is initialized to null and is cleared every time a field
* is added, removed, or updated. This is done to avoid recalculating the spec every time `toSpec`
* is called, which can be expensive if the field list is large.
*/
private cachedFieldSpec: DataViewFieldMap | null = null;

constructor() {
super();
specs.map((field) => this.add(field));
Expand All @@ -112,11 +128,16 @@ export const fieldList = (
return new DataViewField({ ...field, shortDotsEnable });
};

public clearSpecCache = () => {
this.cachedFieldSpec = null;
};

public readonly add = (field: FieldSpec): DataViewField => {
const newField = this.create(field);
this.push(newField);
this.setByName(newField);
this.setByGroup(newField);
this.clearSpecCache();
return newField;
};

Expand All @@ -125,7 +146,9 @@ export const fieldList = (
this.byName.delete(field.name);

const fieldIndex = findIndex(this, { name: field.name });
if (fieldIndex === -1) return;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if a name is provided not actually in the list, the mutation still took place.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait, so did this used to just always remove the last field in the list if passed a not-found field? 🤔

this.splice(fieldIndex, 1);
this.clearSpecCache();
};

public readonly update = (field: FieldSpec) => {
Expand All @@ -135,12 +158,14 @@ export const fieldList = (
this.setByName(newField);
this.removeByGroup(newField);
this.setByGroup(newField);
this.clearSpecCache();
};

public readonly removeAll = () => {
this.length = 0;
this.byName.clear();
this.groups.clear();
this.clearSpecCache();
};

public readonly replaceAll = (spcs: FieldSpec[] = []) => {
Expand All @@ -153,12 +178,17 @@ export const fieldList = (
}: {
getFormatterForField?: DataView['getFormatterForField'];
} = {}) {
return {
if (this.cachedFieldSpec) {
return this.cachedFieldSpec;
}
const toSpecResult = {
...this.reduce<DataViewFieldMap>((collector, field) => {
collector[field.name] = field.toSpec({ getFormatterForField });
return collector;
}, {}),
};
this.cachedFieldSpec = toSpecResult;
return toSpecResult;
}
}

Expand Down