Fix validations based on fields imported from ECS #1452
Conversation
| @@ -318,17 +333,28 @@ func transformImportedField(fd FieldDefinition) common.MapStr { | |||
| m["doc_values"] = *fd.DocValues | |||
| } | |||
|
|
|||
| if len(fd.Normalize) > 0 { | |||
| m["normalize"] = fd.Normalize | |||
| } | |||
There was a problem hiding this comment.
Moving this to the if below will effectively remove normalize from the built packages. But I think this is not needed there? 🤔
I could revert this change if we consider it risky.
There was a problem hiding this comment.
If this field is not used afterwards by Elasticsearch, I think it could be moved to that new if block.
And IIRC , this function is just executed while doing comparing documents (testing) with external fields. In built packages, I guess it's not needed.
|
/test |
💚 Build Succeeded
History
cc @jsoriano |
| @@ -318,17 +333,28 @@ func transformImportedField(fd FieldDefinition) common.MapStr { | |||
| m["doc_values"] = *fd.DocValues | |||
| } | |||
|
|
|||
| if len(fd.Normalize) > 0 { | |||
| m["normalize"] = fd.Normalize | |||
| } | |||
There was a problem hiding this comment.
If this field is not used afterwards by Elasticsearch, I think it could be moved to that new if block.
And IIRC , this function is just executed while doing comparing documents (testing) with external fields. In built packages, I guess it's not needed.
| if options.IncludeValidationSettings { | ||
| if len(fd.Normalize) > 0 { | ||
| m["normalize"] = fd.Normalize | ||
| } | ||
|
|
||
| if len(fd.AllowedValues) > 0 { | ||
| m["allowed_values"] = fd.AllowedValues | ||
| } | ||
|
|
||
| if len(fd.ExpectedValues) > 0 { | ||
| m["expected_values"] = fd.ExpectedValues | ||
| } | ||
| } |
There was a problem hiding this comment.
This means that these fields (normalize, allowed_values, expected_values) won't be added to each field definition when the package are built, is that right?
There was a problem hiding this comment.
Yes, this is not used in Fleet or Elasticsearch.
| checkFn: func(t *testing.T, result []common.MapStr) { | ||
| require.Len(t, result, 1) | ||
| _, ok := result[0]["allowed_values"] | ||
| if !assert.True(t, ok) { | ||
| d, _ := json.MarshalIndent(result[0], "", " ") | ||
| t.Logf("expected to find allowed_values in %s", string(d)) | ||
| } | ||
| }, |
| @@ -163,6 +163,8 @@ func CreateValidatorForDirectory(fieldsParentDir string, opts ...ValidatorOption | |||
|
|
|||
| func createValidatorForDirectoryAndPackageRoot(fieldsParentDir string, finder packageRootFinder, opts ...ValidatorOption) (v *Validator, err error) { | |||
| v = new(Validator) | |||
| // In validator, inject fields with settings used for validation, such as `allowed_values`. | |||
| v.injectFieldsOptions.IncludeValidationSettings = true | |||
There was a problem hiding this comment.
If it is set to true here, every run of validation would use those fields, even in built packages ?
There was a problem hiding this comment.
Talked offline, and this should be just used for validators. Built packages do not use this one.
|
test integrations |
|
Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#7817 |
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.
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:
So these validations haven't been executed since then.
Tests have been included to try to avoid regressions related to this in the future.
Fix #1439