Allow upsert mutation to accept terms & parameters in the search

[justinr1234]

search uses ${collectionName}Input type in the mutation params. It should just use plain JSON and respect selector and options by calling let { selector, options } = await collection.getParameters(terms, {}, context);. Therefore in the upsert default mutation, search would be equivalent to terms in the default list resolver.

[SachaG]

What search are we talking about exactly?

[justinr1234]

The first parameter of the upsert was the search which is just the key/value pairs to match on in collection.find(). I realized later that this is not keeping in line with the rest of Vulcan and should instead follow the same terms approach as a query.

[SachaG]

Oh I didn't notice that. I'm not sure what it should be actually, terms are specifically used for list resolvers.

Looking at your code again though, I think the lack of Utils.performCheck opens a security hole because we never check if the current user can update the current document. So you could potentially use this upsert mutation to modify an existing document and gain access to it, it seems? Am I missing something?

I guess I also didn't really stop to ask why we needed an upsert mutation in the first place. I personally never had to use one before, so unless there's a strong reason to add it maybe the extra complexity involved means it'd be better to leave it out of Vulcan core?

[justinr1234]

It follows security checks because it doesn’t do any actual work until it calls the existing mutations. All it does is see if a document exists with the given params. If it does, it calls the edit mutation. If it doesn’t exist, it calls a new mutation. Those mutations handle the security check.

Upsert is useful if you want to search and update something not using _id or slug and then insert it into the database otherwise. It is absolutely something that should be in core. Same argument as why Meteor and Mongo provide the operation.

[SachaG]

Oh gotcha, I got confused and thought you were calling newMutation and editMutation. We really need to stop calling those "mutations" and use the word "mutator" instead to distinguish them from the default mutations.

As for the search issue I'll defer to you as I don't really have a concrete use case to guide me. It does feel like it would be different from terms here though. On the other hand search doesn't feel appropriate, maybe we just want to call it selector then?

Unlike in the list scenario we're not fetching any data so there is no security risk in letting the client directly specify a selector (I think?).

[SachaG]

Btw maybe edit and single should also take a selector rather than being hardcoded to either an _id or slug? See also https://github.com/VulcanJS/Vulcan/issues/1859

[SachaG]

Oh actually Prisma just call the equivalent parameter where.

[justinr1234]

where is fine and I like the idea for the other single. I am only matching and then letting the other mutations do the heavy lifting including security checking. There is no chance of the user getting access to data they aren’t supposed to see.

I think terms makes sense here since they end up translating to selectors. Which selectors sre the same thing as a where

[SachaG]

The idea behind terms is that they get "translated" to a Mongo selector/options object by the successive parameters callbacks. In this case search will get used as is without translation (I think?), so I'm not sure if we should reuse the same name.

[justinr1234]

Your understanding is correct. I’d like to change upsert to behave with a terms search that gets translated. That works for keeping Vulcan DB agnostic as well down the road to add SQL support.

[SachaG]

DB-agnosticism is a good point, hadn't thought of that. So I guess we have 3 possible approaches:

• where: a special GraphQL type that specifies every possible search parameter.
• search (or I would prefer selector as a name): specifying the Mongo selector directly from the client.
• terms: a JSON object that gets translated into a selector.

[justinr1234]

Whatever we do, I’d like it to match query behavior. I think it makes the most sense from a mental model standpoint to keep the developer in 1 mode of thinking.

[justinr1234]

I guess the where would correspond to the schema fields? Would we create our own translation later for special operators like gt, lt etc?

I’d like to think of the best way to keep it DB agnostic. One of my use cases is eventually swapping out Mongo for SQL as a default.

[SachaG]

I think terms is probably best for now then. But would we run the same set of callbacks? Or have different ones? Presumably we'd need a different hook because we'd be trying to select a single document, not a range of documents?

[justinr1234]

I think maybe we add a new single runner that respects the same list behavior but for a single document.

[SachaG]

So collectionName.parametersSingle maybe?

[justinr1234]

Seems reasonabl

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[eric-burel]

Closed, 1.14 will bring a great new API. Upsert will have the same input argument as other mutations, to allow complex queries.