Skip to content

Fix adjustments to WP_Query pagination to use only post_date.#409

Merged
jasonbahl merged 2 commits intowp-graphql:developfrom
Quartz:pagination-fix
Mar 9, 2018
Merged

Fix adjustments to WP_Query pagination to use only post_date.#409
jasonbahl merged 2 commits intowp-graphql:developfrom
Quartz:pagination-fix

Conversation

@jasonbahl
Copy link
Copy Markdown
Collaborator

@jasonbahl jasonbahl commented Feb 21, 2018

This is an attempt to address #406 (see for details).

This adjusts the where clause generated by WP_Query pagination logic to use only post_date and not post ID to accommodate out-of-order post publishing.

I've added some tests that will produce failures in the current codebase but pass with these changes. They simulate publishing posts out-of-order, such that post ID 12 is not guaranteed to be published after post ID 11 (as an example).

One side-effect of this change is that posts could get duplicated across pagination boundaries if the cursor post shares the exact same publish date (to the second) with another post. In that instance, there is no way to determine which side of the boundary the post belongs to (without, say, storing query results in a state store). However, this seems to be a commonly accepted side-effect of cursor-based pagination.

@jasonbahl
Copy link
Copy Markdown
Collaborator Author

@chriszarate here's the PR re-opened. Taking a look again. Seeing if I have any thoughts/concerns about the pagination changes.

@jasonbahl
Copy link
Copy Markdown
Collaborator Author

@chriszarate it looks like some of the DataConfigTest tests are intermittently passing and failing, so I think we have some edge case we need to investigate.

I haven't looked in detail quite yet. It could potentially be something we can change in the assertions, or it might be an actual bug we need to fix with the pagination change you made.

I'll take a look later today or tomorrow if you don't get to it first.

You can check the past few travis builds and you can see that they sometimes pass and sometimes fail. 🤔

@jasonbahl
Copy link
Copy Markdown
Collaborator Author

@chriszarate
Copy link
Copy Markdown
Collaborator

Hmmm, that's a bummer. I'll take a look tomorrow.

Previously, the where clause also required posts to have a post ID lower
than the cursor post (or higher if the sort order was ascending). That
causes problems when the posts are published out of order (e.g., post ID
2 is published before post ID 1).

// Make sure starting timestamp is sufficiently in the past so that posts
// do not receive a post_status of "future".
$timestamp = time() - 1000;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I had a feeling it was something like this. thanks for fixing!

@chriszarate
Copy link
Copy Markdown
Collaborator

@jasonbahl Found the reason for the intermittent failing tests. I was creating posts with a time offset so that I could reorder them, but in doing so I was creating posts with publish dates in the future. Depending on how long the tests took to run, this sometimes resulted in posts with a post_status of future, which made them invisible to queries looking for a post_status of publish.

Simple fix: put the starting timestamp in the past to avoid the possibility of a post_status of future. Previously the tests would fail about 1 out of 6 times. I've run the tests about 50 times now with now failures. Feel free to run the tests on Travis a bunch.

@jasonbahl
Copy link
Copy Markdown
Collaborator Author

@chriszarate thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants