-
Notifications
You must be signed in to change notification settings - Fork 20
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
Support jsonb search #127
Support jsonb search #127
Conversation
Signed-off-by: clyang82 <[email protected]>
/assign @qiujian16 @skeeey @morvencao |
/retest |
2 similar comments
/retest |
/retest |
@@ -154,6 +155,16 @@ func (s *sqlGenericService) buildSearch(listCtx *listContext, d *dao.GenericDao) | |||
return true, nil | |||
} | |||
|
|||
if isJSONBSearch(listCtx.args.Search) { | |||
// focus on jsonb, ignore type = 'Single' or 'Bundle' | |||
jsonbSearch, _, _ := strings.Cut(listCtx.args.Search, "and type=") |
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.
what does "and type=" mean?
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.
it was added at the beginning to decide the type is Single
or Bundle
. refer to
maestro/pkg/handlers/resource.go
Line 105 in 7a3daed
listArgs.Search = fmt.Sprintf("type='%s'", api.ResourceTypeSingle) |
// handle ->> search. for example: payload -> 'data' -> 'manifest' -> 'metadata' -> 'labels' ->> 'foo' = 'bar' | ||
if strings.Contains(search, "->>") { | ||
jsonbStr := stringSplitwithTrim(search, "->>") | ||
if err := isValidFields(stringSplitwithTrim(jsonbStr[0], "->")); err != nil { |
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.
do we ensure jsonbStr is always not nil
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.
if the search string has "->>", it can enter this method. that means jsonbStr[0] and jsonbStr[1] is always not nil. for example: "'labels'->>", the jsonbStr[0] is labels
and jsonbStr[1] is empty.
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.
what if only "->>"? Also what if there are two "->>" in it? Do we allow only one?
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.
{
"name": "invalid search without field name",
"search": "->>",
"error": "the search field name is invalid",
},
{
"name": "invalid search with two ->> symbols",
"search": "payload -> 'data' -> 'manifest' -> 'metadata' -> 'labels'->> 'foo' = 'bar' AND 'labels'->> 'foo'",
"error": "the search value is invalid",
},
Added 2 tests to cover the above case. Yes. we allow only one ->>
symbol.
return fmt.Errorf("the search field name is invalid: %v", err) | ||
} | ||
labelStr := stringSplitwithTrim(jsonbStr[1], "=") | ||
if err := validation.IsQualifiedName(labelStr[0]); err != nil { |
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.
ditto, do we ensure labelStr is alway not nil?
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.
what is there is no "="?
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.
refer to this test
{
"name": "complex labels",
"search": "payload -> 'data' -> 'manifest' -> 'metadata' -> 'labels' ->> 'example.com/version'",
},
return fmt.Errorf("the search field name is invalid: %v", err) | ||
} | ||
// validate the value is a valid json object | ||
if !isValidJSONObject(jsonbStr[1]) { |
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.
Do we support any json string?
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.
Yes. support any json only if it is valid json object.
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.
do we need to support any json string? For instance, even a spec field in the json string?
Signed-off-by: clyang82 <[email protected]>
fixed: #102