Fix validation of multi-fields of external fields #1335
Conversation
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
| injectOptions := fields.InjectFieldsOptions{ | ||
| // Keep External parameter when rendering fields, so we can render | ||
| // documentation for empty groups imported from ECS, for backwards compatibility. | ||
| KeepExternal: true, |
There was a problem hiding this comment.
This parameter for fields injection can be a bit difficult to follow. Its only purpose is to mimic a collateral behaviour we had with lazy resolution of external fields: In some places we were skipping group fields when they were empty. But this was not happening with external fields, because on these checks the field was still unresolved.
We may argue that this behaviour is unexpected and we should fix it instead, because it treats fields on different ways depending if they are external or not. But fixing this would break CI builds of packages, because it changes what fields are rendered.
This flag helps keeping the old behaviour for rendered documentation:
- When rendering documentation, we keep external, so we can keep the old behaviour.
- In other cases, such as when resolving external fields in package builds, we remove the "external" parameter once the field is injected, so it is not written to fields files. On these cases we may be removing empty groups from built fields files, but this is actually good because they don't generate any meaningful mapping.
|
test integrations |
|
Something is happening with buildkite 🤔 |
|
buildkite test this |
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
|
/test |
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
jsoriano
force-pushed
the
ecs-multi-fields
branch
from
099207e
to
1131866
Compare
💚 Build Succeeded
History
cc @jsoriano |
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#6756 |
|
@mrodm sounds good to me, thanks. |
We inject fields from ECS on different scenarios: when generating documentation, when building packages, or when validating documents in tests. On #1335 we did a refactor around this, to fix some validation issues, in a way that more code is reused between all these uses. After this change, validation used field definitions that include the already resolved external fields, but we weren't including there information that was used by validators, what included validation of expected and allowed values. So these validations haven't been executed since then. Tests have been included to try to avoid regressions related to this in the future.
We inject fields from ECS on different scenarios: when generating documentation, when building packages, or when validating documents in tests. On elastic#1335 we did a refactor around this, to fix some validation issues, in a way that more code is reused between all these uses. After this change, validation used field definitions that include the already resolved external fields, but we weren't including there information that was used by validators, what included validation of expected and allowed values. So these validations haven't been executed since then. Tests have been included to try to avoid regressions related to this in the future.
Refactor validator so it preloads external fields instead of loading them on demand when requested. This allows to have a single implementation of external fields resolution.
Regarding multi-fields in external fields, it was not possible to validate them before because it was not possible to differentiate them from undefined fields without previously resolving the parent. This would require to resolve the parent for all unknown fields that were potential children of external fields. Now they are preloaded, so they can be directly validated.
There was also an error in dependency management that was being ignored to help on testing. This error is reported now, and tests depending on this explicitly disable dependency management to avoid it.
The status data stream of the mongodb package from elastic/integrations#6692 is included here as a test case for time series and multi-fields. This is required because enabling synthetic source is the only way we have now to find multi-fields in test documents.
Fix #1334.