Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡️ DataLoader #722

Merged

Conversation

Projects
None yet
6 participants
@jasonbahl
Copy link
Collaborator

jasonbahl commented Mar 14, 2019

Add DataLoader support

DataLoader is a concept of batching and caching within requests to reduce the number of underlying database queries needed to fetch the required data.

In GraphQL, it's common that resolvers lead to a n+1 problem, where one query is executed, then "n" more queries are executed to resolve the connected objects.

WPGraphQL was largely guilty of this problem.

Take the following query as an example:

{
  posts( first: 10 ) {
    nodes {
       id
       title
       author {
         name
       }
     }
  }
}

This would execute 1 query to get the 10 posts, then 10 queries to get the author for Post 1, Post 2, etc

With this PR, this would execute 2 queries.

The first query would execute to get the posts, then the post_author for each post is cached in a buffer, then a single deferred resolver is executed to fetch all 10 (or fewer if there are duplicates) authors, thus saving us up to 9 queries!

Prior to this PR, we had an initial implementation for Deferred resolvers for post authors, but that was the only place we were making use of this concept of Deferred resolvers for batch queries.

Now, it's implemented pretty much everywhere. There are other improvements as well, so even the DataLoader setup for authors is more efficient than it was previously.

Examples

Here are some before/after examples showing the improvements:

Example 1:

  • before: 20 SQL Queries
  • after: 7 SQL queries
{
  posts {
    nodes {
      id
      title
      date
      author {
        name
      }
    }
  }
}

Example 2

  • before: 114 SQL Queries
  • after: 67 SQL queries
{
  categories {
    nodes {
      id
      name
      posts {
        nodes {
          id
          title
          author {
            name
            posts {
              nodes {
                id
              }
            }
          }
        }
      }
    }
  }
}

jasonbahl and others added some commits Feb 26, 2019

Merge pull request #711 from wp-graphql/master
Sync Develop with Release v0.2.2
📝 Updating links to docs site and repo
📝 Updating links to docs site and repo
Improve global post test
The global post is not used if the post_excerpt is already set.
Change it to empty string to force post_excerpt generation via
the global post.
DATALOADER!!!!
- Add Loaders to AppContext
- Replace all instances of non-deferred resolvers for posts and users with deferred resolvers
- Remove no-longer-used `Data/Loader.php`

- TESTS ARE ALL PASSING
DATALOADER!!!!
- Add Loaders to AppContext
- Replace all instances of non-deferred resolvers for posts and users with deferred resolvers
- Remove no-longer-used `Data/Loader.php`

- TESTS ARE ALL PASSING

------

Merge commit '423055660be4db976aec51ca47ceb35dffadcc9e' into dataloader-03012019

# Conflicts:
#	src/Data/Loader/PostObjectLoader.php
Setup postdata on field levels
Setting it up in resolve_post_object is too late because it fires before
the resolvers as resolvers can be called with different posts.

This also ensures that the global post is reset correctly after the
request.
Merge pull request #714 from epeli/global-post-test
Setup postdata on field levels
- Add CommentLoader to AppContext
- Add $context to all calls for `DataSource::resolve_comment()`
- use DataSource::resolve_menu_item for resolving menu items.
- Add context to the loaders
- Have the PostObjectLoader batch load authors if the author is set, as the author object is queried and set via setup_postdata(), so this batch loads the users so they're already in the cache when setup_postdata() is called.
Merge commit '8cf20c40fd94b43eb0f5e2414ce08124c39aa70d' into dataload…
…er-03012019

# Conflicts:
#	src/Data/Loader/PostObjectLoader.php
Merge branch 'develop' of github.com:wp-graphql/wp-graphql into datal…
…oader-03012019

# Conflicts:
#	src/Data/DataSource.php
- Added TermObjectLoader
- Cleanup some docblock comments
DataLoader
- replace `DataSource::resolve_post_objects_connection` internals with the new PostObjectConnectionResolver
- added a new PostObjectConnectionResolver
- adjusted a @Property comment in Model/Post
- adjusted some tests to better reflect restrictions against querying private statuses. . .we now restrict users from even querying "private" content

