Call 'toJSON' if present for ID and String serialize

[IvanGoncharov]

Fixes #1518

class ObjectId doesn't have valueOf only toString and toJSON:
https://github.com/mongodb/js-bson/blob/5acdebf7a63ed02d3bc1eb8481cdcb709fb5c886/lib/objectid.js#L213-L215

We can't use toString because:

class A {}
const a = new A();
a.toString()
// '[object Object]'

So I added support for toJSON.

[IvanGoncharov]

@alexmcmillan I don't have access to MongoDB so I can't run: https://github.com/alexmcmillan/graphql-mongodb-query-fail-demo

Can you please test it with attached package:
graphql-14.0.2.zip
Just unpack it inside node_modules.

[alexmcmillan]

@IvanGoncharov Can confirm the package you attached does resolve the problem. Thank you - I hope this gets merged soon!

[IvanGoncharov]

@alexmcmillan Thanks 👍
@mjmahone Can you please review?
mongoose is a pretty popular library so I think it's worth to make 14.1.0.
What do you think?

[mjmahone]

This looks good to me, thanks for fixing it!

[mjmahone]

grammar nit: "a complex value" instead of "an complex value"

[IvanGoncharov]

Thanks 👍
Fixed

[mjmahone]

Prior to doing the bump to 14.1.0 I think we'd want @leebyron to help fix our NPM push process: I don't want to accidentally mess up another release, and I don't want Lee to be on the hook for overseeing and validating each patch. Can the version bump wait until the next WG meeting? It's probably worth figuring out with everyone in the same (virtual) room, a process that makes sense. I want us to be able to make fast decisions on situations like this, but for instance I'm not sure why this would be a minor release rather than a patch, and I don't currently have the right framework for figuring out that answer. On the other hand, @IvanGoncharov, you may have that framework well thought-out, which is why I'd like to make sure we can make these decisions quickly, without appealing to "higher authorities".

[IvanGoncharov]

Prior to doing the bump to 14.1.0 I think we'd want @leebyron to help fix our NPM push process: I don't want to accidentally mess up another release, and I don't want Lee to be on the hook for overseeing and validating each patch

Agree, we definitely need to improve bus factor 👍

Can the version bump wait until the next WG meeting?

Yes 👍

I'm not sure why this would be a minor release rather than a patch, and I don't currently have the right framework for figuring out that answer.

AFAIK, it covered by semver:
https://semver.org/#spec-item-7

It MAY be incremented if substantial new functionality or improvements are introduced within the private code.

My personal rule of thumb: if you add something new that clients explicitly depend on it means you need to do minor release. So if someone write the resolvers that depend on toJSON that mean this will work only on >= 14.1.0 and if for somereason we release 14.0.3 it wouldn't contain this feature.

It also important for 3rd tools like graphql-tools and other graphql middlewares since they wrapping resolvers and may be surprised by object with toJSON so they have an option to support only 14.0.x and be shielded from new features.

So for me this change is not a bug fix but a new functionality especially since 0.13.x and previous version didn't use toJSON (they used toString).

[ruiaraujo]

!= nulll

[IvanGoncharov]

> typeof undefined
'undefined'

@ruiaraujo So it will fail on the first check.

[ruiaraujo]

indeed, missed that.

[leebyron]

This change is actually breaking - there may be objects which define both valueOf and toJSON which provide different values. If those objects previously worked correctly, this PR would cause them to work incorrectly.

If the goal is to also support objects which define toJSON but do not define valueOf then this could be easily solved by just moving this new addition to below the existing check for valueOf

[IvanGoncharov]

I did that because:

> typeof ({}).valueOf
'function'

But I agree it breaking so I updated this PR to have this instead:

    if (typeof value.valueOf === 'function') {
      const valueOfResult = value.valueOf();
      if (typeof valueOfResult !== 'object') {
        return valueOfResult;
      }
    }
    if (typeof value.toJSON === 'function') {
      return value.toJSON();
    }

I also changed tests to explicitly test precidence.
Can you please review new code and tests?

[IvanGoncharov]

@mjmahone Since you reviewed this PR and I already addressed @leebyron comments I merging this issue, so people can atleast use our npm branch.
Would be great to disscuss the scope for 14.1.0 release, can you please comment in #1543 ?

[xabra]

I am having this problem also. Using "graphql": "^14.0.2"
"ID cannot represent value: { _bsontype: "ObjectID"
In a query resolver such as below, can someone explain what I do to fix it?
Thanks

    user: async (parent, args, context, info) => {
      let user = await context.UserModel.findOne(args);
      return user;
    }

[frankRose1]

I am having this problem also. Using "graphql": "^14.0.2"
"ID cannot represent value: { _bsontype: "ObjectID"
In a query resolver such as below, can someone explain what I do to fix it?
Thanks

    user: async (parent, args, context, info) => {
      let user = await context.UserModel.findOne(args);
      return user;
    }

I know its a bit of a late response but In regards to xabra's post try this:

user: async(parent, args, ctx, info){
     let user = await ctx.UserModel.findOne(args);
     return { ...user._doc, _id: user._id.toString() };
}

[EmiyaYang]

Well, I have no problems to query the _id field. But the same error happens while I try to populate sth.

  function findOne(id: string) {
    const res = this.nodeModel.findOne({ id }).populate('parent');

    return res;
  }

---

Finally I have this problem solved. The main issue is that Document.prototype.toJSON() return an object instead of a string. REF: document_Document-toJSON

As a workaround, set a transformer like this:

NodeSchema.set('toJSON', {
  transform(doc, ret) {
    return JSON.stringify(ret);
  },
});