Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EPM] Adding support for nested fields #64829

Merged
merged 6 commits into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -291,10 +291,12 @@ test('tests processing object field with property, reverse order', () => {
type: keyword
- name: a
type: object
dynamic: false
`;
const objectFieldWithPropertyReversedMapping = {
properties: {
a: {
dynamic: false,
properties: {
b: {
ignore_above: 1024,
Expand All @@ -307,5 +309,61 @@ test('tests processing object field with property, reverse order', () => {
const fields: Field[] = safeLoad(objectFieldWithPropertyReversedLiteralYml);
const processedFields = processFields(fields);
const mappings = generateMappings(processedFields);
expect(JSON.stringify(mappings)).toEqual(JSON.stringify(objectFieldWithPropertyReversedMapping));
expect(mappings).toEqual(objectFieldWithPropertyReversedMapping);
});

test('tests processing nested field with property', () => {
const nestedYaml = `
- name: a.b
type: keyword
- name: a
type: nested
dynamic: false
`;
const expectedMapping = {
properties: {
a: {
dynamic: false,
type: 'nested',
properties: {
b: {
ignore_above: 1024,
type: 'keyword',
},
},
},
},
};
const fields: Field[] = safeLoad(nestedYaml);
const processedFields = processFields(fields);
const mappings = generateMappings(processedFields);
expect(mappings).toEqual(expectedMapping);
});

test('tests processing nested field with property, nested field first', () => {
const nestedYaml = `
- name: a
type: nested
include_in_parent: true
- name: a.b
type: keyword
`;
const expectedMapping = {
properties: {
a: {
include_in_parent: true,
type: 'nested',
properties: {
b: {
ignore_above: 1024,
type: 'keyword',
},
},
},
},
};
const fields: Field[] = safeLoad(nestedYaml);
const processedFields = processFields(fields);
const mappings = generateMappings(processedFields);
expect(mappings).toEqual(expectedMapping);
});
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,19 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings {
switch (type) {
case 'group':
fieldProps = generateMappings(field.fields!);
attemptAddDynamicAndEnabled(fieldProps, field);
Copy link
Contributor

@skh skh Apr 30, 2020

Choose a reason for hiding this comment

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

👍 for factoring this out into a separate helper function.

Could we not mutate the input parameter fieldProps in these calls? In other places we return a (mutated) copy, so that line would read

fieldProps = attemptAddDynamicAndEnabled(fieldProps, field);

and it is more readable when we stay consistent throughout the code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I updated to keep it similar with the other cases.

break;
case 'group-nested':
fieldProps = generateMappings(field.fields!);
fieldProps.type = 'nested';
attemptAddDynamicAndEnabled(fieldProps, field);

if (field.hasOwnProperty('include_in_parent')) {
fieldProps.include_in_parent = field.include_in_parent;
}
if (field.hasOwnProperty('include_in_root')) {
fieldProps.include_in_root = field.include_in_root;
}
break;
case 'integer':
fieldProps.type = 'long';
Expand All @@ -96,12 +109,7 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings {
break;
case 'object':
fieldProps.type = 'object';
if (field.hasOwnProperty('enabled')) {
fieldProps.enabled = field.enabled;
}
if (field.hasOwnProperty('dynamic')) {
fieldProps.dynamic = field.dynamic;
}
attemptAddDynamicAndEnabled(fieldProps, field);
break;
case 'array':
// this assumes array fields were validated in an earlier step
Expand All @@ -128,6 +136,15 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings {
return { properties: props };
}

function attemptAddDynamicAndEnabled(props: Properties, field: Field) {
if (field.hasOwnProperty('enabled')) {
props.enabled = field.enabled;
}
if (field.hasOwnProperty('dynamic')) {
props.dynamic = field.dynamic;
}
}

function generateMultiFields(fields: Fields): MultiFields {
const multiFields: MultiFields = {};
if (fields) {
Expand Down
162 changes: 162 additions & 0 deletions x-pack/plugins/ingest_manager/server/services/epm/fields/field.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,166 @@ describe('processFields', () => {
JSON.stringify(objectFieldWithPropertyExpanded)
);
});

test('correctly handles properties of object type fields where object comes second', () => {
const nested = [
{
name: 'a.b',
type: 'keyword',
},
{
name: 'a',
type: 'object',
dynamic: true,
},
];

const nestedExpanded = [
{
name: 'a',
type: 'group',
dynamic: true,
fields: [
{
name: 'b',
type: 'keyword',
},
],
},
];
expect(processFields(nested)).toEqual(nestedExpanded);
});

test('correctly handles properties of nested type fields', () => {
const nested = [
{
name: 'a',
type: 'nested',
dynamic: true,
},
{
name: 'a.b',
type: 'keyword',
},
];

const nestedExpanded = [
{
name: 'a',
type: 'group-nested',
dynamic: true,
fields: [
{
name: 'b',
type: 'keyword',
},
],
},
];
expect(processFields(nested)).toEqual(nestedExpanded);
});

test('correctly handles properties of nested type where nested top level comes second', () => {
const nested = [
{
name: 'a.b',
type: 'keyword',
},
{
name: 'a',
type: 'nested',
dynamic: true,
},
];

const nestedExpanded = [
{
name: 'a',
type: 'group-nested',
dynamic: true,
fields: [
{
name: 'b',
type: 'keyword',
},
],
},
];
expect(processFields(nested)).toEqual(nestedExpanded);
});

test('ignores redefinitions of an object field', () => {
const object = [
{
name: 'a',
type: 'object',
dynamic: true,
},
{
name: 'a',
type: 'object',
dynamic: false,
},
];

const objectExpected = [
{
name: 'a',
type: 'object',
// should preserve the field that was parsed first which had dynamic: true
dynamic: true,
},
];
expect(processFields(object)).toEqual(objectExpected);
});

test('ignores redefinitions of a nested field', () => {
const nested = [
{
name: 'a',
type: 'nested',
dynamic: true,
},
{
name: 'a',
type: 'nested',
dynamic: false,
},
];

const nestedExpected = [
{
name: 'a',
type: 'nested',
// should preserve the field that was parsed first which had dynamic: true
dynamic: true,
},
];
expect(processFields(nested)).toEqual(nestedExpected);
});

test('ignores redefinitions of a nested and object field', () => {
const nested = [
{
name: 'a',
type: 'nested',
dynamic: true,
},
{
name: 'a',
type: 'object',
dynamic: false,
},
];

const nestedExpected = [
{
name: 'a',
type: 'nested',
// should preserve the field that was parsed first which had dynamic: true
dynamic: true,
},
];
expect(processFields(nested)).toEqual(nestedExpected);
});
});
54 changes: 46 additions & 8 deletions x-pack/plugins/ingest_manager/server/services/epm/fields/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export interface Field {
object_type?: string;
scaling_factor?: number;
dynamic?: 'strict' | boolean;
include_in_parent?: boolean;
include_in_root?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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


// Kibana specific
analyzed?: boolean;
Expand Down Expand Up @@ -108,18 +110,54 @@ function dedupFields(fields: Fields): Fields {
return f.name === field.name;
});
if (found) {
// remove name, type, and fields from `field` variable so we avoid merging them into `found`
const { name, type, fields: nestedFields, ...importantFieldProps } = field;
/**
* There are a couple scenarios this if is trying to account for:
* Example 1
* - name: a.b
* - name: a
* In this scenario found will be `group` and field could be either `object` or `nested`
* Example 2
* - name: a
* - name: a.b
* In this scenario found could be `object` or `nested` and field will be group
*/
if (
(found.type === 'group' || found.type === 'object') &&
field.type === 'group' &&
field.fields
// only merge if found is a group and field is object, nested, or group.
// Or if found is object, or nested, and field is a group.
// This is to avoid merging two objects, or nested, or object with a nested.
(found.type === 'group' &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skh @ruflin let me know if you have suggestions for cleaning this logic up.

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, I'm fine with it, especially with all the comments you added.

I'd suggest adding an issue in the kibana repo for beta1 to revisit dedupFields() after alpha1, fully specify it's behavior (see also elastic/package-registry#340 ) and verify that it handles all edge cases correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, here's the issue: #64901

(field.type === 'object' || field.type === 'nested' || field.type === 'group')) ||
((found.type === 'object' || found.type === 'nested') && field.type === 'group')
) {
if (!found.fields) {
found.fields = [];
// if the new field has properties let's dedup and concat them with the already existing found variable in
// the array
if (field.fields) {
// if the found type was object or nested it won't have a fields array so let's initialize it
if (!found.fields) {
found.fields = [];
}
found.fields = dedupFields(found.fields.concat(field.fields));
}
found.type = 'group';
found.fields = dedupFields(found.fields.concat(field.fields));

// if found already had fields or got new ones from the new field coming in we need to assign the right
// type to it
if (found.fields) {
// If this field is supposed to be `nested` and we have fields, we need to preserve the fact that it is
// supposed to be `nested` for when the template is actually generated
if (found.type === 'nested' || field.type === 'nested') {
found.type = 'group-nested';
} else {
// found was either `group` already or `object` so just set it to `group`
found.type = 'group';
}
}
// we need to merge in other properties (like `dynamic`) that might exist
Object.assign(found, importantFieldProps);
// if `field.type` wasn't group object or nested, then there's a conflict in types, so lets ignore it
} else {
// only 'group' fields can be merged in this way
// only `group`, `object`, and `nested` fields can be merged in this way
// XXX: don't abort on error for now
// see discussion in https://github.com/elastic/kibana/pull/59894
// throw new Error(
Expand Down