-
Notifications
You must be signed in to change notification settings - Fork 133
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
ROX-18155: pg generic store: Upsert #6882
ROX-18155: pg generic store: Upsert #6882
Conversation
Images are ready for the commit at 8f100c9. To use with deploy scripts, first |
cf1a456
to
cec2bcc
Compare
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.
Please review the logic behind IsWriteAllowed
in pkg/search/postgres/sac.go
.
If the function is not needed, it could be removed.
The other changes look good. There are some possible improvement follow-ups.
pkg/search/postgres/sac.go
Outdated
if err != nil { | ||
return false | ||
} | ||
return sacQuery.GetBaseQuery().GetMatchNoneQuery() != 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.
Write is allowed if the filter query is NOT MatchNoneQuery
. I have the feeling this would return true if the sacQuery
is a MatchNoneQuery
. Am I mistaken here ?
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.
Ah, I forgot to delete it. It's leftover from my try to make scope checker generic.
pkg/search/postgres/store.go
Outdated
|
||
func (s *GenericStore[T, PT]) upsert(ctx context.Context, objs ...PT) error { | ||
if s.insertInto == nil { | ||
err := errors.New("invalid operation, missing insertInto function") |
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.
nit: Why not making this error a file var or const ?
pkg/search/postgres/store.go
Outdated
|
||
func (s *GenericStore[T, PT]) copyFrom(ctx context.Context, objs ...PT) error { | ||
if s.copyFromObj == nil { | ||
err := errors.New("invalid operation, missing copyFromObj function") |
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.
nit: Why not making this error a file var or const ?
pkg/search/postgres/store.go
Outdated
} | ||
|
||
if err := s.copyFromObj(ctx, s, tx, objs...); err != nil { | ||
if err := tx.Rollback(ctx); 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.
nit: could we differentiate the rollback error from the copyFromObj one ?
pkg/search/postgres/store.go
Outdated
return err | ||
} | ||
|
||
conn, err := s.AcquireConn(ctx, ops.Get) |
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.
Although it was already so in the generated store function, I think it would make sense to use ops.Upsert
here.
Can be tracked as a follow-up.
pkg/search/postgres/store.go
Outdated
utils.Should(err) | ||
return err | ||
} | ||
conn, err := s.AcquireConn(ctx, ops.Get) |
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.
Although it was already so in the generated store function, I think it would make sense to use ops.Upsert
here.
Can be tracked as a follow-up.
pkg/search/postgres/store.go
Outdated
return s.permissionChecker != nil | ||
} | ||
|
||
func (s *GenericStore[T, PT]) permissionCheckerUpsertAllows(ctx context.Context) error { |
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.
nit: naming - how about permissionCheckerAllowsUpsert
?
cec2bcc
to
f943032
Compare
f943032
to
5e857e5
Compare
7215e6a
to
bc74488
Compare
5e857e5
to
f9f38c6
Compare
bc74488
to
33d8992
Compare
f9f38c6
to
8f100c9
Compare
/retest |
Description
The last Method migrated from to generic store. I didn't find a way to make SAC checks generic so I inject them with a function.
Checklist
If any of these don't apply, please comment below.
Testing Performed
CI