-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Feature: Data loader integration #51
Comments
Looking forwards to it on solving n+1 requests. |
I'm not sure how it's supposed to work. Does
It looks like I don't need to do anything at all that involves dataloader. |
@Resolver(() => Post)
class PostResolver {
@Query(returns => [Post])
posts() {
return this.manager.find(Post);
}
@FieldResolver()
comments(@Root() posts: Post[]) {
const postIds = posts.map(post => post.id);
const comments = this.manager.find(Comment, { postId: In(postIds) });
// grouping and providing correct order for dataloader
return posts.map(post =>
comments.filter(comment => comment.postId === post.id)
);
}
} So executing this query: query {
posts {
id
title
comments {
text
}
}
} will call DB two times - one for fetching posts collection, one for fetching comments collection, because comments resolver will be batched. |
This is great. It looks like the dataloader is working under the hood in |
When I've used DataLoader before, I've generally used it at the repository layer, rather than on resolvers. The main benefit of this being that it doesn't leak logic around batching into the service layer (requiring everything in the app to work on n ids rather than 1). The main downside is that it requires me to construct services & repositories per-request. Which I can't really figure out how to do with TypeGraphQL — would require the resolvers & DI context to be constructed per request. What do you think about solving this problem by allowing this, rather than bundling dataloader support into the resolver? |
It's easy to solve - create a global middleware that will create new dataloaders and attach them to context. Then you can use the loaders from context in resolver class methods. const DataLoader: MiddlewareFn = ({context}, next) => {
if (!context.isDataLoaderAttached) {
context.isDataLoaderAttached = true;
context.personLoader = new DataLoader(/* ... */);
// ...
}
return next();
} Of course you would need to provide new context object in I'm not a fan of creating DI Container for each "request" as it denies the idea of single-threaded Node.js. |
Is anyone aware of a workable strategy to get dataloader integrated with type-graphql today? |
@ashleyw you can always fallback to the plain apollo-like resolvers strategy - create dataloaders for your app for each request (like in example above - |
@19majkel94 thanks, I'll look into it! Is there anything you've discovered that prevents the batched resolving you posted above? That seems like it would be a sweet, almost automatic solution! |
This should get you started. Create a import DataLoader = require('dataloader');
import { MiddlewareInterface, NextFn, ResolverData } from 'type-graphql';
import { Service } from 'typedi';
import { Context } from './context.interface';
@Service()
export class DataLoaderMiddleware implements MiddlewareInterface<Context> {
async use({ root, args, context, info }: ResolverData<Context>, next: NextFn) {
if (!context.dataLoader.initialized) {
context.dataLoader = {
initialized: true,
loaders: {}
};
const loaders = context.dataLoader.loaders!;
context.connection.entityMetadatas.forEach(entityMetadata => {
const resolverName = entityMetadata.targetName;
if (!resolverName) {
return;
}
if (!loaders[resolverName]) {
loaders[resolverName] = {};
}
entityMetadata.relations.forEach(relation => {
// define data loader for this method if it was not defined yet
if (!loaders[resolverName][relation.propertyName]) {
loaders[resolverName][relation.propertyName] = new DataLoader((entities: any[]) => {
return context.connection.relationIdLoader
.loadManyToManyRelationIdsAndGroup(relation, entities)
.then(groups => groups.map(group => group.related));
});
}
});
});
}
return next();
}
} Then attach when building your schema: const schema = await buildSchema({
globalMiddlewares: [DataLoaderMiddleware],
resolvers: [__dirname + '/**/*.resolver.ts']
}); Then you can use in your FieldResolvers like so: @FieldResolver()
photos(@Root() user: User, @Ctx() ctx: Context): Promise<Photo[]> {
return ctx.dataLoader.loaders.User.photos.load(user);
} This still requires a bunch of boilerplate. I'd love to figure out a way to automatically create these field resolvers while also getting the schema generated and type safety. |
@19majkel94 Hey, I would like to ask if the current approach with FieldResolver() for batching is working or not. Im trying to use the FieldResolver for batching but its not working. More in description in my code
Query example:
Any help of what im doing wrong, please? Thanks! |
@miro4994 It's an issue, "To Do in Board", so how this is supposed to work? Is it a feature documented in docs? 😄 https://19majkel94.github.io/type-graphql/ |
Well, its morning... My mistake :D Thanks for quick response. |
I've recently created BatchLoader written in TypeScript. |
What does it means "better"? Can you show an example what is better, or maybe show some benchmarks with comparison to the dataloader?
I think that Facebook's DataLoader is a well established solution that is known to every graphql developer, so it will be the first choose for now as it has "battery" included. |
One of the issue of dataloader, is that you have to create a loader per field. So if in your query you have 2 fields that are both related to the same entity, you will have to load them 2 times. But, I guess to start, using dataloader might be the best approch ;-) |
First, batchloader takes care of duplicate keys. For example, for the following code loader.load(1),
loader.load(2),
loader.load(1), with BatchLoader, your batch function will get [1, 2] as keys without duplicate keys, while DataLoader gives you [1,2,1]. There is a bit of optimization to remove duplicate keys. As @apiel noted, with BatchLoader you can map a loader to create another loader to get specific fields or chain another query. usernameLoader = userLoader.mapLoader((user) => user && user.username);
pictureLoader = userLoader.mapLoader((user) => user && user.picture);
usernameLoader.load(userId)
pictureLoader.load(userId) both loaders will be sharing the same You can compose a new loader by chaining loaders as well: chainedLoader = loader1.mapLoader(v => loader2.load(v)).mapLoader(v => ...) @apiel, We also use setTimeout to create batch queue. I don't see why it's a problem tho. Otherwise, you will need to somehow manage the queue yourself. Do you have any reason that you don't like setTimeout? do you have a better approach or use case? |
But we are talking about the cached version:
https://github.com/facebook/dataloader#disabling-cache
So can you show me that in this example? How many DB calls to the tables will DataLoader produces and the BatchLoader? 😉 query {
user(id: 12345) {
name
posts(last: 5) {
title
comments(last: 3) {
content
}
}
} |
I was trying to use custom Repositories from import { EntityRepository, Repository } from "typeorm";
import * as DataLoader from "dataloader";
import { User } from "../types/User";
@EntityRepository(User)
export class UserRepository extends Repository<User> {
private loader: DataLoader<number, User> = new DataLoader(ids => {
return this.findByIds(ids);
});
findById(id: number) {
return this.loader.load(id);
}
} and later in the import { Arg, Int, Query } from "type-graphql";
import { InjectRepository } from "typeorm-typedi-extensions";
import { User } from "../types/User";
import { UserRepository } from "../repositories/UserRepository";
export class UserResolver {
constructor(
@InjectRepository(User) private readonly userRepository: UserRepository,
) {}
@Query(returns => User, { nullable: true })
user(@Arg("id", type => Int) id: number) {
return this.userRepository.findById(id); // It's using DataLoader
}
} So in the query like this: query {
user1: user(id: 1) {
fullName
}
user2: user(id: 2) {
fullName
}
} DataLoader will receive array of IDs However, the query {
user1: user(id: 1) {
fullName
posts {
title
user {
fullName
}
}
}
user2: user(id: 2) {
fullName
}
} as there will be two instances of repositories, one for the So the DataLoader in the Do you know if there is a way of reusing injected repository? I'm new to all the DI stuff in TypeScript. |
Of course you can store dataloader in context.
You can use typeorm-loader: |
I just learnt the hard way that dataloader instance should not be shared application-wide. I'm getting similar caching problem as graphql/dataloader#65 I have to create dataloader instance per request and pass it to the Context. I'm looking for alternatives that can be used for batch requests as well as caching results. |
This comment has been minimized.
This comment has been minimized.
Hi @MichalLytek, Was wondering if there is a way to inject the root array(for N+1 problem) for a query when using NestJS, Thanks, |
@laukaichung Have you tried to use scoped containers with a wrapper class around a dataloader and try that instead? @MichalLytek What would be the cons of the approach I just described? |
I'm experimenting with a custom middleware for this, basically the idea is to attach a Dataloader decorator to fields that you want to batch, and then return the batch function from that resolver: @Resolver(Poll)
export class PollResolver {
// ....
@FieldResolver()
@Dataloader()
votes() {
// Return the batch function
return (polls: Poll[]) => this.batchVotes(polls);
}
async batchVotes(polls: Poll[]) {
return await this.voteRepo.getVotesForPolls(polls.map(poll => poll.uuid));
}
} The Dataloader middleware simply creates the dataloader (if it doesn't already exist) and stores it in a scoped container. Then calls the dataloader and returns the result: // dataloader.middleware.ts
import { createMethodDecorator, ResolverData } from 'type-graphql';
import Dataloader, { BatchLoadFn, Options } from 'dataloader';
import Context, { Service } from 'typedi';
import { ResolverContext } from '../types/resolver-context';
interface Loaders {
[key: string]: Dataloader<any, any>;
}
@Service()
class DataloaderFactory {
loaders: Loaders = {};
makeLoader(id: string, batchFunction: BatchLoadFn<any, any>, options?: Options<any, any>) {
if (!!this.loaders[id]) {
return this.loaders[id];
}
const loader = new Dataloader(batchFunction, options);
this.loaders[id] = loader;
return loader;
}
}
function DataloaderMiddleware<K, V>(options?: Options<K, V>) {
return createMethodDecorator(
async ({ root, context, info }: ResolverData<ResolverContext>, next) => {
const dataloaderFactory = Context.of(context.requestId).get(
DataloaderFactory
);
const batchFunction = await next();
const loader = dataloaderFactory.makeLoader(
info.parentType.toString() + '.' + info.fieldName,
batchFunction,
options
);
return loader.load(root);
}
);
}
export {DataloaderMiddleware as Dataloader} As you can see, we can also pass in the dataloader options to the decorator as an argument, making it possible to provide the cache key function for example. Any feedback would be appreciated. |
Inspired by @micnil middleware implementation I tried experimenting with property decorators instead. I would like to inject a unique data-loader instance for each request so I created a custom parameter decorator @Resolver(() => Book)
export class BookResolver {
@FieldResolver(() => User)
public async author(
@Root() root: Book,
@RequestContainer() userDataLoader: UserDataLoader
): Promise<User> {
return userDataLoader.load(root.userId);
}
} So each data-loader is a service class extending the @Service()
export class UserDataLoader extends DataLoader<string, User | undefined> {
constructor(userService: UserService) {
super(async (ids) => {
const users = await userService.findByIds(ids);
return ids.map((id) => users.find((user) => user.id === id));
});
}
} Finally I would make sure that there is a unique instance of the data-loader for each request, by using a custom parameter decorator: export function RequestContainer(): ParameterDecorator {
return function (target: Object, propertyName: string | symbol, index: number) {
return createParamDecorator<Context>(({ context, info }) => {
const paramtypes = Reflect.getMetadata('design:paramtypes', target, propertyName);
const requestContainer = Container.of(context.requestId);
return requestContainer.get(paramtypes[index]);
})(target, propertyName, index);
};
} The request id is just an It would also be advised to reset these containers after each request. Please read more about this in the type-graphql docs. |
Thanks to @nomadoda idea, I've created an utility type-graphql-dataloader to make use of DataLoader with TypeGraphQL more simple. If an application is using TypeORM, just add @ObjectType()
@Entity()
export class Book {
@Field((type) => ID)
@PrimaryGeneratedColumn()
id: number;
@Field((type) => User)
@ManyToOne((type) => User, (user) => user.books)
@TypeormLoader((type) => User, (book: Book) => book.authorId)
author: User;
@RelationId((book: Book) => book.author)
authorId: number;
} In case of not using TypeORM, @Resolver((of) => Book)
export class BookResolver {
@FieldResolver()
@Loader<number, User>(async (ids) => { // batchLoadFn
const repository = getRepository(User);
const users = keyBy(await repository.findByIds(ids), "id");
return ids.map((id) => users[id]);
})
author(@Root() root: Book) {
return (dataloader: DataLoader<number, User>) =>
dataloader.load(root.authorId);
}
} |
@slaypni that looks good. Now if you have a loader with a complex key, like a pair of keys, how would you use it with @TypeormLoader? |
@tafelito That is a limitation of the current |
@slaypni @MichalLytek could we merge type-graphql-dataloader into TypeGraphQL? |
No, TypeGraphQL is orm-lib agnostic. Why would you want to do that? |
Oh goodness, apologies, I mistook this forum for TypeORM's. |
We've started using @slaypni's type-graphql-dataloader at our company a week ago. It works well and has been saving us a lot of man-hours. |
Seconded! I'm new to TypeGraphQL and expected the API to automatically resolve relations via decorators if requested in the query. type-graphql-dataloader bridges that gap. |
I'm having an issue with this solution for subscriptions. The request id being created on the connection of the subscription, the data loader is not "renewed" for every subscription execution resulting in stale data in my subscriptions. |
I'm adding my solution to the mix, I just added the dataloader to the context so its created for every request. I'm using Mikro-Orm but you can replace the entity manager with any other way you query the database in your application. /index.ts
resolvers/author.ts
|
Status on this? |
https://github.com/facebook/dataloader
Introduce batched resolving - taking array of roots:
The text was updated successfully, but these errors were encountered: