Off by one error in latest.json page 1

Bug
1

latest.json page 0 , topic_list.topics[29] is the same item as page 1 topic_list.topics[0]

image.png988×233 11.7 KB

image.png996×236 12.9 KB

This only seems to happen between pages 0 and 1.

I can’t find the code that is causing this.

4 « J'aime »
2

@blake can you add to your list, but we need to make sure @eviltrout or myself review this carefully prior to merging cause I worry this could break the front end if it is somehow depending on this super odd behavior.

6 « J'aime »
4

I also looked into this and it seems that this bug is related only to pages 0 and 1.

The selection queries are something like:

SELECT ... LIMIT 30; -- Query A (page 0)
SELECT ... LIMIT 30 OFFSET 29; -- Query B (page 1)
SELECT ... LIMIT 30 OFFSET 59; -- Query C (page 2)

Query A is going to select first 30 rows (1-30); query B is going to select 30 rows, but would skip the first 29 (30-59 – this is why row 30 is duplicated); and query C is going to select 30 rows, but would skip the first 59, so (60-89).

The fix would be to always skip page * 30. Currently the formula is page * 30 - 1.

Unfortunately, I do not know where exactly to look to fix this issue. I have been looking through the ListController, but failed. Any suggestions are appreciated.

Also, I believe that this will not affect the front-end, but I cannot say for sure.

3 « J'aime »
5

We need to be careful here, cause it is not impossible that the client is expecting overlap. Be sure to ensure scrolling latest continues to work if you apply this fix.

2 « J'aime »
6

The code is in lib/topic_query.rb

This looks ok:

but then this does not:

1 « J'aime »
7

I looked into this and it really is the second snippet of code the one that causes the issue. I made the changes (removed the - 1), did some testing and everything works as expected. I will be testing more.

6 « J'aime »
8

I recall seeing a PR to fix this, did it ever get merged in?

9

I too remember sending a PR, but it looks like I never did. I did some work on this issue and tested it, but probably I considered that more testing was required and that’s why I did not finalize sending the PR.

Anyway, here it is:
https://github.com/discourse/discourse/pull/4858

Since it is an important part of the system, I would really appreciate if you could look into it as well.

7 « J'aime »