fix: refactor resolvers to better follow Relay spec#2457
fix: refactor resolvers to better follow Relay spec#2457jasonbahl merged 21 commits intowp-graphql:developfrom
Conversation
…orks when using separate POST requests. The failing tests in the wpunit/CommentConnectionQueriesTest.php file are related to multiple calls to `graphql()` being executed in one php request. _something_ is not being cleared between separate calls to graphql(). The theory is one of the filters on the WP_Comment_Query class, but we still need to debug to identify the exact issue.
…ment. This allows different "graphql_args" to be used to determine different caches since all args are not respected when the cache key is generated
…ursor class to build the cursor - introduce the CommentObjectCursor class to build the cursor for comment queries - update some functions in the other cursor classes for consistent code style - update CommentConnectionQueriesTests to not be marked incomplete
Do you know what issue this is? Is it an open Github issue or just a bug you noticed but doesn't have a formal issue opened? (edit: nevermind. I see the test that's marked incomplete. Checking it out now) |
|
|
||
| // Assert the 2nd item is the 4th most recent post | ||
| $this->assertSame( $this->created_post_ids[3], $actual['data']['posts']['nodes'][1]['databaseId'] ); | ||
| if ( isset( $graphql_args['where']['search'] ) ) { |
There was a problem hiding this comment.
@justlevine looks like this was a false bug.
I tried reproducing and couldn't but then realized that the test was creating 7 posts with uniqueString and then asserting that page 3 of 2 posts per page should have hasNextPage as false. But since there were 7 posts, hasNextPage was properly resolving as true as page 3 returns nodes 5 & 6, and page 4 would return the 7th node.
I updated the test to only create 6 posts and it's good now. 🙌🏻
|
Yeah no prior GH issue, something unearthed during tests. Though come to think of it maybe #2419 is also related? |
@justlevine seems loosely related, but I don't think directly. 🤔 I think that issue is maybe an issue with the argument getting mapped from the graphql input to the query_args passed to WP_Query? could be wrong tho |
…ead of 7 when testing pagination as the tests make assertions based on 6 records, not 7. - add cleanup to delete posts after the tests
… use a new $field_key variable - add some compare/order logic for when menu_order is the orderby. (not really happy with this, but it works. Would love to refactor this as I feel it's very naiive right now) - update connection resolver from MenuItem to "connectedNode" to query "any" post_status (as drafts, etc _can_ be in menus) - remove "markTestIncomplete" from menu item connection queries tests
- set the "posts_per_page" argument for one to one connections as 1 instead of the max value
|
@justlevine I pushed up a few commits. All tests are now passing. No more are marked incomplete 👏🏻 I'm not incredibly pleased with the solution I came up with for the the pagination issues related to when posts are sorted by "menu_order". I think it needs more thought, but I don't have brilliant ideas at the moment. I got it to work, but I'm not sure I love the implementation. Open to iteration either now, or sometime in the future. Anything else I did is of course also open to iteration, cleanup, DRYing up, etc. I feel like there's a lot we can do in the cursor classes to DRY things up, but again, my focus was on getting things working first, before making the code DRY and abstracting patterns. |
The important thing is that it's working and I dont think it's worth delaying this PR over (I cant think of any alternatives either 😅). I did a basic refactor of the CursorObject classes so they extend One thing I'm stuck on is what to do with the old The only place we're actually using Do we get rid of them altogether, or mark them as deprecated in a comment? I think it's more likely for people to be setting them then getting them in their own projects (and rare altogether), so I'm not that worried about removing them, but its still something to consider. Once a decision is made on that, I think this PR is good to ship. |
@justlevine you missed the link for this plugin. What plugin? |
This plugin meaning WPGraphQL core. We're still setting those custom query vars in all those connection resolvers, but only using Not sure why I wasn't explicit 😅. Updated the comment. |
|
@justlevine ah! I thought you meant like an extension plugin. Sorry about that. 🤦🏻 |
…olvers - update config.php to early return when not a graphql request - php cs fixes
…ection-resolver-pagination # Conflicts: # src/Data/Config.php # src/Data/Cursor/CommentObjectCursor.php # src/Data/Cursor/PostObjectCursor.php # src/Data/Cursor/UserCursor.php
@justlevine I agree that this is a bugfix not a breaking change. Any queries that did change would have just changed the order of the items. Like 10-1 instead of 1-10. But the items will still be there. |
|
Code Climate has analyzed commit 9fc6928 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
What does this implement/fix? Explain your changes.
This PR addresses numerous underlying issue with paginating GraphQL connections, by refactoring
AbstractConnectionResolverto better follow the Relay Connection spec, while using smaller methods to allow easier extension by child classes.Originally surfaced by #2343, a review showed not just regressions caused by #2294, but others dating back to v1.6.x as well.
Initial work was done in #2408 , which became unwieldy due to leaky tests hiding false positives (addressed in #2455).
Changelog
beforeandafterargs.AbstractConnectionResolverto support all parts of Relay Spec, including more robust over-fetching and slicing.{Type}ConnectionResolver::isValidOffset()methods, and existing ones to be (slightly) more performant.firstandlastare the same. [Possibly breaking, see comment below).{Type}ConnectionResolver::get_query_args()now always returns an array.{Type}ConnectionResolver::$queryproperties have been given a type in the docblock.AbstractConnectionResolver::get_offset()has been deprecated in favor ofAbstractConnectionResolver::get_offset_for_cursor().AbstractCursor.$queryto thePostCursorandUserCursorconstructor has been deprecated in favor of passing the$query->query_varsinstead.PostCursor::get_cursor_post()TermCursor::get_cursor_term()andUserCursor::get_cursor_user()have been deprecated in favor ofAbstractCursor::get_cursor_node().graphql_cursor_offsetandgraphql_cursor_comparein favor ofgraphql_before_cursorandgraphql_after_cursor.Does this close any currently open issues?
TBD.
Any relevant logs, error output, GraphiQL screenshots, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Any other comments?
Several existing tests were removed as they are covered more comprehensively by newer ones.This PR does not (yet) include fixes to the{type}ObjectCursorclasses, which are the source for incomplete tests forCommentandMenuItemtypes. These classes still rely on the logic from the now-deprecatedAbstractConnectionResolver::get_offset()There is also an issue when paginating PostObject connections filtered bysearch, which may or may not be connected.A separate issue has been surfaced by these tests wheredraftPosts are not available onmenuItems.nodes.connectedNode, but they are available onconnectedObject. 🤔lastordering should be considered a breaking change. I want to say it isn't, and definitely feel strongly about that for any regressions caused by fix: refactor AbstractConnectionResolver::get_nodes() to prevent children double slicing #2294 , but if theorderwas backwards before 1.6.x, the question is whether this discrepancy between the plugin and Relay is a bug or expected behavior users have built around.Where has this been tested?
Operating System: Ubuntu 20.01 (Wsl2 + devilbox + php8.0.19)
WordPress Version: 6.0.1