TreeMan360 / nestjs-graphql-dataloader

Easily add dataloader to a nestjs/graphql 7+ application with automatic sequencing and a template method
86 stars 24 forks source link

Im not getting data from my .load function. #3

Closed itstheandre closed 4 years ago

itstheandre commented 4 years ago

This is one of my first ever issues on github, so pardon me for some possible lack of etiquette.

I am just now trying to learn more about data loader and nestjs.

I will be able to find my code in this repo - https://github.com/itstheandre/nest

I have am using Mongoose, but I assume its suppposed to get data. This is my dataloader getOptions

  protected getOptions = () => ({
    query: async (keys: Array<string>) => {
      const allUsers = await this.userService.findManyById(keys);
      console.log('allUsers:', allUsers);
      // console.log('allUsers:', allUsers);
      return allUsers;
    },

And that console log gives me the data i need.

However, in my Task Resolver when i console log the response from the userLoader, it just breaks. I never get to that console log .Help?

  @ResolveField(returns => User)
  async owner(
    @Parent() task: Task,
    @Loader(UserLoader) userLoader: DataLoader<User['_id'], User>,
  ) {
    const allUsers = await userLoader.load(task.owner);

    console.log('allUsers: inside query resolver', allUsers);
    return allUsers;
    /

the data that the graphql playground gives me is: "User does not exist (5e9ae8932ba831ad0dbec94a)", and i know for a fact that it does.

I know i might be wrong somewhere, jst dont know. Please help 🙏

itstheandre commented 4 years ago

I created two users and some dummy tasks on my db to test how this works, but i simply cant figure out where I am going wrong

TreeMan360 commented 4 years ago

I think i know what your problem is.

Your id property is _id. If you do not specify the propertyKey it defaults to id. So in this case it is trying to order your results by id which does not exist hence the error.

If you change your getOptions like so I imagine it will work:

 protected getOptions = () => ({
    propertyKey: '_id',
    query: async (keys: Array<string>) => { ....
itstheandre commented 4 years ago

I tried it with propertyKey: "_id"

and I still get the same error. "User does not exist (5e9ae8932ba831ad0dbec94a)",

inside my getOptions I am still getting the allUsers nicely consol logged. but it never really surfaces back to the resolver

itstheandre commented 4 years ago

For more data, i added a trycatch and this is my error message:

e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae8932ba831ad0dbec94a)
e: User does not exist (5e9ae99895dc11ae07825e95)
TreeMan360 commented 4 years ago

OK when I have some time I can investigate this further for you. It would be more useful if you supplied me with your other logs such as the ones you mention in getOptions. I don't want to have to pull and build your example.

itstheandre commented 4 years ago

ok, will try these steps, one second.

my log using the _id in the get options:

allUsers: inside getoptions [
  {
    _id: 5e9ae8932ba831ad0dbec94a,
    name: 'Andre',
    age: 25,
    occupation: 'Teacher',
    __v: 0
  },
  {
    _id: 5e9ae99895dc11ae07825e95,
    name: 'Catarina',
    age: 25,
    occupation: 'Teacher',
    __v: 0
  }
]

if I just remove the await and simple return the promise i get the same errors.

itstheandre commented 4 years ago

by just getting the promise from userLoader.load I hadded a .catch and got the same errors I mean

TreeMan360 commented 4 years ago

Ok that is strange I will have to try and replicate this to help you further. I am very busy at the moment but this is just a very very simple wrapper around the dataloader library. It is a single file that you can easily pull apart and work out the issue.

I use this in production without any issues and I just put it out there just in the event anybody finds this useful.

itstheandre commented 4 years ago

if you have to pull, and in order not to have to go thorugh the whole code, this is the structure of building a user

mutation {
  createUser(name:"Andre" , occupation:"Instructor", age:25) {
   age
    name
    _id
    occupation
  }
}

and this is the basics for the createTask

mutation {
  createTask(title:"Fourth Task", owner:"ADD_A_USER_ID_HERE") {
    completed
    _id
    title
    owner {
      name
    }
  }
}

when I create a task this is the console i get

allUsers: inside getoptions [
  {
    _id: 5e9ae8932ba831ad0dbec94a,
    name: 'Andre',
    age: 25,
    occupation: 'Teacher',
    __v: 0
  }
]
Error: User does not exist (5e9ae8932ba831ad0dbec94a)

As you can see. inside the getOptions i am finding the right user, but it doesnt surface

TreeMan360 commented 4 years ago

I wont't execute your code in that fashion I will take all of that out as it is not relevant to me. I will create a static data structure which works with the loader which has the same shape you are working with here.

To be honest that is the kind of specific test case I would expect to have submitted to me in an issue rather than the application scaffold.

itstheandre commented 4 years ago

Yeah yeah , whwnever you have some time. Even asnwering here is very much appreciated. I am just trying to figure out the dataloader. Oh, I am sorry. How could i submit better for a next time? Dont want to bother anyone

TreeMan360 commented 4 years ago

No I didn't mean it like that but for example you could just have a dataloader which does a return promise.resolve with the data structure and a resolver with a single field resolver and no database code, no services or models etc. Just the code which applies to the issue at hand.

Generally with a problem like this that is a good route to diagnosing the issue yourself anyway.

Either way it's no bother, happy to help. I will try and spend some time on this in the next 24 hours and let you know. I really should write some tests for the library at some point.

itstheandre commented 4 years ago

ah, I see. THank you for the advice. I have been looking at this for a few hours and after relentless code searching I reached out. I will try to make this even simpler with a simple async function that returns a string or so, to see if that is still happening

itstheandre commented 4 years ago

Thank you so much! That is really really appreciated. If theres anything that I can do from my side to make your job easier, please dont hesitate to say! I will try to jump on this quickly so that you dont waste a lof ot time waiting for me

TreeMan360 commented 4 years ago

Hey,

OK so I made seemingly equivalent example and typically it worked first time for me so something must be different:

@ObjectType()
class TestResponse {
  @Field(type => ID)
  public _id: string

  @Field()
  public name: string

  @Field()
  public age: number

  @Field()
  public occupation: string

  @Field()
  // Not related but note that when using __v you will receive:
  // GraphQLError: Name "__v" must not begin with "__", which is reserved by GraphQL introspection. 
  public _v: number
}

@Injectable()
class TestResponseLoader extends OrderedNestDataLoader<
  string,
  TestResponse
> {
  constructor() {
    super()
  }

  protected getOptions = () => ({
    propertyKey: '_id',
    query: (keys: string[]) =>
      Promise.resolve([
        {
          _id: '5e9ae8932ba831ad0dbec94a',
          name: 'Andre',
          age: 25,
          occupation: 'Teacher',
          _v: 0,
        },
        {
          _id: '5e9ae99895dc11ae07825e95',
          name: 'Catarina',
          age: 25,
          occupation: 'Teacher',
          _v: 0,
        },
      ] as TestResponse[]),
  })
}

...

  @ResolveField(returns => TestResponse)
  public async test(
    @Loader(TestResponseLoader)
    testLoader: DataLoader<string, TestResponse>,
  ) {
    return testLoader.load('5e9ae99895dc11ae07825e95')
  }

...

Then in my graphql query response I receive:

        "test": {
          "_id": "5e9ae99895dc11ae07825e95",
          "name": "Catarina",
          "age": 25,
          "occupation": "Teacher",
          "_v": 0
        }

Given this test I would expect your code to work. Perhaps you can take this and see if it can help you resolve your problem. If I have missed anything let me know.

Good luck and I hope you are safe and well in these strange times.

TreeMan360 commented 4 years ago

Are you 100% sure when you tested with propertyKey something else didn't go awry and give you a false negative? This really does seem to be the likely cause of your issue given the outcome.

itstheandre commented 4 years ago

I think I figured it out. So your code immediately works. The id comes from that Promise.resolve as a string. And thats how i defined it. However, an id in Mongoose and Mongo is an object.

So I guess i maybe have to turn these objects into strings? Or maybe accept an object in the model?

What do you think?

itstheandre commented 4 years ago

yep, thats it. It seems to be kind of a hacky way to achieve this. byt clearly its the fact that the id from mongoose is an object. i dont know if this is a best practice or too hacky.

      const normalizedArr = allUsers.map(el => {
        return { ...el._doc, _id: el._doc._id.toString() };
      });
TreeMan360 commented 4 years ago

Good news. Yeah I was half expecting you to say this actually.

You can convert your Mongo ObjectId instance to a string by doing _id.toString() and you can go back to an ObjectId with ObjectId(stringValue).

I would initially recommend doing this and keeping the key as a string.

itstheandre commented 4 years ago

Yeah, thats what i thought. Definitely the string is easier to work with. So I guess that turning the responses's id to string is what i should do

TreeMan360 commented 4 years ago

You could try having the key as the ObjectId and see if that works however IIRC your model has the _id as a string.

itstheandre commented 4 years ago

in order to do that, i guess i need to define the type as mongoose.Types. ObjectId? Something like that? Or even, maybe, a mixed type? string | mongoose.Types.ObjectId

TreeMan360 commented 4 years ago

Based on this I will close this issue. Glad you are in a better place now.

itstheandre commented 4 years ago

Thank you so much! All the best for you and stay safe out there! :)

TreeMan360 commented 4 years ago

in order to do that, i guess i need to define the type as mongoose.Types. ObjectId? Something like that? Or even, maybe, a mixed type? string | mongoose.Types.ObjectId

Well you need to think about your graphql type and you aren’t going to be able to have a MongoId there unless you create a custom scalar. I would personally stick to the string as I imagine this will work fine and is less complicated.