Bug introduced by PR #1206 (allow spaces in table names)

[zurchpet]

Issue

PR #1206 introduced support for spaces in table names.

Expected behavior

Table names with spaces should be correctly escaped and interpreted.

For example:

• INSERT INTO table name with spaces ... should assume the table name is table and stop parsing after the first space.
• INSERT INTO [table name with spaces] ... should be the only syntax to treat spaces as part of the table name.

Actual behavior

The method ActiveRecord::ConnectionAdapters::SQLServer::SchemaStatements#get_raw_table_name does not correctly identify table names in INSERT statements of the format INSERT INTO tablename SELECT * FROM other_table.

Currently, it returns the entire string tablename SELECT * FROM other_table as the table name, rather than stopping at tablename.

How to reproduce

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "tiny_tds"
  gem "activerecord", "7.2.1"
  gem "activerecord-sqlserver-adapter", "7.2.0"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(
  adapter:  "sqlserver",
  timeout:  5000,
  pool:     100,
  encoding: "utf8",
  database: "master",
  username: "SA",
  password: "StrongPassword!",
  host:     "127.0.0.1",
  port:     1433,
  )
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  drop_table :bug_tests rescue nil

  create_table :bug_tests, force: true do |t|
    t.bigint :external_id
  end
end

class BugTest < ActiveRecord::Base
end

class TestShouldBeWorking < Minitest::Test
  def setup
    BugTest.connection.execute("INSERT INTO bug_tests select('1')")
  end

  def test_should_be_working
    assert_equal 1, BugTest.count
  end
end

class TestWorking < Minitest::Test
  def setup
    BugTest.connection.execute("INSERT INTO bug_tests (external_id) VALUES ('1')")
  end

  def test_working
    assert_equal 1, BugTest.count
  end
end

Details

• Rails version: 7.2.1

• SQL Server adapter version: 7.2.0

• TinyTDS version: 2.1.7

• FreeTDS details:

  Compile-time settings (established with the "configure" script)
                            Version: freetds v1.4.23
             freetds.conf directory: /etc/freetds
     MS db-lib source compatibility: yes
        Sybase binary compatibility: no
                      Thread safety: yes
                      iconv library: yes
                        TDS version: auto
                              iODBC: no
                           unixodbc: yes
              SSPI "trusted" logins: no
                           Kerberos: yes
                            OpenSSL: yes
                             GnuTLS: no
                               MARS: yes
  

[aidanharan]

The adapter is not going to be able to parse the table name from all raw SQL statements. It is just too complicated as you have to take into account:

• Table names containing spaces
• Qualified table names ([database].[schema].[table])
• Square brackets optional
• Insert SQL that doesn't contain VALUES
• Probably a few more...

For the issue you encountered you already have the fix for the example you provided (which is to use "INSERT INTO bug_tests (external_id) VALUES ('1')").

Going to close this issue.

[zurchpet]

INSERT INTO tablename SELECT ('1') is valid SQL and can NOT be used it the current state of the adapter.

I'll try to come up with a pull request.

Also consider this use case.

INSERT INTO table_copy SELECT * FROM table

[zurchpet]

def get_raw_table_name(sql)
          return if sql.blank?

          s = sql.gsub(/^\s*EXEC sp_executesql N'/i, "")

          if s.match?(/^\s*INSERT INTO.*/i)
            s.split(/INSERT INTO/i)[1]
              .split(/OUTPUT INSERTED/i)[0]
              .split(/(DEFAULT)?\s+VALUES/i)[0]
              .split(/SELECT/i)[0]
              .match(/\s*([^(]*)/i)[0]
          elsif s.match?(/^\s*UPDATE\s+.*/i)
            s.match(/UPDATE\s+([^\(\s]+)\s*/i)[1]
          else
            s.match(/FROM\s+((\[[^\(\]]+\])|[^\(\s]+)\s*/i)[1]
          end.strip
        end

Just adding .split(/SELECT/i)[0] fixes the issue.

[eglitobias]

The fix seems to be quite simple. We also have this issue and would like it if we could fix it directly in the library.

[aidanharan]

@zurchpet Please open a PR with the fix and tests.

[zurchpet]

PR opened #1238