Skip to content

Fix insert_all/upsert_all for single character, temporary and three part table names - #1405

Open
Alt70155 wants to merge 1 commit into
rails-sqlserver:8-1-stablefrom
Alt70155:fix/merge-into-aliased-target-table-name-8-1
Open

Fix insert_all/upsert_all for single character, temporary and three part table names#1405
Alt70155 wants to merge 1 commit into
rails-sqlserver:8-1-stablefrom
Alt70155:fix/merge-into-aliased-target-table-name-8-1

Conversation

@Alt70155

@Alt70155 Alt70155 commented Sep 3, 2026

Copy link
Copy Markdown

Fixes #1402.
Replaces #1403, which targeted main.

I used AI assistance to narrow down the cause and to check the patch for
regressions, but everything here was verified by running the reproduction script
and the adapter's test suite against a local SQL Server 2019.

upsert_all (and insert_all when it takes the MERGE path) raises
NoMethodError when the MERGE table name pattern cannot match the target, and
returns a wrong name when the target carries an alias. See #1402 for the full
analysis of why the two identifier slots behave that way.

INSERT, UPDATE and FROM already handle three part names; only MERGE was
limited to two.

Fix

Replace the pattern with an explicit identifier that matches either a bracket
quoted or a bare name, joined by up to two dots:

MERGE_TARGET_IDENTIFIER = /(?:\[[^\]]+\]|[a-z0-9_-]+)/i # :nodoc:
MERGE_TARGET_TABLE_NAME = /\A\s*MERGE\s+INTO\s+(#{MERGE_TARGET_IDENTIFIER}(?:\s*\.#{MERGE_TARGET_IDENTIFIER}){0,2})\s+(?:AS|WITH|USING)/i # :nodoc:

Notes on the choices:

  • A bracket quoted segment is \[[^\]]+\], so spaces inside brackets are kept
    ([WITH - SPACES]) while a space is no longer allowed in a bare name. That is
    what stops an aliased target being captured as part of the table name.
  • The dot separator is \s*\., not \., so MERGE INTO dbo .products keeps
    working. It parses today and I did not want to change that.
  • {0,2} allows one, two and three part names, matching the other branches.

Tests

  • test/cases/schema_test_sqlserver.rb — 8 cases added to the existing
    MERGE statements block (5 -> 13): bracket quoted and bare names, a single
    character name, a name with spaces inside brackets, two and three part names,
    with and without a WITH (UPDLOCK, HOLDLOCK) hint, and the space before the
    dot case.
  • test/cases/adapter_test_sqlserver.rb — one more assertion in the
    query_requires_identity_insert? test, using the generated MERGE with the
    table hint removed so the aliased target is exercised.

Run against Rails 8.1.3.1 and SQL Server 2019.

Without the change:

schema_test_sqlserver.rb     32 runs, 40 assertions, 3 failures, 2 errors
adapter_test_sqlserver.rb    82 runs, 176 assertions, 0 failures, 1 errors, 1 skip

With the change:

schema_test_sqlserver.rb     32 runs, 42 assertions, 0 failures, 0 errors
adapter_test_sqlserver.rb    82 runs, 184 assertions, 0 failures, 0 errors, 1 skip
insert_all_test_sqlserver.rb  1 runs,   1 assertions, 0 failures, 0 errors

bundle exec standardrb is clean for the whole repository.

Branches

Targeting 8-1-stable as requested in #1403.

The same pattern is present on main and 8-0-stable. The change applies
cleanly to both, and test/cases/schema_test_sqlserver.rb is identical across
the three branches. Happy to open follow up PRs for either once you are happy
with this one.

The MERGE branch of get_raw_table_name has two identifier slots and both
are `+`, so each needs to consume at least one character. `[products]`
matches because the engine can split it across the slots (`produc` +
`ts`), but `[x]` cannot be split and the match fails, so `nil[1]` raises
NoMethodError. The same happens for any name whose characters are not in
`[a-z0-9_ -]`, which covers temporary tables, non-ASCII names, names
containing `$`, and three part (cross database) names.

Because the character class also allows a space and the second slot is
reachable without a preceding dot, an aliased target is swallowed into
the captured name:

  # MERGE INTO [products] AS target USING ...
  get_raw_table_name(sql)  # => "[products] AS target"

That case is not reachable from build_sql_for_merge_insert, which always
emits `WITH (UPDLOCK, HOLDLOCK) AS target` and so terminates the match
early, but it is reachable from hand written MERGE passed to execute.

Replace the pattern with an explicit identifier that matches either a
bracket quoted or a bare name, joined by up to two dots, so one, two and
three part names are all handled. INSERT, UPDATE and FROM already handle
three part names; only MERGE was limited to two.

`MERGE INTO dbo .products` (a space before the dot) keeps working, since
the separator allows surrounding whitespace.

Fixes rails-sqlserver#1402.
@Alt70155

Alt70155 commented Sep 3, 2026

Copy link
Copy Markdown
Author

The BelongsToAssociationsTest#test_belongs_to_coerced failure on 3.4.7 looks unrelated to
this change. It is order dependent, and the three Ruby jobs each drew a different seed —
3.4.7 (--seed 45802) failed while 3.3.9 (23587) and 3.2.9 (58100) passed on the same code.

inheritance_test.rb creates two Firm subclasses with Class.new(Firm), and the
remove_const in its ensure block does not take them out of Firm.descendants — only GC
does. Confirmed locally:

initial                    Firm.descendants => ["Agency"]                                        type IN => 2 values
after Class.new(Firm) x2   Firm.descendants => ["Firm::FirmOnTheFly", "FirmOnTheFlyA", "Agency"] type IN => 4 values
after remove_const         Firm.descendants => unchanged                                         type IN => 4 values
after GC.start             Firm.descendants => ["Agency"]                                        type IN => 2 values

With four STI types the bind numbers shift, which matches the failing query exactly:
type IN (@0, @1, @2, @3) and FETCH NEXT @5 ROWS ONLY instead of the expected @3.

This PR only changes the MERGE branch of get_raw_table_name, which a SELECT never
reaches. Could you re-run the failed job when you get a chance?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants