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

Unable to query for MongoDB ObjectIDs #1518

Closed
alexmcmillan opened this issue Sep 10, 2018 · 23 comments · Fixed by #1520
Closed

Unable to query for MongoDB ObjectIDs #1518

alexmcmillan opened this issue Sep 10, 2018 · 23 comments · Fixed by #1520

Comments

@alexmcmillan
Copy link

I recently updated from 0.13.2 to 14.0.2 which includes breaking changes.

This introduced errors with existing queries which include MongoDB Object Ids (probably from #1382):

ID cannot represent value: { _bsontype: "ObjectID", id: <Buffer 5b 96 3d bf 98 0a 04 09 85 c6 6e a1> }

Repository with complete, minimal repeatable example here:

const Thing = mongoose.model('Thing', new mongoose.Schema({
  id: mongoose.Schema.Types.ObjectId,
  name: String
}));

const ThingType = new GraphQLObjectType({
  name: 'thing',
  fields: function () {
    return {
      id: { type: GraphQLID },
      name: { type: GraphQLString }
    }
  }
});

const RootMutation = new GraphQLObjectType({
  name: 'CreateMutation',
  fields: {
    create: {
      type: ThingType,
      description: 'Create new thing',
      args: {
        name: {
          name: 'Name of Thing',
          type: new GraphQLNonNull(GraphQLString)
        }
      },
      resolve: (root, args) => {
        const newThing = new Thing({ name: args.name });
        newThing.id = newThing._id;
        return new Promise((res, rej) => {
          newThing.save(err => {
            if (err) return rej(err);
            res(newThing);
          });
        });
      }
    }
  }
});
@alexmcmillan
Copy link
Author

alexmcmillan commented Sep 10, 2018

Background: I'm a learner. Following this tutorial I encountered this problem which was very confusing trying to debug. It would be nice if there was perhaps a clearer error message (*GraphQLID* cannot represent value... (instead of just ID) would have helped me a lot). Perhaps adding an indication of how to properly cast a MongoDB ObjectID?

@IvanGoncharov
Copy link
Member

@alexmcmillan Thanks for detail description and especially example repo 👍
It should be fixed by #1520


It would be nice if there was perhaps a clearer error message (GraphQLID cannot represent value... (instead of just ID) would have helped me a lot).

Problem is that you can define GraphQL types in SDL without working directly with GraphQL* classes:

type thing {
  id: ID
  name: String
}

So we can't use GraphQLID in error message because it will confuse SDL users.

Perhaps adding an indication of how to properly cast a MongoDB ObjectID?

We can't have mongoose as a dependency so we can't detect that some object is coming from Mongo.

@alexmcmillan
Copy link
Author

@IvanGoncharov that makes perfect sense, thank you for your clear explanations here. Hope your solution gets merged soon so I can come forward to v14 :)

@sibelius
Copy link

sibelius commented Oct 2, 2018

any changes this gets into 14.0.3?

@RyannGalea
Copy link

Yes, I am still stuck using '^0.13.2' as the latest '^14.0.2' continues to give me these issues.

@GlauberF
Copy link

GlauberF commented Oct 3, 2018

I would also like to know if it has been fixed for the ^14.0.2,
Why am I having to use the ^0.13.2

I tested the version ^14.0.2 and returned the same problem! :(

@IvanGoncharov
Copy link
Member

@RadAcademy @GlauberF We plan to include it into the next release.
However since instead of depending on toString we now using toJSON it can't be a patch release. So we are working on 14.1.0 that will include this and a few other features.
We will try to release RC ASAP.

@axe-z

This comment has been minimized.

@IvanGoncharov
Copy link
Member

@axe-z We already have a fix for this issue in #1520
We will try to release it ASAP.

@axe-z
Copy link

axe-z commented Oct 11, 2018

oh thanks, when you are at your first steps like me , nosebleeds are ruff.

@IvanGoncharov
Copy link
Member

@alexmcmillan @sibelius @RadAcademy @GlauberF @axe-z I went ahead and merged #1520.
So now you can use our npm branch as a temporary solution until we figure out how to release 14.1.0:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge

@RyannGalea

This comment has been minimized.

@sachintbits
Copy link

I had the same issue, i resolved this issue by writing this code in the types definition file of graphql

import mongoose from "mongoose";

const { ObjectId } = mongoose.Types;

ObjectId.prototype.valueOf = function() {
  return this.toString();
};

@sibelius
Copy link

should this be done on mongoose instead of graphql-js?

@vkarpov15 does it make sense to ObjectId implement valueOf?

@vkarpov15
Copy link

@sibelius not a bad idea. Opened up a mongoose issue to track this

@sibelius
Copy link

sibelius commented Dec 4, 2018

@vkarpov15 tks, keep the good work

@ocbrollingpaper
Copy link

still not fixed?

@vkarpov15
Copy link

I don't use graphql so I'm not certain whether this is fixed, but this might help: http://thecodebarbarian.com/whats-new-in-mongoose-54-global-schematype-configuration.html

@sibelius
Copy link

sibelius commented Jan 7, 2019

@infinitegachi try to use this mongoose PR Automattic/mongoose#7353

@sibelius
Copy link

sibelius commented Jan 7, 2019

I've made a field resolver to help on this

import { GraphQLNonNull, GraphQLString } from 'graphql';
import { Types } from 'mongoose';

export const mongooseIDResolver = {
  _id: {
    type: GraphQLNonNull(GraphQLString),
    description: 'mongoose _id',
    resolve: ({ _id }: { _id: Types.ObjectId}) => ({ _id: _id.toString() }),
  },
};

so if you want to expose an _id in your GraphQL, you need a custom type or transform ObjectId to string

@vkarpov15
Copy link

@sibelius just FYI, Mongoose does not support import { Types } from 'mongoose' in general. It should work, but we recommend always using import mongoose from 'mongoose' to be safe because of how mongoose is structured.

@sibelius
Copy link

sibelius commented Jan 7, 2019

@artboard-studio
Copy link

I believe this is fixed in ^14.2.1 and mongoose ^5.4.20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants