Skip to content

Conversation

@seuros
Copy link
Member

@seuros seuros commented Jul 1, 2025

Prevent silent failures when transaction: true is used outside transactions

closes #74

Prevent silent failures when transaction: true is used outside transactions

Co-authored-by: Vladimir Lyzo <donbobka@gmail.com>
@seuros seuros marked this pull request as ready for review July 1, 2025 21:01
@seuros seuros requested a review from Copilot July 1, 2025 21:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a guard to prevent silent failures when transaction: true is used outside of an active database transaction by raising an error, along with new tests to verify this behavior for both PostgreSQL and MySQL.

  • Add validation in the advisory lock core to enforce active transactions
  • Add tests for both PostgreSQL and MySQL to assert the error is raised when no transaction is open

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/with_advisory_lock/core_advisory.rb Inserts a runtime check to raise ArgumentError if transaction: true is used without an active transaction
test/with_advisory_lock/transaction_test.rb Adds two new tests (PostgreSQL and MySQL) to assert the error is raised outside of transactions; refactors a test block’s exception message
Comments suppressed due to low confidence (1)

lib/with_advisory_lock/core_advisory.rb:21

  • The call to transaction_open? may not be defined in this context. Consider using a method on the connection (e.g., ActiveRecord::Base.connection.open_transactions > 0) or explicitly delegating the call to ensure it’s available.
      if options.fetch(:transaction, false) && !transaction_open?

exception = assert_raises(ArgumentError) do
MysqlTag.with_advisory_lock 'test', transaction: true do
raise 'should not get here'
raise 'Behold! Thou hath trespassed into the sacred MySQL transaction realm!'
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The whimsical exception message in this test could be confusing for future maintainers. It may be clearer to simplify it to something like raise 'should not reach this code' for consistency with the PostgreSQL test.

Suggested change
raise 'Behold! Thou hath trespassed into the sacred MySQL transaction realm!'
raise 'should not reach this code'

Copilot uses AI. Check for mistakes.
@seuros seuros merged commit e4bc6c1 into master Jul 1, 2025
6 checks passed
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