manifoldmarkets / manifold

Manifold Markets: A market for every question
https://manifold.markets
MIT License
399 stars 154 forks source link

`before` and `after` in the API skips over objects with identical `createdTime` #2304

Open Axel-Jacobsen opened 7 months ago

Axel-Jacobsen commented 7 months ago

Howdy!

I found a bug where using before or after in the API url will miss objects. I'll paste in the urls that I used for easy reproducibility, I've just been poking around with my browser. I haven't tested on all endpoints, but just going by the docs, I'd guess that this affects any GET that returns a sequence ordered by created time, so GET /v0/markets, GET /v0/users/, GET /v0/groups, and GET /v0/managrams. Here's an example w/ the GET /v0/bets endpoint:

https://api.manifold.markets/v0/bets?&userId=pZOxBAWFRrUJPK6c1a5RI89Fw6x1&before=

As of the time of creating this issue, the 0th bet is C4nKqKXEnPwR6ORnALsa with creation time 1702224677794. The next bet is 1fmJ0FRUEsUc40eYvwMo with creation time 1702152570207. However, it turns out that the next 8 bets also have the same creation time of 1702152570207.

So, if you ask all bets before the 0th, you get bet id 1fmJ0FRUEsUc40eYvwMo as expected:

https://api.manifold.markets/v0/bets?&userId=pZOxBAWFRrUJPK6c1a5RI89Fw6x1&before=C4nKqKXEnPwR6ORnALsa

But, if you ask for all bets after 1fmJ0FRUEsUc40eYvwMo, it skips the next 8 bets, and the first bet you get is IrAVl6uo04tkBax71DUy, creation time 1702152557761.

https://api.manifold.markets/v0/bets?&userId=pZOxBAWFRrUJPK6c1a5RI89Fw6x1&before=1fmJ0FRUEsUc40eYvwMo

I guess that the endpoints are implemented as something like SELECT * FROM BETS WHERE createdTime < beforeId.createdTime (excuse my pseudo sql), which will skip over bets where createdTime == beforeId.createdTime. And, if you do SELECT * FROM BETS WHERE createdTime <= beforeId.createdTime, that will also include before which would be annoying for the API user.

Two solutions that I can think of:

  1. change createdTime to a f64 - ie down to the millisecond. It probably would work, but it would be a breaking change and doesn't get to the core of the issue (but it would be easy!)
  2. change the query to SELECT * FROM BETS WHERE createdTime <= beforeId.createdTime && id != beforeId.id

Also, I wouldn't mind just doing this if someone can point me where to look and what solution you prefer!

Cheers, and Merry Christmas!

ProgramCrafter commented 7 months ago

change the query to SELECT * FROM BETS WHERE createdTime <= beforeId.createdTime && id != beforeId.id

Won't work if there are three bets A@1s, B@1s, C@1s and you've already got A and B; that solution would return A and C.

Axel-Jacobsen commented 7 months ago

change the query to SELECT * FROM BETS WHERE createdTime <= beforeId.createdTime && id != beforeId.id

Won't work if there are three bets A@1s, B@1s, C@1s and you've already got A and B; that solution would return A and C.

Ah lol you're right - I guess you just need f64 timestamps eh

sipec commented 5 months ago

yeahh lol we really should do a real pagination method. for most APIs we can now, since we use supabase. the current behavior with before and after should stay the same though since it is semantically correct

I think this would be a pretty good task for an open source contribution ;)

benmanns commented 4 months ago

You can do something like:

SELECT *
FROM bets
WHERE
  (
    createdTime = beforeId.createdTime
    AND id < beforeId.id
  ) OR (
    createdTime < beforeId.createdTime
  )
ORDER BY
  createdTime DESC,
  id DESC

basically using the tuple (createdTime, id). With Postgres directly I think you could actually go even simpler:

SELECT *
FROM bets
WHERE
  (createdTime, id) < (beforeId.createdTime, beforeId.id)
ORDER BY
  createdTime DESC,
  id DESC

though that’s not supported via the public Supabase API.