Fix behaviour of hydrateRelationships with dangling references #6040
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/BWMt7wXG3f43Q4DJiPyKrwLAb7uM |
🦋 Changeset detectedLatest commit: fb29c33 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // If we're unable to find the user (e.g. we have a dangling reference), or access was denied | ||
| // then simply return null. | ||
| console.error(`Unable to fetch relationship data: relationship: ${JSON.stringify(relationship)}, id: ${id} `); | ||
| console.error(err); |
There was a problem hiding this comment.
I'm undecided as to whether these console error messages are helpful, or just noise 🤔
There was a problem hiding this comment.
Yeah, i don't think they're helpful though could you look at what it looks like to only do this on access control/does not exist? (the easiest way might be to do a many query and just get the first one from the array and handle if the array is empty?)
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| // If we're unable to find the user (e.g. we have a dangling reference), or access was denied | ||
| // then simply return null. | ||
| console.error(`Unable to fetch relationship data: relationship: ${JSON.stringify(relationship)}, id: ${id} `); | ||
| console.error(err); |
There was a problem hiding this comment.
Yeah, i don't think they're helpful though could you look at what it looks like to only do this on access control/does not exist? (the easiest way might be to do a many query and just get the first one from the array and handle if the array is empty?)
timleslie
force-pushed
the
fix-dangling-references
branch
from
c12eee8
to
9e54d9c
Compare
| const r = JSON.stringify(relationship); | ||
| console.error(`Unable to fetch relationship data: relationship: ${r}, id: ${id} `); | ||
| console.error(err); | ||
| return { id, data: null }; |
There was a problem hiding this comment.
| return { id, data: null }; | |
| return { id }; |
| @@ -41,30 +41,36 @@ export function addRelationshipData( | |||
| } | |||
| return []; | |||
|
(oh sorry, missed that the time that the review was requested was before we talked) |
All good, just working through things now. I'm gonna save the |
timleslie
force-pushed
the
fix-dangling-references
branch
from
df1e384
to
fb29c33
Compare
|
@mitchellhamilton OK, Ready for re-review. The only thing I haven't covered here is the |
If an item referenced in a document field is deleted from Keystone, the document field should handle this gracefully. The current behaviour will return an error when querying the field, which prevents the user from being able to manage the issue. This PR fixes this to simply return
nullfor the missing item. This also covers the behaviour for when a user doesn't have read access for an item.We also need to make a distinction between
data.data === null(object not found/access denied) anddata.data === {}(no selection fields in the config) in the document renderer.