TESTS ARE PASSING 100%!!!!!!!!!!!!!!!!

jasonbahl added some commits Mar 15, 2019

DataLoader
- fix issue with the Term model returning the entire Taxonomy object instead of a reference to it
@renatonascalves

This comment has been minimized.

Copy link
Contributor

renatonascalves commented Mar 20, 2019

That's fantastic!

jasonbahl added some commits Mar 20, 2019

DataLoader Updates
- Updates to separate Taxonomy from TermObject model
DataLoader
Continued implementation
- Removing deprecated files instead of trying to maintain them. Just …
…will need to document the upgrade path really well, but I think that will ultimately be easier than trying to preserve and maintain backward compatiblity
@hughdevore
Copy link
Collaborator

hughdevore left a comment

Awesome work @jasonbahl 👏 just some formatting stuff

Show resolved Hide resolved src/Data/Connection/TermObjectConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/TermObjectConnectionResolver.php
Show resolved Hide resolved src/Data/MenuItemConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/TermObjectConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/UserConnectionResolver.php
Show resolved Hide resolved tests/wpunit/TermObjectQueriesTest.php
Show resolved Hide resolved tests/wpunit/TermObjectQueriesTest.php
Show resolved Hide resolved tests/wpunit/TermObjectQueriesTest.php Outdated
Show resolved Hide resolved tests/wpunit/UserObjectQueriesTest.php Outdated
Show resolved Hide resolved tests/wpunit/UserObjectQueriesTest.php Outdated
@CodeProKid
Copy link
Member

CodeProKid left a comment

@jasonbahl still not done, but wanted to share some comments that I've got so far. I also don't want GH to lose these 😆

Show resolved Hide resolved src/AppContext.php
Show resolved Hide resolved src/Data/Connection/AbstractConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/CommentConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/PostObjectConnectionResolver.php Outdated
Show resolved Hide resolved src/Data/Connection/PostObjectConnectionResolver.php Outdated
Show resolved Hide resolved src/Data/Connection/UserConnectionResolver.php
Show resolved Hide resolved src/Data/Connection/UserRoleConnectionResolver.php Outdated
Show resolved Hide resolved src/Data/DataSource.php Outdated
Show resolved Hide resolved src/Data/Loader/AbstractDataLoader.php
Show resolved Hide resolved src/Data/Loader/AbstractDataLoader.php
@CodeProKid
Copy link
Member

CodeProKid left a comment

@jasonbahl mostly just some additional comments here.

return $parent;
},
'userId' => function() {
return ! empty( $this->comment->user_id ) ? $this->comment->user_id : null;

This comment has been minimized.

Copy link
@CodeProKid

CodeProKid Mar 27, 2019

Member

Should this default to 0 instead of null? We are pretty inconsistent on wether the default is null, or the empty value within the scalar type 0, '', etc...

This comment has been minimized.

Copy link
@jasonbahl

jasonbahl Mar 29, 2019

Author Collaborator

well, if the ID is actually 0 we'd want to return 0, but if the ID is 0 or falsey because it's not a user, we should return null, I think...

🤷‍♂️

Show resolved Hide resolved src/Model/MenuItem.php
Show resolved Hide resolved src/Model/Post.php
Show resolved Hide resolved src/Model/Post.php
Show resolved Hide resolved src/Model/Post.php
Show resolved Hide resolved src/Request.php

jasonbahl added some commits Mar 29, 2019

👌 CODE REVIEW UPDATES
- Updates to DocBlocks and formatting
- Removing some old @todo comments
- Removing some unused `use` statements
- Removing some leftover commented code
- Removing some `codecept_debug()` statements
👌 CODE REVIEW UPDATES
- Updates to DocBlocks and formatting
- Removing some old @todo comments
- Removing some unused `use` statements
- Removing some leftover commented code
- Removing some `codecept_debug()` statements
👌 CODE REVIEW UPDATES
- Updates to DocBlocks and formatting
- Removing some old @todo comments
- Removing some unused `use` statements
- Removing some leftover commented code
- Removing some `codecept_debug()` statements

@jasonbahl jasonbahl merged commit 7bd66d6 into wp-graphql:feature/model-layer Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.