feat: allow pagination of message by date #890
Conversation
Signed-off-by: eternal-flame-AD <[email protected]>
|
@jmattheis Is this original "since" parameter semantically backwards? I would imagine "since yesterday" retrieves messages that are dated after yesterday, however the current implementation returns messages that are before yesterday. To not introduce breaking changes I named the opposite parameter "after" so it's unambiguous . |
Signed-off-by: eternal-flame-AD <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #890 +/- ##
==========================================
+ Coverage 79.15% 79.25% +0.09%
==========================================
Files 56 56
Lines 2226 2256 +30
==========================================
+ Hits 1762 1788 +26
- Misses 360 362 +2
- Partials 104 106 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
See #34 (comment) for the naming. |
| Limit int `form:"limit" binding:"min=1,max=200"` | ||
| Since uint `form:"since" binding:"min=0"` | ||
| Limit int `form:"limit" binding:"min=1,max=200"` | ||
| Since uint64 `form:"since" binding:"min=0"` |
There was a problem hiding this comment.
All other date-time related fields use RFC3339 format, this would break the consistency with it being a unix timestamp.
I think I'd prefer either something like an ISO interval e.g. &interval=2025-10-10T23:00Z/2025-10-15T23:00Z and then allowing to use the after/before id offset, to page in this daterange to prevent the bugs described in #889 (comment)
or have separate sinceTime and afterTime fields which accept an RFC3339 datetime.
What do you think? I don't dislike this current solution, so I'm open for any arguments (:.
There was a problem hiding this comment.
or have separate sinceTime and afterTime fields which accept an RFC3339 datetime.
+1 I prefer ISO dates to unix timestamps
api/message.go
Outdated
| params := &pagingParams{Limit: 100} | ||
| params := &pagingParams{Limit: 100, By: "id"} | ||
| if err := ctx.MustBindWith(params, binding.Query); err == nil { | ||
| log.Printf("Paging params: %+v", params) |
There was a problem hiding this comment.
This is probably not needed right?
| Column: clause.Column{ | ||
| Table: "messages", | ||
| Name: by, | ||
| }, | ||
| Desc: since != 0 || after == 0, |
There was a problem hiding this comment.
Does this mean, the messages are returned in another order based on if since or after is set. I feel like this is unexpected if both parameters are set. It should be another parameter to make it more explicit.
Either way, it should always use sorting by id, as this will be more precise than ordering by date, as there could be messages with the same date.
There was a problem hiding this comment.
I agree this is not ideal, the reason I used this scheme is mostly:
The computational complexity of finding inequalities on two keys is O(N) time O(N) space (or you can build 4 indices, regardless the complexity is squared and as if no indices apply), this is made worse by the fact that the database will not be permitted to assume id and date are monotonic to each other.
So I am trying to design the API to prevent making it possible to make queries like id > 100 and timestamp < 200, to which the only viable query plan will be traverse idx_id upwards and collect 50 messages satisfying timestamp < 200 which may be expensive depending on the selectivity of the timestamp < 200 to this query.
There was a problem hiding this comment.
I am open to using distinct keys and just enforce they are mutually exclusive to each other though, but the tradeoff is vocabulary bloat instead of this "switch pagination key" semantics.
| var messages []*model.Message | ||
| db := d.DB.Where("application_id = ?", appID).Order("messages.id desc").Limit(limit) | ||
| db := d.DB.Where("application_id = ?", appID).Order(clause.OrderBy{Columns: []clause.OrderByColumn{ | ||
| { |
There was a problem hiding this comment.
Maybe this can be unified between the two *Paginated apis, the only difference is the additional filtering for application id or?
There was a problem hiding this comment.
Yes .. I don't think I wrote this part (?) so I am not sure the exact rationale but I assume it's to accommodate it being mounted on a different API path. It can be just one interface at the data model layer.
Signed-off-by: eternal-flame-AD <[email protected]>
Fixes #889
TODO:
[ ]: check the composite index is created correctly on postgres/mysql.