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

Identifying objects to delete #62

Open
breyed opened this issue May 29, 2022 · 4 comments
Open

Identifying objects to delete #62

breyed opened this issue May 29, 2022 · 4 comments

Comments

@breyed
Copy link

breyed commented May 29, 2022

The use of MutateNoteInput is surprising to me as the parameter type for deleteNote. It makes sense for updateNote, but for delete, I would have expected inputs patterned after what the Find and Get operations take, since a delete is essentially a Find/Get, except with the object removed from storage in addition to a copy of it being returned.

@wtrocki
Copy link
Member

wtrocki commented May 30, 2022

Very good point. I do not think we explain this properly.

Spec essentially see every operation that makes alteration of the data as "single object" operation. Reasons why this was designed this way was:

  • underlying storage limitations (MongoDB single object transactions)
  • challenging security model (Being able to alter or delete entire table from client is scary and needs number of new abstractions in the spec - like concept of ownership)
  • In typical production systems very rarely data is deleted and schema should be designed to incorporate that (it is mainly archived)

GraphQL CRUD is discouraging users to use it's model as only way of interacting with data.
It is just mapping common operations and limiting boilerplate. That is why spec considers all multi object operations as batch operation that need to be manually implemented by user (meaning that filter is not exposed/configured on the client). That approach fully matches GraphQL spec ethos (Do not force application developer to 100% rely on Anemic CRUD Model).

@wtrocki
Copy link
Member

wtrocki commented May 30, 2022

In short form - Deletions on multiple objects should be designed as separate mutation outside spec without including any user input

Real life production example that follows this pattern

archiveAllMessagesForMarketplacePost(advertId: "176bdb1856d95f3d990c337c80c1c516")

We could imagine if we pull that into spec we will end up with anti-pattern and really bad design where client does business logic:

deleteMessages(filter: {owner: "wojtekteer", marketplaceAdvertId: "176bdb1856d95f3d990c337c80c1c516})
## For each deleted
deleteNotifications(filter: {channelId: "..."})

@breyed
Copy link
Author

breyed commented May 30, 2022

For any mutation, whether insert, update, or delete, there are simple CRUD operations, and there are custom operations based on business logic. GraphQLCRUD addresses only the simple CRUD operations. Any special cases are out of scope. For some object types, there will be no CRUD delete operations, or even no delete operations at all, in which case the GraphQLCRUD convention for delete won't apply. But for other use cases, it will apply.

It is worth noting that change tracking is independent of GraphQLCRUD. For example, a back end my keep track of the states of the data store both before and after an object was inserted, updated, or deleted. This may be done with custom code, or through a generic algorithm such as SQL Server's temporal tables. The GraphQLCRUD API is the same regardless of whether a history of past states is maintained. Deletes are not a special case relative to inserts and updates, but rather just another kind of mutation.

The GraphQLCRUD API is also independent of the security model. An API that accepts inputs formatted such that any object could be inserted, updated, or deleted can still reject mutations that exceed a caller's permissions. This can be implemented in the GraphQL layer or, as is common for PostGraphile for example, in the database.

@wtrocki
Copy link
Member

wtrocki commented May 30, 2022

Any special cases are out of scope. For some object types, there will be no CRUD delete operations, or even no delete operations at all, in which case the GraphQLCRUD convention for delete won't apply. But for other use cases, it will apply.

Exactly. For real life example see: https://graphback.dev/docs/crud/overview#per-type

The GraphQLCRUD API is also independent of the security model. An API that accepts inputs formatted such that any object could be inserted, updated, or deleted can still reject mutations that exceed a caller's permissions. This can be implemented in the GraphQL layer or, as is common for PostGraphile for example

I'm worried that this is quite hard to done right because it would need to evaluate specific user filters build on client or even map database permissions to client user permissions.

If we check graphback docs that provides authorization policies: https://graphback.dev/docs/authentication/keycloak#field-updates-authorization
and https://graphback.dev/docs/authentication/keycloak#dynamic-filtering

Those are specifically subject to get out of sync with the schema evolution and are hard to maintain.

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

No branches or pull requests

2 participants