Webtomizer / typeorm-loader

A database-aware data-loader for use with GraphQL and TypeORM.
82 stars 8 forks source link

invalid reference to FROM-clause entry #5

Open Mando75 opened 5 years ago

Mando75 commented 5 years ago

I was going to build something just like this package when I stumbled across your reddit post sharing this. Thanks for putting this together!

Just brief spec/version information:

TypeORM: "0.2.17" Postgres: "0.2.16" Typeorm-loader: "0.0.1-0.5" ApolloServer Express: "2.0.7" GraphQL: "14.2.0" Typescript: "3.3.4000"

I'm running into an issue when loading the nested relationships via the info argument to the loader. I've tried it with both regular and lazy relations in typeorm, and I'm getting the same issue regardless. It looks like an issue with constructing the joins for the nested relationships. The error it spits out is error: invalid reference to FROM-clause entry for table "Q"

Here's a pastebin of the entire error log: https://pastebin.com/97PcamW9

When examining the query, it looks to me that this error could easily be fixed by modifying the FROM statement...

Original query:

SELECT "Q"."id"           AS "Q_id",
       "Q"."name"         AS "Q_name",
       "Q"."active"       AS "Q_active",
       "Q"."created_date" AS "Q_created_date",
       "Q"."updated_date" AS "Q_updated_date",
       "Q"."guide_id"     AS "Q_guide_id",
       "Q"."id"           AS "Q_id",
       "Q_Group"."id"     AS "Q_Group_id",
       "Q"."id"           AS "Q_id",
       "Q"."guide_id"     AS "Q_guide"
FROM "groups" "Q",
     "guides" "Guide" -- Selecting from this table is what causes the error
         LEFT JOIN "guides" "Q_Group" ON "Q_Group"."id" = "Q"."guide_id"
WHERE "Q"."id" = 'd4b6581c-9522-4093-a4a2-27b0ff3181af';

Modified that seems to execute properly:

SELECT "Q"."id"           AS "Q_id",
       "Q"."name"         AS "Q_name",
       "Q"."active"       AS "Q_active",
       "Q"."created_date" AS "Q_created_date",
       "Q"."updated_date" AS "Q_updated_date",
       "Q"."guide_id"     AS "Q_guide_id",
       "Q"."id"           AS "Q_id",
       "Q"."id"           AS "Q_id",
       "Q"."guide_id"     AS "Q_guide",
        "Q_Group"."id"    AS "Q_Group_id"
FROM "groups" "Q" 
-- Remove the extra from statement
LEFT JOIN "guides" "Q_Group" ON "Q_Group".id = "Q"."guide_id"
WHERE "Q"."id" = 'd4b6581c-9522-4093-a4a2-27b0ff3181af';

I'd be willing to help figure out and fix what is causing the issue if you could point me in the right direction in your package.

Example query:

{
  group(id: "d4b6581c-9522-4093-a4a2-27b0ff3181af'") {
    id
    guide {
      id
    }
  }
}

Relevant entity fields: Group:

@JoinColumn()
  @ManyToOne(() => Guide, guide => guide.groups, { nullable: false })
  guide: Guide;

Guide:

  @OneToMany(() => Group, group => group.guide, { nullable: true })
  groups: Promise<Group[]>;

Resolver function:

Query: {
    async group(_: any, { id }: GQL.IGroupOnQueryArguments, { loader }, info) {
      return await loader.loadOne(Group, { id }, info);
    }
  }

Please let me know if you need any more info. I would be happy to help.

Mando75 commented 5 years ago

From my tinkering, it looks like the loader is creating an invalid query due to adding an additional FROM query when the guide table should actually just be joined, not SELECT FROM'ed. Looks like it's an error in the logic of the select method when generating the query.

alexolivier commented 5 years ago

Commenting out this line fixes

https://github.com/Webtomizer/typeorm-loader/blob/f49b9fa87996c1d9642e1c53a7ac45edb3a313b5/src/select.ts#L115

Mando75 commented 5 years ago

@alexolivier I had tried that when looking into it on my own, and while it did generate a valid SQL query after removing addFrom, the queue/cache then returned null back for the record. I will try again to see if maybe I accidentally tweaked something else without realizing it. While I do think it may be a good workaround, would it not then break the idea of fetching multiple top level type from the query object? i.e.

query {
   type1 {
      subfield
   }
   type2 {
      subfield
   }
}

From the bit I spent playing around with the select method, it seems the addFrom is meant to allow you to select multiple top level query types and then nest the joins for each. Removing the addFrom may break that functionality. Not 100% sure though. I'll play around with it some more and report back any findings. Would love to get some feedback from the author :).

EDIT: Commenting out that line did fix it this time. I must have messed with something else when I when I commented it out before. Still don't know if this will break other features or not, but going ahead with it for now.

alexolivier commented 5 years ago

Glad it's working - but it does seem the push down of which fields to load isn't working. still effectively doing a SELECT * on the tables :/

Mando75 commented 5 years ago

Yeah I noticed that as well, though it seems to be only happening on the top level type, the nested relations seem to only load the fields being queried. It's not a huge concern to me at the moment, as I'm looking more for the nested joining for query optimization. I did find another issue when having multiple joined relations on a query, the selector was trying to use the same alias for all of the join tables. Changing the childAlias to use the relation name instead of the target name seemed to fix that:

selectModifications

My plan for now is to just keeping using this and fixing issues I find along the way until I'm confident everything is working right. Then I'll probably make a PR that includes all the fixes. I'll keep updating this thread with any new problems/solutions I find.

