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 table alias breaks insert, update, and delete statements #48775

Open
lavoiesl opened this issue Jul 20, 2023 · 3 comments · May be fixed by #48932
Open

Arel table alias breaks insert, update, and delete statements #48775

lavoiesl opened this issue Jul 20, 2023 · 3 comments · May be fixed by #48932

Comments

@lavoiesl
Copy link

lavoiesl commented Jul 20, 2023

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

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

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

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "~> 7.0.0"
  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 :posts, force: true do |t|
    t.string "name"
  end
end

class Post < ActiveRecord::Base
end


class BugTest < Minitest::Test
  def setup
    Post.arel_table.table_alias = nil
  end

  def test_select
    assert_equal('SELECT "posts".* FROM "posts"', Post.all.to_sql)
  end

  def test_select_alias
    Post.arel_table.table_alias = "p"
    assert_equal('SELECT "p".* FROM "posts" "p"', Post.all.to_sql)
  end

  def test_insert
    post = Post.create!
    refute_nil(post)
  end

  def test_insert_alias
    Post.arel_table.table_alias = "p"
    e = assert_raises(ActiveRecord::StatementInvalid) do
      Post.create!
    end
    assert_equal('INSERT INTO "posts" "p" DEFAULT VALUES', e.sql)
    assert_equal('SQLite3::SQLException: near ""p"": syntax error', e.message)
  end

  def test_update
    post = Post.create!
    refute_nil(post)
    post.name = "foo"
    post.save!
  end

  def test_update_alias
    post = Post.create!
    refute_nil(post)

    Post.arel_table.table_alias = "p"
    e = assert_raises(ActiveRecord::StatementInvalid) do
      post.name = "foo"
      post.save!
    end
    assert_equal('UPDATE "posts" "p" SET "name" = ? WHERE "p"."id" = ?', e.sql)
    assert_equal('SQLite3::SQLException: near ""p"": syntax error', e.message)
  end

  def test_delete
    post = Post.create!
    refute_nil(post)
    post.delete
  end

  def test_delete_alias
    post = Post.create!
    refute_nil(post)

    Post.arel_table.table_alias = "p"
    e = assert_raises(ActiveRecord::StatementInvalid) do
      post.delete
    end
    assert_equal('DELETE FROM "posts" "p" WHERE "p"."id" = ?', e.sql)
    assert_equal('SQLite3::SQLException: near ""p"": syntax error', e.message)
  end
end

Expected behavior

The table alias is valid on SELECTs, but not on INSERT, UPDATE, or DELETE statements.
The example is with sqlite, but MySQL has similar rules where it specifically asks for a table name.

I would expect Arel to NOT use the alias on insert, update, and delete statements

System configuration

Rails version:
6.1.7.4

Ruby version:
3.2.2

MySQL:
MySQL 5.7, using the mysql2 0.5.5 gem

@lavoiesl
Copy link
Author

lavoiesl commented Jul 20, 2023

I locate the issue as being that ToSql#visit_Arel_Nodes_InsertStatement calls the visitor on the relation:

def visit_Arel_Nodes_InsertStatement(o, collector)
collector << "INSERT INTO "
collector = visit o.relation, collector

which ends up calling:

if o.table_alias
collector << " " << quote_table_name(o.table_alias)
end

Perhaps visit_Arel_Nodes_{Insert,Update,Delete}Statement should call quote_table_name(o.relation.name) directly?

@lavoiesl
Copy link
Author

lavoiesl commented Aug 6, 2023

Yes, the tests are currently testing the failure. Would you prefer a version of the tests that properly fail?

@paulreece
Copy link
Contributor

Thanks for clarifying! Nope, I now have your tests failing with my fix :)

paulreece added a commit to paulreece/rails-paulreece that referenced this issue Aug 13, 2023
…tom set a table alias. It then accounts for the SQL needed for this in the AST. It addresses rails#48775.
paulreece added a commit to paulreece/rails-paulreece that referenced this issue Aug 25, 2023
…tom set a table alias. It then accounts for the SQL needed for this in the AST. It addresses rails#48775.
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