Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Remove unwanted transaction when has one association is built #8154

Merged
merged 1 commit into from

4 participants

@bogdan

Example:

> Developer.new.build_profile
   (0.3ms)  BEGIN
   (0.1ms)  COMMIT
=> #<DeveloperProfile id: nil, developer_id: nil, current_company: nil, current_title: nil, ...>
> 
@steveklabnik
Collaborator

Seems legit. Is there any way we can test this?

@carlosantoniodasilva carlosantoniodasilva commented on the diff
...lib/active_record/associations/has_one_association.rb
@@ -28,7 +28,7 @@ def replace(record, save = true)
# If target and record are nil, or target is equal to record,
# we don't need to have transaction.
if (target || record) && target != record
- reflection.klass.transaction do
+ transaction_if(save) do

Isn't that triggering an extra save call here, that might be unwanted? Not sure it will affect anything, haven't looked further.

@bogdan
bogdan added a note

Ok, now I looked the entire method, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

Looks fine, not sure there's an way to test it though. :+1:

@steveklabnik
Collaborator

Yeah, I don't think so either, I was just wondering out loud.

@bogdan

Added test case.

@carlosantoniodasilva

I think it'd be nice to have a changelog entry for that, with the example code, wdyt? Also, please squash your commits. Thanks.

@bogdan

@carlosantoniodasilva can you advise how to update PR with squashed commits?

Previously I was recreating a branch, but after last github update that no longer work: PR is getting closed with no chance to update it even after reopen.

@carlosantoniodasilva

@bogdan I usually squash the commits locally in my branch, and push force to the remote branch. Github shows the change in the pull request properly. Also, @steveklabnik has written about squashing and updating pull requests recently :)

@bogdan

@carlosantoniodasilva thanks, all I need to know is that push -f black magic.

@carlosantoniodasilva

Np :), squash is ok, but I can't see the changelog entry, have you added it? Thanks.

@bogdan

done

@carlosantoniodasilva

Thanks, but now github says it cannot be merged.. ow this changelog hell, can you please rebase from current master?

Also, perhaps the changelog message could read:

Do not create useless database transaction when building `has_one` association.

(I mean, the ActiveRecord bit is implicit in the changelog). Thanks, and sorry to bother so much :)

@bogdan

Changed Changelog entry as you want and rebase agains master.

Please, invent a solution for this changelog merge problem somewhere in your private chats if you dont like mine:
#7210

Now I am sure that i've spent days summarily to resolve this issue across different PRs.
This demotivates contribution a lot when a patch itself takes less time than administration issues.

@carlosantoniodasilva carlosantoniodasilva merged commit e803f3a into rails:master
@carlosantoniodasilva

@bogdan merged, thanks for that.

I understand that the current changelog is a problem, and we have to keep asking people to rebase because of it, as you had to. That's an unfortunate problem, but we do need changelog entries. Perhaps one solution is to not ask for changelogs anymore, as @rafaelfranca commented in there, I don't know.

Contribution is important, and I agree with you that these minor things makes it harder and annoying, but until we don't have another solution, we'll have to keep doing it.

I'll try to bring this discussion again to see if we can come up with a solution. Thanks!

@nikitug nikitug referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 10, 2012
  1. @bogdan
This page is out of date. Refresh to see the latest.
View
9 activerecord/CHANGELOG.md
@@ -1,5 +1,14 @@
## Rails 4.0.0 (unreleased) ##
+* Do not create useless database transaction when building `has_one` association.
+
+ Example:
+
+ User.has_one :profile
+ User.new.build_profile
+
+ *Bogdan Gusiev*
+
* :counter_cache option for `has_many` associations to support custom named counter caches.
Fix #7993
View
10 activerecord/lib/active_record/associations/has_one_association.rb
@@ -28,7 +28,7 @@ def replace(record, save = true)
# If target and record are nil, or target is equal to record,
# we don't need to have transaction.
if (target || record) && target != record
- reflection.klass.transaction do
+ transaction_if(save) do

Isn't that triggering an extra save call here, that might be unwanted? Not sure it will affect anything, haven't looked further.

@bogdan
bogdan added a note

Ok, now I looked the entire method, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
remove_target!(options[:dependent]) if target && !target.destroyed?
if record
@@ -90,6 +90,14 @@ def remove_target!(method)
def nullify_owner_attributes(record)
record[reflection.foreign_key] = nil
end
+
+ def transaction_if(value)
+ if value
+ reflection.klass.transaction { yield }
+ else
+ yield
+ end
+ end
end
end
end
View
6 activerecord/test/cases/associations/has_one_associations_test.rb
@@ -206,6 +206,12 @@ def test_successful_build_association
assert_equal account, firm.account
end
+ def test_build_association_dont_create_transaction
+ assert_no_queries {
+ Firm.new.build_account
+ }
+ end
+
def test_build_and_create_should_not_happen_within_scope
pirate = pirates(:blackbeard)
scoped_count = pirate.association(:foo_bulb).scope.where_values.count
Something went wrong with that request. Please try again.