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

Parse ObjectIDs using BSON #973

Closed
wants to merge 1 commit into from

Conversation

tubbo
Copy link

@tubbo tubbo commented Jul 23, 2021

Description

The ObjectID scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a bson.ObjectId type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.

This change adds a dependency on the bson library so we can use its
ObjectId class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.

Example of code before this change:

someResolver: (_parent, { id }, { db }) => {
  const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })

  return someData
},

And here's what it will look like afterward:

someResolver: (_parent, { id }, { db }) => {
  const someData = await db.collection('things').findOne({ _id: id })

  return someData
},

Similar to Date objects which are serialized appopriately in MongoDB
if you use Timestamp, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.

Related #429

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Screenshots/Sandbox (if appropriate/relevant):

Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate

How Has This Been Tested?

For the following tests, create a query using an ObjectID both in its
field arguments and the return type:

type Returned {
  id: ObjectID!
}

type Query {
  returned(id: ObjectID!): Returned!
}

query ReturnStuff($id: "5e5677d71bdc2ae76344968c") {
  returned(id: $id) {
    id
  }
}
  • In the resolver function for Query.returned, the id passed in
    should be an instanceof ObjectId.
  • When the resolver returns its result, the Returned.id should be
    serialized to the client as a hexadecimal string.
  • When passing in an argument as a String literal in GraphQL, this
    should be serialized to an ObjectID when passed into the resolver.

Test Environment:

  • OS: macOS 11.3
  • GraphQL Scalars Version: 1.10.0
  • NodeJS: 12.22.1

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

I'm hoping that the solution I came up with to only import ObjectId from the BSON library is sufficient to maintain the small bundle size of this library.

@tubbo tubbo force-pushed the return-objectid-from-scalar branch from 48e1b5f to c33b4fa Compare July 23, 2021 15:45
The `ObjectID` scalar type does some great work ensuring that the data
in the database maps to valid ObjectID values for MongoDB. However, in
order to use arguments supplied as ObjectID, we still need to convert
them to a `bson.ObjectId` type on-the-fly in resolvers or else we'll end
up not matching documents that should be matched.

This change adds a dependency on the `bson` library so we can use its
`ObjectId` class as a means of serializing ObjectID data from the
GraphQL API into a usable object for resolvers.

Example of code before this change:

```javascript
someResolver: (_parent, { id }, { db }) => {
  const someData = await db.collection('things').findOne({ _id: new ObjectId(id) })

  return someData
},
```

And here's what it will look like afterward:

```javascript
someResolver: (_parent, { id }, { db }) => {
  const someData = await db.collection('things').findOne({ _id: id })

  return someData
},
```

Similar to `Date` objects which are serialized appopriately in MongoDB
if you use `Timestamp`, ObjectIDs should be parsed properly into their
correct type before persistence into the database. By doing so, we're
ensuring that this type is always consistent.

Resolves Urigo#429
@tubbo tubbo force-pushed the return-objectid-from-scalar branch from c33b4fa to 8d5102e Compare July 23, 2021 15:49
@ardatan
Copy link
Collaborator

ardatan commented Jul 23, 2021

Have you seen this conversation?
#429 (comment)

@tubbo
Copy link
Author

tubbo commented Jul 23, 2021

Yes @ardatan, I was hoping that this would be a sufficient solution to the bundle size problem. Although this still depends on bson as a package, it only imports bson/lib/objectid, which does not include the entire implementation of BSON in a tree-shaken environment. I did a local test by compiling my branch and then master, and the bundle size only increased by about 34 bytes. This is because of the direct bson/lib/objectid import, so we can discard the entire rest of the module since we only want the object definition here.

master:

drwxr-xr-x  14 tscott  staff     448 Jul 23 12:06 .
drwxr-xr-x  30 tscott  staff     960 Jul 23 12:06 ..
-rw-r--r--   1 tscott  staff    1076 Jul 23 12:06 LICENSE
-rw-r--r--   1 tscott  staff    2524 Jul 23 12:06 README.md
-rw-r--r--   1 tscott  staff     414 Jul 23 12:06 RegularExpression.d.ts
-rw-r--r--   1 tscott  staff    9860 Jul 23 12:06 index.d.ts
-rw-r--r--   1 tscott  staff  110965 Jul 23 12:06 index.js
-rw-r--r--   1 tscott  staff  182852 Jul 23 12:06 index.js.map
-rw-r--r--   1 tscott  staff  107086 Jul 23 12:06 index.mjs
-rw-r--r--   1 tscott  staff  182389 Jul 23 12:06 index.mjs.map
-rw-r--r--   1 tscott  staff    2356 Jul 23 12:06 mocks.d.ts
-rw-r--r--   1 tscott  staff     792 Jul 23 12:06 package.json
drwxr-xr-x  51 tscott  staff    1632 Jul 23 12:06 scalars
-rw-r--r--   1 tscott  staff    2758 Jul 23 12:06 typeDefs.d.ts

this branch:

total 1232
drwxr-xr-x  14 tscott  staff     448 Jul 23 12:05 .
drwxr-xr-x  30 tscott  staff     960 Jul 23 12:06 ..
-rw-r--r--   1 tscott  staff    1076 Jul 23 12:05 LICENSE
-rw-r--r--   1 tscott  staff    2524 Jul 23 12:05 README.md
-rw-r--r--   1 tscott  staff     414 Jul 23 12:05 RegularExpression.d.ts
-rw-r--r--   1 tscott  staff    9860 Jul 23 12:05 index.d.ts
-rw-r--r--   1 tscott  staff  110999 Jul 23 12:05 index.js
-rw-r--r--   1 tscott  staff  182905 Jul 23 12:05 index.js.map
-rw-r--r--   1 tscott  staff  107074 Jul 23 12:05 index.mjs
-rw-r--r--   1 tscott  staff  182421 Jul 23 12:05 index.mjs.map
-rw-r--r--   1 tscott  staff    2356 Jul 23 12:05 mocks.d.ts
-rw-r--r--   1 tscott  staff     813 Jul 23 12:05 package.json
drwxr-xr-x  51 tscott  staff    1632 Jul 23 12:05 scalars
-rw-r--r--   1 tscott  staff    2758 Jul 23 12:05 typeDefs.d.ts

I'm not using ls -lah here because if you do that, they look exactly the same. I don't think there's a significant bundle size increase here by doing it this way. The only trade-off is that we need to vendor the type definition in types/bson.d.ts, because BSON does not compile with .d.ts files for everything in lib/.

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

Successfully merging this pull request may close these issues.

2 participants