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
feat: support contains
to compare json objects
#3687
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
if rightValue.Value().Type == types.TypeJson && comparatorName == "contains" { | ||
if leftValue.Value().Type == types.TypeJson || leftValue.Value().Type == types.TypeArray { | ||
comparatorName = "json-contains" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was cleaner to write than a longer if
statement containing all four conditions.
{ | ||
Name: "should_be_able_to_compare_with_subset", | ||
Query: `'{"name": "john", "age": 32, "email": "john@company.com"}' contains '{"email": "john@company.com", "name": "john"}'`, | ||
ShouldPass: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add another case here as a sanity check: comparing the same object, but with different attribute orders, like:
{"name": "john", "age": 32, "email": "john@company.com"}' contains '{"email": "john@company.com", "name": "john", "age": "32"}
{ | ||
Name: "should_match_complete_arrays", | ||
Query: `'{"numbers": [0,1,2,3,4]}' contains '{"numbers": [0,1,2,3,4]}'`, | ||
ShouldPass: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this case, I would add a case to test arrays that seems to be equal, but doesn't, like:
{"numbers": [0,1,2,3,4]}' contains '{"numbers": [0,1,"2","3",4]}
executeTestCases(t, testCases) | ||
} | ||
|
||
func TestJSONExecution(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to add a comparison for escaped JSON? Something like:
'{"name": "john", "age": 32, "email": "john@company.com"}' contains '{\"email\": \"john@company.com\" }'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escaping is not stable in the engine. As this is a problem in the core of the engine and not in the feature, I'll skip this and wait for users to ask for a fix. Not sure if that will happen.
Name: "should_be_able_to_compare_deep_objects_in_subset", | ||
Query: `'{"name": "john", "age": 32, "email": "john@company.com", "company": {"name": "Company", "address": "1234 Agora Street"}}' contains '{"email": "john@company.com", "name": "john", "company": {"name": "Company"}}'`, | ||
ShouldPass: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a test with deep objects and arrays?
{"name": "john", "age": 32, "email": "john@company.com", "company": {"name": "Company", "address": "1234 Agora Street", "telephones": ["01", "02", "03"]}}' contains '{"email": "john@company.com", "name": "john", "company": {"name": "Company", "telephones": ["01", "02", "03"]}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An editing suggestion.
Co-authored-by: Julianne Fermi <julianne@kubeshop.io>
This PR allows an expression to compare two json objects with a
subset
relationship. Related to #3684Checklist
Loom video
Check tests for details