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

Arel::UpdateManager with join statement on a sqlite and postgres database are invalid #44401

Open
gagalago opened this issue Feb 11, 2022 · 2 comments

Comments

@gagalago
Copy link

gagalago commented Feb 11, 2022

Steps to reproduce

complex update with joins produce invalid sql syntax with sqlite and postgres.
the following test it "generates an update statement with joins" is checking an sql query that is invalid in sqlite and postgres.

this is how postgres update syntax can be and how sqlite update syntax
is defined. To join table during an update we should use the FROM.
it seems that it's a syntax only valid in mysql.

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem 'rails', github: 'rails/rails', branch: 'main'
  gem 'sqlite3'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.integer :posts_count
  end

  create_table :posts, force: true do |t|
    t.integer :user_id
  end
end

class BugTest < ActiveSupport::TestCase
  def test_update_with_joins
    um = Arel::UpdateManager.new

    table = Arel::Table.new(:users)
    join_table = Arel::Table.new(:posts)
    join_source = Arel::Nodes::JoinSource.new(
      table,
      [table.create_join(join_table)]
    )

    um.table join_source
    um.set [[table[:posts_count], join_table[:id].count]]
    assert_nothing_raised { ActiveRecord::Base.connection.update(um) }
  end
end

Expected behavior

it should be possible to set the FROM to join other tables. and at the end generate the following sql UPDATE "users" SET "posts_count" = COUNT("posts"."id") FROM "posts" WHERE "users"."id" = "posts"."user_id".

for example with the following syntax:

table = Arel::Table.new(:users)
join_table = Arel::Table.new(:posts)

um.table table
um.from join_table
um.set [[table[:posts_count], join_table[:id].count]]
um.where table[:id].eq(join_table[:user_id])
ActiveRecord::Base.connection.update(um)

Actual behavior

for now, it generate the following sql UPDATE "users" INNER JOIN "posts" SET "posts_count" = COUNT("posts"."id") that is an invalid syntax

System configuration

Rails version: 7.0.1

Ruby version: 3.1

@nimmolo
Copy link

nimmolo commented Mar 8, 2022

Arel UpdateManager with a join generates invalid SQL syntax in MySQL also, I believe. @ignacio-chiazzo may have insight here, having worked on this part of the code recently.

table = Arel::Table.new(:users)
join_table = Arel::Table.new(:posts)
join_source = Arel::Nodes::JoinSource.new(
  table, [table.create_join(join_table)]
)
Arel::UpdateManager.new.
  table(join_source).
  where(table[:project_id].eq(join_table[:project_id])).
  set([[table[:column_name], join_table[:column_name]]])

The generated SQL from this does not specify the table where the column_name is to be set.

UPDATE `users` 
INNER JOIN `posts` 
SET `column_name` = `posts`.`column_name` 
WHERE (`users`.`project_id` = `posts`.`project_id`) 

In the Arel source, it's generating a Nodes::UnqualifiedColumn.new(column).

The error in the SQL is

Mysql2::Error: Column 'column_name' in field list is ambiguous: 

It seems like it would solve the issue if Arel qualified column_name with table.column_name. I don't know how to test this, but i'd be happy to.

Rails version: 5.2.6
Ruby version: 2.6

@jonsgreen
Copy link

I also bumped into this trying to construct an update with a self-join and found that I could not create a valid sql update using UpdateManager and resorted to writing the SQL by hand. I am curious why @lazaronixon closed his PR and if there are any thoughts on where this issue goes from here.

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

Successfully merging a pull request may close this issue.

4 participants