Skip to content

Conversation

@seuros
Copy link
Member

@seuros seuros commented Jul 21, 2025

Summary

This PR fixes the ArgumentError: missing keyword: :lock_name error that occurs when running Rails migrations with MySQL on Rails 7.2+.

Problem

Rails calls release_advisory_lock with a single positional argument during migrations:

connection.release_advisory_lock(lock_id)

However, the gem's MySQL implementation was expecting keyword arguments:

def release_advisory_lock(lock_keys, lock_name:, **)

This mismatch caused migrations to fail with the error reported in #126.

Solution

Updated MySQL's release_advisory_lock method to handle both signatures:

  • Rails' signature: release_advisory_lock(lock_id) - single positional argument
  • Gem's signature: release_advisory_lock(lock_keys, lock_name:, **) - with keyword arguments

The implementation now matches PostgreSQL's approach, which already handles both signatures correctly.

Testing

  • Added comprehensive tests for both signature variants
  • Verified that Rails migrations work correctly
  • All existing tests pass

Fixes #126

@luciiii6 @luk4s Could you please test this fix with your Rails 7.2 applications to confirm it resolves the migration issue?

…7.2+

Rails 7.2+ changed the signature of release_advisory_lock to require a
lock_name: keyword argument. This causes an ArgumentError when migrations
run: "missing keyword: :lock_name".

This commit updates MySQL's implementation to handle both signatures,
similar to how PostgreSQL already does it. The method now checks if it's
being called with ActiveRecord's signature (single positional arg, no kwargs)
or the gem's signature (lock_keys positional arg with keyword args).

Fixes #126
@seuros seuros requested a review from Copilot July 21, 2025 09:54
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

This PR fixes a signature compatibility issue with MySQL's release_advisory_lock method that causes Rails 7.2+ migrations to fail. The problem occurs because Rails calls the method with a single positional argument while the gem expects keyword arguments.

  • Updated MySQL's release_advisory_lock to handle both Rails' positional signature and the gem's keyword signature
  • Added comprehensive test coverage for both signature variants and error handling scenarios

Reviewed Changes

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

File Description
lib/with_advisory_lock/mysql_advisory.rb Modified release_advisory_lock method to accept both Rails' positional signature and gem's keyword signature
test/with_advisory_lock/mysql_release_lock_test.rb Added comprehensive test suite covering both signature variants and error handling
Comments suppressed due to low confidence (1)

test/with_advisory_lock/mysql_release_lock_test.rb:114

  • The error handling test doesn't actually test the error handling path described in the comment. It only verifies that certain error messages don't appear, but doesn't simulate actual connection errors. Consider using a mock or stub to actually test the error handling behavior, or remove this test if it cannot meaningfully test the intended scenario.
      refute_match(/Lost connection|MySQL server has gone away|Connection refused/i, e.message)


execute_successful?("GET_LOCK(#{quote(lock_keys.first)}, #{mysql_timeout})")
end

Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The method lacks documentation explaining the dual signature support. Consider adding a comment above the method explaining it handles both Rails' release_advisory_lock(lock_id) signature and the gem's release_advisory_lock(lock_keys, lock_name:, **) signature for Rails 7.2+ compatibility.

Suggested change
# This method supports two signatures for compatibility:
# 1. Rails' ActiveRecord signature: release_advisory_lock(lock_id)
# - Used by Rails migrations with a single positional argument.
# 2. Gem's custom signature: release_advisory_lock(lock_keys, lock_name:, **)
# - Allows releasing locks using lock keys and additional options.
# The method determines the signature based on the arguments passed.

Copilot uses AI. Check for mistakes.

# Acquire lock using SQL (ActiveRecord doesn't provide get_advisory_lock method)
lock_keys = model_class.connection.lock_keys_for(lock_name)
result = model_class.connection.select_value("SELECT GET_LOCK(#{model_class.connection.quote(lock_keys.first)}, 0)")
Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The direct SQL query construction is duplicated on lines 72, 81, and 85. Consider extracting this into a helper method like acquire_lock_sql(lock_key) and release_lock_sql(lock_key) to reduce duplication and improve maintainability.

Suggested change
result = model_class.connection.select_value("SELECT GET_LOCK(#{model_class.connection.quote(lock_keys.first)}, 0)")
result = model_class.connection.select_value(acquire_lock_sql(lock_keys.first))

Copilot uses AI. Check for mistakes.
@luk4s
Copy link

luk4s commented Jul 21, 2025

Tested this branch, and passed ✅

Confirm fixed. 👍

@seuros seuros merged commit 94253ca into master Jul 21, 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.

MySQL migrations issues after upgrade

3 participants