Off by one error in latest.json page 1

(Michael - #1

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

This only seems to happen between pages 0 and 1.

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

(Sam Saffron) #2

@oblakeerickson 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.

(Bianca) #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.

(Sam Saffron) #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.

(Michael - #6

The code is in lib/topic_query.rb

This looks ok:

but then this does not:

(Bianca) #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.

(Sam Saffron) #8

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

(Bianca) #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:

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

(Jeff Atwood) #10