VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Default single resolver no longer supports slug #2118

Closed ErikDakoda closed 6 years ago

ErikDakoda commented 6 years ago

Regarding: #2112 single resolver documentId undefined -> now returns null

I would like to ask about this breaking change. In my code I rarely use documentId to query for a single document. Most of the time I use slug as documented here:

http://docs.vulcanjs.org/resolvers.html#Collection-Resolvers:

single: used to return a single document matching an _id or slug

Querying by slug using the single resolver is no longer supported. Is that really what was intended by this change? If so, why?

Additionally, the single resolver could be passed a selector to query for a single document by any field that was designated "selectable" and "unique" in the schema. This seems to be a legitimate use case when you have a secondary key in your collection. This is now also deprecated.

This will result in tons of refactoring in my code - after just completing tons of refactoring when we standardized graphql CRUD operations. Before I embark on that I just want to make sure that this was intended.

eric-burel commented 6 years ago

Hi, TL;DR: single resolver should not accept any param that could return more than one document. We can improve it by allowing complex key instead of just the documentId (a slug or a key composed of multiple selectable/unique fields).

Just to document the idea behind this change and provide more explanation:

Previous behaviour

Previously, the single resolver would:

What was wrong: fetching the first document available if no documentId is provided has some drawbacks. First it can feel surprising. Then it is redundant with withMulti + limit=1.

Eventually, it is explicitely forbidden to provide a selector defined client side to the multi resolver, as stated in the Terms and parameters doc. I understand this as the equivalent to SQL injection but for a NoSQL db, am I right? But, if you allow the single resolver to both return the first document it finds if no id is provided, and to take a selector as an option, it is just equivalent to allowing selector in multi: just write a loop and you can fetch what you want in the database.

New behaviour

Note that when I proposed #2112, I think the slug was already deprecated since late august, same for accepting a selector. I must admit that I don't know much about the history of this feature, as I only use single resolver with a documentId

Now, the single resolver will simply return null if you don't pass an id. That solves all those drawbacks of the previous behaviour, but it may indeed be too restrictive.

Possible improvements

So at any point we must guarantee that the client cannot send a query that could potentially select more than one document using the single resolver. I can't tell if it is a good idea or not, but at least that's consistent with current implementations.

We could start by allowing to pass a field that is tagged unique in the schema + its value (eg idField:"slug", id:"my-awesome-article" where slug is unique) instead of the documentId.

@ErikDakoda could you please provide some code samples of how you used to use the single resolver or the withSingle hoc? Also, when did your app break? Just after my PR or is it an older app you are updating?

In the mean time, the pattern to replace single+selector would be to use the multi resolver + a limit = 1 + a custom view. If your selector is not "strong" enough to guarantee that you are fetching only 1 document (or 0 if not found), that's the way to go anyway.

SachaG commented 6 years ago

Sorry I completely missed that, I thought that #2112 only changed the behavior of the single resolver when nothing was passed at all (no documentId, no _id, no slug, etc.). I didn't realized it broke fetching documents by slug, which is not great since people (well, me at least) currently rely on it.

I reverted some of the changes in the PR for now until we can find a better way of doing things to both adopt the new behavior described by @eric-burel while also preserving backwards compatibility.

ErikDakoda commented 6 years ago

@eric-burel You can accomplish what you want, I believe, by iterating through the own enumerable properties of the selector object, and making sure that each one has a non-empty value - otherwise return { result: null }.

If you are ambitious, you could also verify that each of the keys in selector are defined in SimpleSchema as "selectable" and "unique".

eric-burel commented 6 years ago

Yep definitely. Unicity actually can't really be checked statically, you would need a concept of multiple fields primary key instead. (you can have 2 fields that are not unique at all, but the combination of the two is unique, I don't think you can express this in SimpleSchema/MongoDB). You can just check that fields are selectable, and if there is only one field, that it is unique.

We can also throw an error if 2 documents are found, imo that's an error case of the single resolver.

The thing is that syntax needs thinking as those features are massively used and thus can be changed afterward. So in any case we will have to create a POC and validate it together, I was too eager to merge. I'll refactor the DataTable a bit I think (to make it easier to replace its style without duplicating logic, as for the forms), it will be the occasion to think about terms/resolvers/hocs API and propose ideas.