zhenwenc commented 5 years ago

Sorry for posting a question here.

I landed on this package as I am also looking a "loader" for TypeORM and GraphQL. I though this package would use the same batching mechanism as DataLoader

A batch loading function accepts an Array of keys, and returns a Promise which resolves to an Array of values*.

Then load individual values from the loader. DataLoader will coalesce all individual loads which occur within a single frame of execution (a single tick of the event loop) and then call your batch function with all requested keys.

But seems this is not the case by looking into the source code (but I might by wrong though), which is simply batching the query executions and cache the results. From what my understanding, TypeORM already provided such feature (caching).

Then what's the benefit of using this package or use case that TypeORM cache doesn't cover?

Thanks in advance for any reply and sorry for annoying anyone!

Mando75 commented 5 years ago

@zhenwenc No worries, it's a good question.

The reason I am using this package is because of the deep relationship nesting it provides for TypeORM entities. With normal TypeORM relations, you need to specify which entity relations you want to load via the queryBuilder or find APIs, e.g.

User.findOne(id, {relations: ["posts"])
User.createQueryBuilder("u")
        .leftJoinAndSelect("u.posts", "p")
        .where("u.id = :id", {id})
        .getOne()

If you don't specify the relation on load, the resulting user.posts will be null. This can present a problem if your GraphQL Query type resolvers don't know which relations to load. There are two methods I have found to alleviate this, use Promise based relations (what I did previously), or parse the request tree to figure out which relations to load on every request (you could just always load all of them, but that doesn't scale well).

I was using Promise based relations for a while, but it began to clog up my business logic code with many await calls to load the relations when I wanted to just load them right away. I started building a GraphQL AST parser to start to deep load the TypeORM relations I needed on each query, but I found this package which is supposed to do that for you.

I haven't really looked into using this as a replacement for the regular dataloader, I was planning on using them in tandem, but from what the author has written, it should be performing that same functionality (see this article, about half-way through he explains it https://dev.to/voodooattack/cloudpresspart-2-cmx-the-user-friendly-variant-of-jsx-boo). I haven't looked far enough into it to be sure though, I've mostly been playing with the select method, and any "dataloader" emulation would be happening in the load methods. I would imagine that because this is TypeORM specific, you don't need to specify your batch function because we already know the method which will be used to fetch the data (queryBuilder api). Instead you would just need to add in your where conditions (normally your id fields) for all the requests to be folded in on each other. That's mostly speculation though, like I mentioned, I haven't looked too far into it.

zhenwenc commented 5 years ago

@Mando75 Thanks very much for sharing the knowledge! Much appreciated!

If you don't specify the relation on load, the resulting user.posts will be null. This can present a problem if your GraphQL Query type resolvers don't know which relations to load.

I am also trying to find an easy way to solve this problem which also leads to the ugly N+1 issue. But one difficulty is, the GraphQL object type field names are not guaranteed to be the same as TypeORM relation names. For example:

# GraphQL schema

type User {
  posts: [Post] // it's "posts" here
}

# TypeORM

@Entity()
class User {
  @OneToMany(_ => Post, post => post.owner)
  ownPosts: Post[];  // but "ownPosts" here
}

...

This package does it by optimistically assume that the relation field names are the same in GraphQL and TypeORM definitions. here

One possible idea is, we can use GraphQL directives to explicitly specify the TypeORM relations for each object type fields.

Mando75 commented 5 years ago

You could easily do that with something like TypeGraphQL with TypeORM, though I'm not sure if that would then remove the need for this package. For me, most of my GraphQL schemas tend to match up with the entity fields. When it doesn't, I just define a custom resolver on that field/type

User: {
   posts: async (parent, args, context, info) => {
       return parent.ownPosts
   }
}

This is where you may have trouble with the relation not being loaded. This is an example of when you may want to use facebook dataloader to wrap the typeorm-loader. Maybe something like this (pseudo code)?

// create a closure so we can use info
postsLoader = (info: GraphQLInfo) => new DataLoader((ids) => {
      // typeorm dataloader
      return new GraphQLDataLoader(Post, {user: ids}, info
})

Then in your resolver do the something like the following:

User: {
   posts: async (parent: User, args: any, {postsLoader}: Context, info: GraphQLInfo) => {
       return postsLoader(info).load(parent.id)
   }
}

Fair warning, I didn't try this at all, and I suspect you have issues with using a closure to always return a new facebook Dataloader instance in each resolver call. You generally want one DataLoader/batch loader instance for the full request so caching/folding works correctly. Maybe you could grab the info when creating the loader within the GraphQL context creation. I'm not entirely sure, but I think that is a step in the direction you would want to move towards if you wanted to avoid adding decorators. I hope all that made sense.

zhenwenc commented 5 years ago

Yes, they do made sense. And I am using TypeGraphQL with TypeORM, they are amazing!

Again, thanks a lot for sharing! I will try this approach! 🍻

amille14 commented 4 years ago

Any update on this? I'm running into the same invalid reference to FROM-clause entry for table "Q" issue

Mando75 commented 4 years ago

@amille14 I forked this project, fixed a bunch of issues and added some new features. You can try it out and see if it works. I hadn't heard anything from the author, so I published my changes to npm a bit ago. I'm more than willing to merge them back together, but the lack of activity in this repo doesn't get my hopes up. https://www.npmjs.com/package/@mando75/typeorm-graphql-loader If you decide to check it out and run into any issues, please make an issue in the repo linked in npm.