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

Added transaction verbosity for activerecord #65

Merged
merged 1 commit into from Oct 12, 2014

Conversation

Strech
Copy link
Contributor

@Strech Strech commented Oct 11, 2014

Make spec from c1f32f6 fail

As you can see here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L75

AR by default swallows exception. But in rails app, this attribute set to true.
After you merge this request, you will find #64 very helpful

@joevandyk
Copy link
Contributor

I believe this option is for the upcoming Rails 4.2 release.

@Strech
Copy link
Contributor Author

Strech commented Oct 11, 2014

@joevandyk yap, this is for rails 4.2 ... we already use it. And #64 is the real pain for us ...

@Strech
Copy link
Contributor Author

Strech commented Oct 11, 2014

@joevandyk I could add new Gemfile for 4.2 and set this options only if Rails version >= 4.2

@Strech
Copy link
Contributor Author

Strech commented Oct 12, 2014

@chanks

In Rails.4.1.6 it swallows exceptions by default

def rollback_records
        @state.set_state(:rolledback)
        records.uniq.each do |record|
          begin
            record.rolledback!(parent.closed?)
          rescue => e
            record.logger.error(e) if record.respond_to?(:logger) && record.logger
          end
        end
      end

@Strech
Copy link
Contributor Author

Strech commented Oct 12, 2014

@chanks I just try to say – you passed spec is not true. Because exception is swallowed by rescue block.

I added ActiveRecord version check in spec. Please 👀

@Strech Strech force-pushed the activerecod-transaction-verbosity branch from 5b27f11 to 7cd24ce Compare October 12, 2014 06:54
@joevandyk
Copy link
Contributor

I can confirm that when using Rails master and adding this to the spec:

ActiveRecord::Base.raise_in_transactional_callbacks = true

results in this error:

Failures:

  1) Que using the ActiveRecord adapter should be able to survive an ActiveRecord::Rollback without raising an error
     Failure/Error: ActiveRecord::Base.transaction do
     NoMethodError:
       undefined method `rolledback!' for #<Que::Adapters::ActiveRecord::CommittedCallback:0x007fc3753e23b8>
     # ./spec/adapters/active_record_spec.rb:106:in `block (2 levels) in <top (required)>'

All the changes I made:

@Strech
Copy link
Contributor Author

Strech commented Oct 12, 2014

@joevandyk The main problem is that AR swallow real exception and the spec is passed

@joevandyk
Copy link
Contributor

@Strech Yes, that makes sense now. Thanks for your time on this!

@chanks chanks merged commit 7cd24ce into que-rb:master Oct 12, 2014
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.

None yet

3 participants