[EPM] Adding support for nested fields #64829
Conversation
jonathan-buttner
added
v8.0.0
release_note:skip
|
Pinging @elastic/ingest-management (Feature:EPM) |
| // 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' && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good, here's the issue: #64901
| @@ -28,6 +28,8 @@ export interface Field { | |||
| object_type?: string; | |||
| scaling_factor?: number; | |||
| dynamic?: 'strict' | boolean; | |||
| include_in_parent?: boolean; | |||
| include_in_root?: boolean; | |||
There was a problem hiding this comment.
| @@ -72,6 +72,12 @@ export function generateMappings(fields: Field[]): IndexTemplateMappings { | |||
| switch (type) { | |||
| case 'group': | |||
| fieldProps = generateMappings(field.fields!); | |||
| attemptAddDynamicAndEnabled(fieldProps, field); | |||
There was a problem hiding this comment.
👍 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.
There was a problem hiding this comment.
good point, I updated to keep it similar with the other cases.
There was a problem hiding this comment.
Thank you for adding this! I have one request about not mutating function input, see inline comment.
Apart from that this looks good to me -- we'll probably want to revisit dedupFields() once we found out all the other things it needs to do, but we don't have to do it now.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Allowing nested types to be merged with group * Adding nested to case and handling other fields * Cleaing up if logic * Removing unneeded if statement * Adding nested type to switch and more tests * Keeping functions immutable Co-authored-by: Elastic Machine <[email protected]>
This PR adds support for a couple things:
groupdynamic,enabled, etc) to be placed on agrouptype if it was merged with a top levelobjectornestedtypeTesting
I added more tests to cover the additional cases with
nestedtypes.