feat: Handle pagination cases where after_id does not exist #1947
Conversation
Throw an error to the user in these cases - Templateversions - Workspacebuilds User pagination does not need it as suspended users still have rows in the database
| if len(params.Status) == 0 { | ||
| params.Status = []database.UserStatus{database.UserStatusActive} | ||
| } |
There was a problem hiding this comment.
postgres defaults to an "active" filter.
coderd/templateversions.go
Outdated
| if paginationParams.AfterID != uuid.Nil { | ||
| // See if the record exists first. If the record does not exist, the pagination | ||
| // query will not work. | ||
| _, err := api.Database.GetTemplateVersionByID(r.Context(), paginationParams.AfterID) |
There was a problem hiding this comment.
I think this almost entirely solves the problem, but not 100%. It handles the case where the AfterID is bogus (never existed), and where the row was deleted before the current request started, but there's still a possibility that the row is deleted between GetTemplateVersionByID and GetTemplateVersionsByTemplateID.
(Or alternatively, if the DB is using something like RDS with read-replicas, the row could appear to be deleted if the two queries hit different replicas, which would break the assumption of sequential consistency. Not sure if that's something we plan to support.)
I guess even if that race happened, the consequence would just be falling back to the current behavior of returning an empty resultset, so this is still a big improvement.
There was a problem hiding this comment.
Yea, I was trying to do it in postgres to prevent that tiny window @dwahler, but I don't think you can raise an exception in a query. It has to be a postgres procedure, which is a headache.
I think this is good enough.
There was a problem hiding this comment.
I am dumb, @dwahler I'll put both db calls in a transaction.
Add rules.go for catching this
|
@dwahler I am going to move it to another PR since it was acting strange in CI. I'll put it back in |
* feat: Handle pagination cases where after_id does not exist Throw an error to the user in these cases - Templateversions - Workspacebuilds User pagination does not need it as suspended users still have rows in the database
Throw an error to the user in these cases
User pagination does not need it as suspended users still
have rows in the database
I added unit tests to confirm status code
Bad Requestfor these cases. And confirmed theuserspagination is ok and works in both the db fake + postgres.