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.

Sign up for GitHub

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

Jump to bottom

Fix upsert warning for MySQL #51274

Merged
merged 1 commit into from Mar 7, 2024
Merged

Fix upsert warning for MySQL #51274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 31 additions & 12 deletions activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
View file
Open in desktop
Expand Up @@ -639,18 +639,37 @@ def default_index_type?(index) # :nodoc:
end

def build_insert_sql(insert) # :nodoc:
sql = +"INSERT #{insert.into} #{insert.values_list}"

if insert.skip_duplicates?
no_op_column = quote_column_name(insert.keys.first)
sql << " ON DUPLICATE KEY UPDATE #{no_op_column}=#{no_op_column}"
elsif insert.update_duplicates?
sql << " ON DUPLICATE KEY UPDATE "
if insert.raw_update_sql?
sql << insert.raw_update_sql
else
sql << insert.touch_model_timestamps_unless { |column| "#{column}<=>VALUES(#{column})" }
sql << insert.updatable_columns.map { |column| "#{column}=VALUES(#{column})" }.join(",")
no_op_column = quote_column_name(insert.keys.first)

# Avoid MySQL 8.0 deprecation warning, see https://dev.mysql.com/worklog/task/?id=13325.
if !mariadb? && database_version >= "8.0.0"
values_alias = quote_table_name("#{insert.model.table_name}_values")
sql = +"INSERT #{insert.into} #{insert.values_list} AS #{values_alias}"

if insert.skip_duplicates?
sql << " ON DUPLICATE KEY UPDATE #{no_op_column}=#{values_alias}.#{no_op_column}"
elsif insert.update_duplicates?
sql << " ON DUPLICATE KEY UPDATE "
if insert.raw_update_sql?
sql << insert.raw_update_sql
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we have been ignoring the MySQL warning

Currently, we have a couple of places that provide a raw_update_sql pointing to the VALUES(...), which, with this change, created a mixed query:

INSERT table (...) VALUES (...) as table_alias ON DUPLICATE KEY UPDATE x=VALUES(x)

The query didn't work as expected, and the expected behavior broke. We may need some way to keep using the previous behavior for a while. (I'm working on a PR unless someone beat me)

cc: @matthewd

Copy link
Member

Choose a reason for hiding this comment

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

Yes. If the raw_update_sql is provided, we should not try to alias, because we don't even know the alias.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created this PR to address this issue.

else
sql << insert.touch_model_timestamps_unless { |column| "#{insert.model.quoted_table_name}.#{column}<=>#{values_alias}.#{column}" }
sql << insert.updatable_columns.map { |column| "#{column}=#{values_alias}.#{column}" }.join(",")
end
end
else
sql = +"INSERT #{insert.into} #{insert.values_list}"

if insert.skip_duplicates?
sql << " ON DUPLICATE KEY UPDATE #{no_op_column}=#{no_op_column}"
elsif insert.update_duplicates?
sql << " ON DUPLICATE KEY UPDATE "
if insert.raw_update_sql?
sql << insert.raw_update_sql
else
sql << insert.touch_model_timestamps_unless { |column| "#{column}<=>VALUES(#{column})" }
sql << insert.updatable_columns.map { |column| "#{column}=VALUES(#{column})" }.join(",")
end
end
end

Expand Down
17 changes: 17 additions & 0 deletions activerecord/test/cases/insert_all_test.rb
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ class InsertAllTest < ActiveRecord::TestCase

def setup
Arel::Table.engine = nil # should not rely on the global Arel::Table.engine
@original_db_warnings_action = :ignore
end

def teardown
Expand Down Expand Up @@ -336,6 +337,22 @@ def test_upsert_logs_message_including_model_name
end
end

def test_upsert_and_db_warnings
skip unless supports_insert_on_duplicate_update?

begin
with_db_warnings_action(:raise) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Not worth doing here, more for a followup, but I wonder if it would make sense to run CI/tests with DB warnings treated as errors by default. I think it might have caught this issue way sooner.

Also we probably should backport that fix.

assert_nothing_raised do
Book.upsert({ id: 1001, name: "Remote", author_id: 1 })
end
end
ensure
# We need to explicitly remove the record, because `with_db_warnings_action`
# prevents the wrapping transaction to be rolled back.
Book.delete(1001)
end
end

def test_upsert_all_logs_message_including_model_name
skip unless supports_insert_on_duplicate_update?

Expand Down