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.

Sign up for GitHub

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

Fix validation of subfields of flattened objects #2088

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Fix validation of subfields of flattened objects #2088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions internal/fields/testdata/flattened.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"userName": "Bob",
"groupName": "admin"
}
}
},
"flattened.request_parameters.userID": 1000
}
}
}
18 changes: 18 additions & 0 deletions internal/fields/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,8 @@ func (v *Validator) validateScalarElement(key string, val any, doc common.MapStr
switch {
case skipValidationForField(key):
return nil // generic field, let's skip validation for now
case isFlattenedSubfield(key, v.Schema):
return nil // flattened subfield, it will be stored as member of the flattened ancestor.
case isArrayOfObjects(val):
return fmt.Errorf(`field %q is used as array of objects, expected explicit definition with type group or nested`, key)
case couldBeMultifield(key, v.Schema):
Expand Down Expand Up @@ -855,6 +857,22 @@ func isArrayOfObjects(val any) bool {
return false
}

func isFlattenedSubfield(key string, schema []FieldDefinition) bool {
for strings.Contains(key, ".") {
i := strings.LastIndex(key, ".")
key = key[:i]
ancestor := FindElementDefinition(key, schema)
Comment on lines +862 to +864
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be replaced by findParentElementDefinition ?

Suggested change
i := strings.LastIndex(key, ".")
key = key[:i]
ancestor := FindElementDefinition(key, schema)
ancestor := findParentElementDefinition(key, schema)

Copy link
Contributor

Choose a reason for hiding this comment

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

mmm I guess it would be needed to somehow return also the key of the field, or being able to retrieve it from the ancestor struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this is the problem I found, we still need to loop over the ancestors.

if ancestor == nil {
continue
}
if ancestor.Type == "flattened" {
return true
}
Comment on lines +868 to +870
Copy link
Contributor

@mrodm mrodm Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If it finds an ancestor with a different type of "flattened", should this return false directly here?

I was thinking in the case of the ancestor being something like: ancestor.Type = object and ancestor.ObjectTYpe == "keyword"

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, there are packages defining mappings for subfields of flattened objects (for example here). This doesn't make a lot of sense as a mapping, but this is allowed now, and can be used for example to provide documentation.

We probably should not allow to define mappings under flattened fields, but this would be a different task, and checked in a different place. And we would need to provide something for cases where developers want to provide docs for flattened subfields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, there are packages defining mappings for subfields of flattened objects (for example here). This doesn't make a lot of sense as a mapping, but this is allowed now, and can be used for example to provide documentation.

Ok, I didn't know about this kind of usage.

We probably should not allow to define mappings under flattened fields, but this would be a different task, and checked in a different place. And we would need to provide something for cases where developers want to provide docs for flattened subfields.

Agreed!

}

return false
}

func findElementDefinitionForRoot(root, searchedKey string, fieldDefinitions []FieldDefinition) *FieldDefinition {
for _, def := range fieldDefinitions {
key := strings.TrimLeft(root+"."+def.Name, ".")
Expand Down