search

feathersjs-ecosystem / feathers-sequelize

A Feathers service adapter for the Sequelize ORM. Supporting MySQL, MariaDB, Postgres, SQLite, and SQL Server
MIT License
208 stars 75 forks source link

Better handle paranoid models #405

Closed DaddyWarbucks closed 2 months ago

DaddyWarbucks commented 1 year ago

We should be cautious of any methods that do get/find AFTER modification. This can cause issues when using paranoid models. Sequelize's paranoid option automatically adds WHERE deleatedAt IS NULL to queries. This means if the user patch/updates deletedAt: new Date(), the the adapter's uses of _getOrFind after the Model.update will fail.

For example service.patch(1, { deletedAt: new Date() }) will update the record, but will then throw NotFound due to this line: https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/46af5bddcec51a3c7f52116e2e41a0bdd51def79/src/adapter.ts#L346

I am not sure if this library can accomplish this. The developer should not be using patch/update to remove. Note error will still happen if using the underscore or dollar methods too. We could probably inspect the Model to determine the paranoid setting and deleted key, but thats super complicated.

I don't know if this should be fixed or documented, but I wanted to capture it here.

Servaker commented 1 year ago

There are situations when the master data is maintained in a third-party system and transferred automatically to the Featners App. The same record from the master system can come in the beginning with the status deleted and then be restored. If Sequelize is used with the paranoid option enabled, then the only way to restore the record, i.e. set deleted_at=null field, this is to execute the service.patch(1, { deleted_at: null }) method with paranoid=false parameter set in the request. As a result the correct line with all fields and deletedAt: null is produced. I don't see the error NoteFound you are reporting.

If the service.patch(1, { deletedAt: null }) method is executed without the paranoid=false parameter set, then the record will not be updated, the timestamp in the deleted_at field will be preserved, and a NotFound message will be returned in response.

Executing the service.update(1, { deleted_at: null }) method, even with the paranoid=false parameter, does not remove the deletion timestamp and does not restore the record.

feathers-sequelize 6.3.6.

DaddyWarbucks commented 1 year ago

@Servaker This only happens when "setting" deletedAt to a date, not when "unsetting" it with null. Anything that does { deletedAt: null } works as expected. But when trying to update/patch with { deletedAt: new Date() } it fails as described. The developer really shouldn't be using patch/update to set { deletedAt: new Date() } and should just use the remove method. But it's still a potential error.

DaddyWarbucks commented 2 months ago

This should be fixed with V8. We no longer fetch data after mutative methods. So the data/query should no longer clash.