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

Better handle paranoid models #405

Open
DaddyWarbucks opened this issue Nov 16, 2022 · 2 comments
Open

Better handle paranoid models #405

DaddyWarbucks opened this issue Nov 16, 2022 · 2 comments

Comments

@DaddyWarbucks
Copy link
Contributor

DaddyWarbucks commented Nov 16, 2022

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:

const result = await this._getOrFind(id, findParams);

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
Copy link

Servaker commented Dec 1, 2022

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
Copy link
Contributor Author

@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.

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