Off by one error in latest.json page 1

Contribute 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개의 좋아요
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개의 좋아요
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개의 좋아요
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개의 좋아요
6

The code is in lib/topic_query.rb

This looks ok:

but then this does not:

1개의 좋아요
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개의 좋아요
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개의 좋아요