VulcanJS / vulcan-next

The Next starter for GraphQL developers
http://vulcan-docs.vercel.app
MIT License
395 stars 29 forks source link

ObjectIds and Strings should both work with useUpdate and filters #162

Closed GraemeFulton closed 2 years ago

GraemeFulton commented 2 years ago

Describe the bug Currently, for a user to update a document, the userID of the doc needs to be an ObjectId. Whereas graphQL filters only work for strings, so you can't filter by userID if the ID is an ObjectID.

More detail: If you have documents with relational userID as a String ID (not ObjectId), the useUpdate mutation will give a permission error when you try to edit it, even if you are the owner. To fix it, change the userId in the database to an ObjectId, and useUpdate will then work.

Conversely if you set the userIds to objectIds instead of Strings for all those documents, the useMulti filters will not work when filtering by the userId. useMulti filter will only work with strings.

To Reproduce Steps to reproduce the behavior:

  1. Use Compass to change a userId for a document to a String
  2. Try to update the document with useUpdate - you get permission error
  3. Change the userId to an ObjectId
  4. Try to update the document with useUpdate - it works!
  5. Now try filtering with useMulti, by userId
  6. It doesn't work

Expected behavior

Screenshots (if applicable) Screenshot 2022-06-17 at 09 32 11

GraemeFulton commented 2 years ago

Also, when you create a new document, it looks like the document is inserted with the userId as an objectID always. So I think only fixing the filter part of this issue would do the trick?

eric-burel commented 2 years ago

Probably, the problem is serialization of the variables, your ObjectId doesn't survive the network connection basically But server-side you can't use a string either because Mongo will never find a matching userId, as it stores it as ObjectID So I have to think about it more carefully, we will have a similar issue with any relation id

Maybe adding a graphql scalar that does the translation, in which case we would need a new type "MongoId" for fields, it's annoying though

GraemeFulton commented 2 years ago

That explains it, if it is common practice to use ObjectID everywhere, even for relational IDs, I don't know how people are filtering by document owner with apollo - I tried apollo's useQuery, but it can't filter by objectId either đŸ˜¤

GraemeFulton commented 2 years ago

I tried doing this too, adding a virtual field that I could query by string, but found it's not possible to query it

uidString:{
    type: String,
    optional:true,
    canRead:['anyone'],
    resolveAs:{
      resolver:async(doc, args, context)=>{
        const usr = await context.VulcanUser.connector.findOne(
          { _id:  ObjectId(doc.userId) }
          );
          return usr._id;
      }
    }
  },
eric-burel commented 2 years ago

Yeah in graphql you have this concept of Scalar if you want to pass stuffs like Date, GraphQL should translate them automatically.

However it's complicated as you have to add this info in the Vulcan schema as well, so that the type of "userId" is not "string" but "objectId" in your graphql schema basically

I think only Mongo+GraphQL user will have the response to this

It's great to have the week-end coming because it will need some careful thinking ^^ but very relevant issue indeed

eric-burel commented 2 years ago

Update:

Your are then expected to do import { GraphqlObjectId } from '@vulcanjs/mongo-apollo'; const mySchema = { _id : { ..., typeName: GraphqlObjectId }} You just need to tell Vulcan that it's not a String, but a GraphqlObjectId. I haven't actually tested it yet though.

Filters probably don't work yet, it may take some time to figure everything right but I think it should work ok.

This is published in version 0.6.9-alpha.3 of @vulcanjs/mongo-apollo (latest version basically)

GraemeFulton commented 2 years ago

thanks Eric, for short term so I can have filtering, I did a workaround:

So only Ownership checks use the userId ObjectID

eric-burel commented 2 years ago

I've improved the support for custom scalar for ids, and exposed a GraphqlObjectId typeName, in the latest alpha release

This seems to work in my basic test, will dig and merge later. I need to double check the "owners" property but it should work ok.

What happens currently:

eric-burel commented 2 years ago

Still experimental but it should work better now, we can switch betwen ObjectId (default) and strings I've started some work to get rid of Mongoose and prefer mongo as well, see https://github.com/VulcanJS/vulcan-npm/issues/130