feat: allow pagination of message by date #890
Conversation
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
|
@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 <yume@yumechi.jp>
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.
There was a problem hiding this comment.
Sorry, I'm currently a little unresponsive, and probably will continue to be for the rest of the month.
Ahh yeah, that's tough.
From a general point of view, do we need the new "after" filter. If we want to query a specific date range e.g. 2025-01-01 to 2025-01-31, I don't think it makes much difference if you start fetching the messages from the newest (2025-01-31) or oldest (2025-01-01 messages.
The user would be responsible to fetch as many pages until all messages in the range were fetched. The only thing to consider would be big date ranges, and wanting to lazy load and starting with the oldest message, but I'm not sure if we need to support this.
I'm probably also okay with just not having an index for the features not used in the official clients. I don't think the difference it performance should be too visible, because the query is still pretty simple, and we're probably not talking about billions of messages in gotify.
Another idea would be for an intermediate api to query the message id boundaries. Something like
Get /message/idrange?interval=2025-10-10T23:00Z/2025-10-15T23:00Z
{"begin": 42, "end: 1337}
We'd only query messages with the actual ids, so it should make the pagination simpler. Maybe we could do this also internally in our pagination api, if a date is provided we translate it to a message. And if both is provided we do a min(translated date id, provided since message id) or similar.
What do you think about any of that?
There was a problem hiding this comment.
Yeah honestly I don't really care about the perf degredation for my own use case - I am just worried about it becoming a hard to resolve issue once somebody opens a ticket saying they have DB timeout for what looks like a simple query why (had this problem in another project I am engaged in)
This final proposal looks best to me because it inherits existing schemes , but we also need to implement the same thing again in the /application path, probably needs some refactoring, but the API presentations looks okay to me.
@cxtal Do you have any input on this? It should work for your case and respect your preference for long timestamps.
There was a problem hiding this comment.
after I come home from this week's vacation.
Sounds cool, having a good one?
If by "filter" you mean "give me the messages on this date", I have to give a hard no on that because time is a complex but continuous measure, the only viable way to implement it on server side is a range query (i.e. pagination).
Imagine it like this. Let's say I have a server with 100 messages. The user asks me for messages between two dates. I write a client like Winify that queries the 1000 messages. Because there is no way to filter the responses, I have to pull all the 1000 messages from the server, put them in a collection or array, then I need to sort them by their date field and only then I can offer the result to the user by matching the date-range.
This blows up quickly, let's say on an Android app. Imagine I am across the planet and I query my server back home with a date-range. Imagine the network bloat just to pull those messages and imagine the RAM or CPU my Android device would just waste on sorting useless messages that could have been pre-filled by the server.
The actual way of retrieving these messages would not matter and the current way that you are doing things are just fine using IDs and pagination. When Gotify receives a query like:
/application/10/message?limit=5&since=10&dateRange=S,T
Gotiy could retrieve all those messages using limit and since and only after apply the filter dateRange with S being a start date and T.
The problem is If let's say I live in UTC-8 you live in UTC+2 , we use the same gotify instance to maintain a project together. What is
/message/2025-12-03supposed to do?
RFC3339 dates that @jmattheis suggested carry the timezone so it should then be:
/message/2025-10-15T23:00Z
where Z stands for Zulu time which is military for UTC, which is why I chimed in as a preference over timestamps.
With RFC/ISO times, you would know the server timezone so you could even pass your local timezone and Gotify would know what that date represents.
But how you can present it on the frontend is much more freedom
You're right, pagination is great for UI-oriented frontends but they're not too great for data. Clearly, it would feel awkward to display on the same page 10000000s of results like on a Google search result page. Imagine the scrolling, lol.
Instead pagination is awkward when accessed via programmer's hook because I have to send stuff like this:
/application/10/message?limit=5&since=15
/application/10/message?limit=5&since=10
/application/10/message?limit=5&since=5
...
till I retrieve all the messages for application 10. It would even be cool to be able to retrieve all the messages at once.
When I couple Gotify with, say Amazon Alexa, and I tell Amazon Alexa "Alexa, show all notifications last Tuesday", there is no UI, pages, calendar selector or anything of the sort, not even conceptually.
IoT transforms "last Tuesday" into a date and then a program like Winify will query Gotify by performing calls similar to the above in order to retrieve all the messages from "last Tuesday" and then send a message back to IoT that will make Alexa talk back, perhaps, the notification titles of the messages matching the date "last Tuesday".
There is no user-selection here, well, except the voice, and Gotify has to be queried programmatically by Winify to process the messages itself.
(By the way, this scenario is real, Winify can do that because it connects with MQTT to brokers for automation.)
There was a problem hiding this comment.
Thanks, I get what you are talking about .. let me think about it (probably after I come back).. in short I think this debate cannot end unless we literally redesign away from REST principles (which , by construction, enforces some kind of hierarchical ordering of keys that isn't universally beneficial).
There was a problem hiding this comment.
If we want to be ambitious I mean we can have a graphQL endpoint for ultimate freedom on how to lookup messages .. I am open to it but depends on Jannis's appetite for complexity.
The thing is, I totally get your case, if you put this ticket into my own fringe project I will make an API for you specifically. But this is a multi person project where I have to be responsible for a lot of user's API stability and long term scope bloat (like magic query parameters, application specific endpoints, etc) control. I am not quite convinced any solution proposed here isn't just "hey this is a way to modify API so my specific case of a structural problem is fixed"
There was a problem hiding this comment.
@eternal-flame-AD Great idea, I mean, I see why you stay close to UI because Gotify indeed has an UI that is used to setup applications but in principle, not all REST endpoints have to be user-endpoints and they can be programmer endpoints and hooks.
Then, retrieving "all" messages is something an end-user would not do but a programmer that wants to query Gotify would. It's a bit of a cheat because then the API becomes a wrapper around the database but that's the only way it would work because some backends like SQLite are not really concurrent safe so querying the database while Gotify uses it would not work reliably.
Like if you use Sonarr, Radarr, etc, they all have an API token where you can perform various actions that are more data-oriented than what their respective UIs offers.
I was about to write that SQLite has a pretty powerful FTS engine, so at least for SQLite it would even be easy to offer a "fitler" to search message text itself and it's "free" because this is already built-in to SQLite via plugin.
There was a problem hiding this comment.
I think speaking as a user I never treated Gotify as some kind of data source or sink .. If I lost my database I won't be losing a night's sleep on it. That's why I am as you said "stay close to UI" because I think that's the primary use case.
Whether we should go for expose the entire database as long as it isn't a security vuln kind of thinking .. I can accept the validity but I would say would be a niche use case. If gotify were a paid product, the selling point would be a simple no frills API to get stuff on your phone notification , not how extensible the message query system is.
About FTS .. yes we could have that but we need to have coverage on MySQL and Postgres as well. I'm sure it's a resolvable problem but let's keep it to a different issue.
| 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 <yume@yumechi.jp>
Fixes #889
TODO:
[ ]: check the composite index is created correctly on postgres/mysql.