Don't send BEGIN to the database until the first statement in a transaction is made #2337

Closed
ghost opened this Issue Jul 29, 2011 · 8 comments

Comments

Projects
None yet
5 participants
@ghost

ghost commented Jul 29, 2011

After upgrading to Rails 3.1.0.rc5 from 3.1.0.rc4, the following test ceased to pass:

  context "when updating" do
    subject { Factory(:project) }

    describe "with the maximum number of users already added" do
      before do
        App.maximum_users_per_project.times { subject.users << Factory(:user) }
      end

      it "should not allow users to be added by saving a user model" do
        lambda { Factory(:user, projects: [subject]) }.should_not change(subject.users, :count)
        subject.errors[:base].should_not be_empty
      end
    end
  end

These are the classes involved:

class Project < ActiveRecord::Base
  has_and_belongs_to_many :users, before_add: :check_maximum_users
private
  def check_maximum_users(user)
    if users.count >= App.maximum_users_per_project
      errors.add :base, :exceeded_quota
      raise ActiveRecord::Rollback
    end
  end
end

class User < ActiveRecord::Base
  has_and_belongs_to_many :projects, before_add: :check_maximum_users
private
  def check_maximum_users(project)
    project.send :check_maximum_users, self
  end
end

The test stops with a propagated ActiveRecord::Rollback exception. It passes on Rails 3.1 RC4.

Now, I understand that, since this was called outside the scope of a transaction (the before_add is called when I'm actually building the object in memory, adding projects to its collection), then the exception was propagated. Maybe, is actually the intended behaviour and I can see the reasoning.

I'm a bit uncertain only to:

  1. Why does it pass on RC4? is it really the intended behaviour?
  2. If it's intended behaviour, what would be the recommended alternative to implement the same logic without having to catch an Exception, if possible.

Sorry if posted in the wrong place, is the first time I open an issue in Rails. Keep on the amazing work.

@ghost

ghost commented Jul 29, 2011

Apparently, this was the commit that changed the behaviour: fa8dfad

Member

jonleighton commented Jul 29, 2011

Hmm. This makes a good point, because you could actually be doing database work in your callbacks, which then ought to be wrapped in transactions.

My preferred solution would be to revert the above commit, and modify how transactions work so that they don't actually sent the 'BEGIN' to the database until the first actual statement is made (if at all). Does anyone see a problem with that approach?

Feels a bit risky for 3.1 though, so I am not sure. Maybe we should simply revert the above for 3.1 and do my suggested optimisation for 3.2.

@tenderlove, @josevalim, any thoughts?

jonleighton was assigned Jul 29, 2011

@ghost

ghost commented Jul 29, 2011

Delaying the "begin" call until the first statement would definitely be the best choice, but very complex to implement, I suppose...

+1 to revert on 3.1 then

Owner

tenderlove commented Jul 29, 2011

@jonleighton I agree. Please revert this on 3-1-stable, but we should keep this ticket open until it's fixed on master.

@jonleighton jonleighton added a commit that referenced this issue Jul 30, 2011

@jonleighton jonleighton Revert "Replace inline lambdas with named methods" and "Don't wrap op…
…erations on collection associations in transactions when they are not needed, so the connection adapter does not send empty BEGIN COMMIT transactions blocks to the database."

This reverts commits df63c99 and b17fd25.

The change had unintended side effects, please see #2337.

Conflicts:

	activerecord/test/cases/associations/has_many_associations_test.rb
048215a
Contributor

isaacsanders commented May 1, 2012

Is this still an issue, @tenderlove @jonleighton?

Since the related commits were reverted and we didn't get news on that, I'm closing this. Please let us know if you still have any issue with that, and we'll gladly reopen. Thanks!

@tenderlove ops, I just saw your comment. Do you still have any concern about this one?

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