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

One-to-One Association keeps old records #205

Open
mereghost opened this issue Jun 2, 2017 · 7 comments
Open

One-to-One Association keeps old records #205

mereghost opened this issue Jun 2, 2017 · 7 comments
Labels

Comments

@mereghost
Copy link

Heya folks,

I was working on hanami/model support for one-to-one associations when I noticed that when I associate a new record the old one is kept in the database.

What I expected was that issuing a create command on a one-to-one association would remove the old record (if any) and insert a new one. While it does retrieve the correct one, it leaves the database a mess and might generate constraint violation errors.

@solnic solnic added the bug label Jun 4, 2017
@solnic
Copy link
Member

solnic commented Jun 4, 2017

Thanks for reporting this. We'll address this in the next version.

@mereghost
Copy link
Author

By next version you mean rom-4.0? =)

@solnic
Copy link
Member

solnic commented Jun 7, 2017

Nope, next bug fix release. @flash-gordon you think you could take care of this?

@flash-gordon
Copy link
Member

@solnic sorry for being stupid here, but how is it supposed to work? Remove existing references in a before hook?
@mereghost I'd say it should trigger a constraint violation error, even if we remove the existing record this operation is not going to be atomic, leaving room for possible duplications. FWIW I'd recommend creating one-to-one records along simultaneously, in a single transaction, and use updates later on.

@solnic
Copy link
Member

solnic commented Jun 7, 2017

@flash-gordon I thought about wrapping it in a transaction as a special way of handling this type of association

@flash-gordon
Copy link
Member

yes, this should be run inside a transaction, so that possible errors won't pollute the database with partial writes, but this won't prevent from having duplicates anyway, consider this

session 1 session 2
starts a command starts a command
deletes associated records -> 0 ...
... deleted associated records -> 0
inserts a new record ...
... inserts a new record
commit commit

Having a unique constraint will pause session 2 on the insert step until session 1 is completed (or rolled back). So we actually need a DB constraint. And yes, transaction is a must too :)
Another option is using pessimistic locking on the parent record, i.e. SELECT FOR UPDATE, this way both sessions will be synchronized before the delete phase starts. Unfortunately, not all databases support this mechanism, for instance, SQL does not.

@mereghost
Copy link
Author

@flash-gordon While I do agree that having a constraint at the database level is the right thing to do, I see no way to enforce it to the end users.

Guarding against partial writes and then warning users that duplicates may end in the database, due to race conditions, if no unique constraint is in place seems more "user friendly" to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants