[bugfix] Sort follows chronologically #2801
Conversation
The id on the follows table is not a ULID, but a random ID. Sorting on them results in a completely random order. Instead, sort on created_at, which sould result in a stable and intended sort order. Fixes: #2769
| "http://localhost:8080/users/1happyturtle", | ||
| "http://localhost:8080/users/admin" | ||
| "http://localhost:8080/users/admin", | ||
| "http://localhost:8080/users/1happyturtle" |
There was a problem hiding this comment.
This actually seems wrong to me. The created_at of 1happyturtle is later than admin, so I feel like the original order was correct given order by created_at DESC. But the test failed. If someone knows why we have an inversion here I'm curious.
There was a problem hiding this comment.
Actually helps if you fetch the column you're sorting on, which explains the inversion. Need to fix that 😄
There was a problem hiding this comment.
Wait, no, that's not necessary. Sorting seems to work because you can sort on columns not part of a select. Now I'm back to being confused.
There was a problem hiding this comment.
Oh, is this because Never mind, that just gets a list of IDs so should already be in the right order.GetFollowsByIDs needs to also do the sorting?
There was a problem hiding this comment.
Although, do we even want that for federation? This is just returning a Collection, so there's no guarantee of an order anyway. So maybe it's fine?
There was a problem hiding this comment.
it's passing the more comprehensive set of paging tests in ./internal/api/client/accounts/follow_test.go, which creates and tests its own set of follow / following relations instead of just the pre-prepared follows from the test models, which i trust more for confirming that we're maintaining correct behaviour imo
There was a problem hiding this comment.
looks good to me! though not quite sure what is going on with the test runner... i'll restart it see if that kicks it into gear :p
EDIT:
the standard set of drone tests just aren't being picked up as necessary for this PR 🤔 the drone push step is only cloning the repo ...
Description
The id on the follows table is not a ULID, but a random ID. Sorting on them results in a completely random order. Instead, sort on created_at, which sould result in a stable and intended sort order.
Fixes: #2769
Checklist
Please put an x inside each checkbox to indicate that you've read and followed it:
[ ]->[x]If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).
go fmt ./...andgolangci-lint run.