- Notifications
You must be signed in to change notification settings - Fork928
feat: Handle pagination cases where after_id does not exist#1947
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Throw an error to the user in these cases- Templateversions- WorkspacebuildsUser pagination does not need it as suspended users stillhave rows in the database
| if len(params.Status) == 0 { | ||
| params.Status = []database.UserStatus{database.UserStatusActive} | ||
| } |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this almost entirely solves the problem, but not 100%. It handles the case where theAfterID 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 betweenGetTemplateVersionByID andGetTemplateVersionsByTemplateID.
(Or alternatively, if the DB is using something like RDS with read-replicas, the row couldappear 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 isgood enough.
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 am dumb,@dwahler I'll put both db calls in a transaction.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 existThrow an error to the user in these cases- Templateversions- WorkspacebuildsUser pagination does not need it as suspended users stillhave rows in the database
Uh oh!
There was an error while loading.Please reload this page.
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.