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

Fixed model attribute synchronization for update method #2062

Merged
merged 2 commits into from Mar 22, 2020

Conversation

naz
Copy link
Contributor

@naz naz commented Mar 6, 2020

Introduction

When model's attributes are modified during even hooks, e.g. on('save'), changes are not detected and incorrect attributes are persisted in the db.

Motivation

As described in the #2007.

When making changes to the saved object during even hooks they should be persisted.

Proposed solution

The keep .attributes available for modification and avoid loosing reference to that object during triggerThen stage we could refetch attributesToSave through a method instead of keeping an object around.

Alternatives considered

Alternatively we could be explicit about not allowing to modify attributes directly by the bookshelf clients. This would mean forcing use of set (sometimes with unset flag) method on the model to do any modifications. This approach is rather inflexible and would require clients to modify code.

closes bookshelf#2007

- The attributes object can be modified within `triggerThen` through hooks. Sample scenario described here bookshelf#2007 (comment).
- Recalculating 'attributesToSave' object in after-triggerThen step solves the syncronization issue because it is still based on model's `attributes` property (the reference is not lost)
Copy link
Member

@ricardograca ricardograca left a comment

Can you add a test for the use case this is trying to fix? That should prevent the issue from being reintroduced in the future.

@naz naz requested a review from ricardograca Mar 13, 2020
@naz
Copy link
Contributor Author

@naz naz commented Mar 13, 2020

Hey @ricardograca 👋 let me know if the test is illustrative enough and if anything else is needed from my side fro this change to land in master.

Copy link
Member

@ricardograca ricardograca left a comment

Seems to do what it's supposed to. Thanks!

@ricardograca ricardograca linked an issue Mar 22, 2020 that may be closed by this pull request
@ricardograca ricardograca merged commit 9942912 into bookshelf:master Mar 22, 2020
2 checks passed
@naz
Copy link
Contributor Author

@naz naz commented Mar 23, 2020

Thanks @ricardograca I'll get the commit cherry-picked into https://github.com/bookshelf/bookshelf/tree/0.15 in coming days. Thank you very much for all the help!

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

Successfully merging this pull request may close these issues.

2 participants