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

Add flexibility to the field set reuse mechanism #820

Closed
wants to merge 12 commits into from

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Apr 20, 2020

New possibilities:

  • nest as: lets us nest a field set as another name.
  • self-nesting: lets us nest a field set within itself.

Most of the time, self-nesting and "nest as" will be used together. Nothing currently prevents the nesting of a fieldset elsewhere as another name. But this should be used with caution, as it could lead to confusion.

Examples:

It's important to highlight one detail about self-nesting. Field sets like user that are also reused within other field sets (e.g. destination.user) and that will soon also have self-nesting (e.g. user.target, user.effective) will not carry the self-nesting in all other places (there won't be a destination.user.target).

In addition to improving the reuse mechanism, this PR replaces all of the process.parent.* manually duplicated fields. The process of removing this manual duplication exposed a few problems with the prior manual duplication, which are fixed by this PR:

  • Small differences had crept up between the process.* and process.parent.* fields.
  • The pe field set was nested process.pe but not process.parent.pe.
  • Not an issue per se, but since process.parent.* fields were all explicit before, they were all explicitly listed in the asciidoc field listing. They are now indicated only via the normal "field reuse" section at the bottom of the page.

This PR does not address #809. This will be done as a separate PR.

@webmat webmat self-assigned this Apr 20, 2020
@webmat webmat changed the title Add flexibility to field set reuse (nest as, and self-nesting) Add flexibility to the field set reuse mechanism (nest as, and self-nesting) Apr 20, 2020
@webmat webmat changed the title Add flexibility to the field set reuse mechanism (nest as, and self-nesting) Add flexibility to the field set reuse mechanism Apr 20, 2020
@webmat
Copy link
Contributor Author

webmat commented Apr 20, 2020

Ah I hadn't noticed the broken asciidoc links that cause the docs build to fail. I'll fix that shortly.

The rest can already be reviewed.

@webmat
Copy link
Contributor Author

webmat commented Apr 20, 2020

Ok this is ready for a review.

@@ -4630,6 +4373,10 @@ example: `/home/alice`

==== Field Reuse

The `process` fields are expected to be nested at: `process.parent`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought of ways to explain what each nesting means? I think we just copy and paste fieldsets over, but I don't know if we explain what the context of each nesting should mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new structure for how we specify field reuse will easily allow for that:

  reusable:
    top_level: true
    expected:
      - at: process
        as: parent
        description: The process that gave birth to the one detailed at the root of the event.

And I think it's very much needed. However I think we should tackle this independently, as this will be a significant endeavour to add all of these contextual definitions and redesign the docs section about field reuse.

@webmat
Copy link
Contributor Author

webmat commented Apr 21, 2020

@rw-access I pinged you on this PR in part because we had forgotten to nest pe at process.parent.pe. This PR will fix this.

raise ValueError(
'Reusable fields cannot be put inside other reusable fields except when the destination reusable is at the top level')
nesting_destination = nesting_destination.setdefault('fields', {})
nesting_destination[nest_as] = copy.deepcopy(schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Inserting a copy of the schema here instead of a reference to the original schema will lead to the same bug that was fixed by #722. I think to solve this we can make this line nesting_destination[nest_as] = schema and change line 177 below to schema['self_nestings'][self_nesting['as']]['fields'] = schema['fields']. This way the both the regular and self reused fieldsets will also receive any modifications to the schema fields that occur after this reusable is inserted.

Then, on line 230 when the fieldsets references are being recursively replaced with independent copies, self_nestings can be combined with fields to produce the final set of fields.

@webmat
Copy link
Contributor Author

webmat commented Jun 9, 2020

This PR has been superseded by #864

@webmat webmat closed this Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants