Conversation
yamaceay
force-pushed
the
prevent-sql-injection
branch
5 times, most recently
from
f6e5c6b to
f092b43
Compare
Noel-Ch
left a comment
Noel-Ch
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I think it'll be better if we can review unrelated changes in a separate PR, especially if they are more complicated
| if err != nil { | ||
| // In case of error, return original actions | ||
| return actionStrings |
There was a problem hiding this comment.
Should we ignore the errors and return the original actions here, or actually return an error so that the user knows that there is a problem?
There was a problem hiding this comment.
Good question, I'm switching it back.
|
|
||
| func prepareCreateSql(parameters *v1alpha1.AuditPolicyParameters) string { | ||
| query := fmt.Sprintf("CREATE AUDIT POLICY %s AUDITING %s", parameters.PolicyName, parameters.AuditStatus) | ||
| func OptimizeAuditActions(actionStrings []string) []string { |
There was a problem hiding this comment.
There is a lot of complexity added here, and it seems like the changes are not directly related to the SQL injection risk. Maybe we can handle this in a separate PR?
internal/utils/utils.go
Outdated
| if _, found := set2[item]; !found { | ||
| return false | ||
| } | ||
| func MapKeysToUpper[A comparable](input map[string]A) map[string]A { |
There was a problem hiding this comment.
Is this function necessary? There is a loop immediately after each use of the function. Can we do the uppercasing in that loop, so that we don't have a extra allocation of a new map?
internal/utils/utils.go
Outdated
| } | ||
|
|
||
| return true | ||
| func MapsEqual[A comparable](map1, map2 map[string]A) bool { |
There was a problem hiding this comment.
We can remove this function and use maps.Equal instead
| if len(leftDifference) != 0 || len(map1) != len(map2) { | ||
| return false, nil, nil | ||
| } |
There was a problem hiding this comment.
If isEqual is false, shouldn't we return the difference instead of nil?
There was a problem hiding this comment.
Good point! I remember we talked about shortcutting the isEqual output (so that we just check one difference instead of two). Then I think it's better to check both differences in either case. I will introduce a new flag to allow shortcutting.
There was a problem hiding this comment.
If we already know that both maps are the same, we can return early because the difference would be nil.
But if the maps are different, we have to check the maps for what is difference, and return the difference
internal/utils/utils.go
Outdated
| if len(leftDifference) != 0 || len(set1) != len(set2) { | ||
| return false, nil, nil | ||
| } |
There was a problem hiding this comment.
If isEqual is false, shouldn't we return the difference instead of nil?
yamaceay
force-pushed
the
prevent-sql-injection
branch
from
f092b43 to
f90fe0f
Compare
yamaceay
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Error
yamaceay
force-pushed
the
prevent-sql-injection
branch
from
f90fe0f to
498804c
Compare
yamaceay
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Error
yamaceay
force-pushed
the
prevent-sql-injection
branch
from
498804c to
d81c9cc
Compare
yamaceay
had a problem deploying
to
pr-e2e-approval
GitHub Actions
Failure
2 participants
No description provided.