Fix insert_all/upsert_all for single character, temporary and three part table names - #1403
Closed
Alt70155 wants to merge 1 commit into
Closed
Conversation
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.
Contributor
|
@Alt70155 Could you create a PR against the |
Author
|
Thanks for taking a look. Opened #1405 against |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1402.
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(andinsert_allwhen it takes theMERGEpath) raisesNoMethodErrorwhen theMERGEtable name pattern cannot match the target, andreturns 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,UPDATEandFROMalready handle three part names; onlyMERGEwaslimited 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:
Notes on the choices:
\[[^\]]+\], so spaces inside brackets are kept(
[WITH - SPACES]) while a space is no longer allowed in a bare name. That iswhat stops an aliased target being captured as part of the table name.
\s*\., not\., soMERGE INTO dbo .productskeepsworking. 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 existingMERGE statementsblock (5 -> 13): bracket quoted and bare names, a singlecharacter name, a name with spaces inside brackets, two and three part names,
with and without a
WITH (UPDLOCK, HOLDLOCK)hint, and the space before thedot case.
test/cases/adapter_test_sqlserver.rb— one more assertion in thequery_requires_identity_insert?test, using the generatedMERGEwith thetable hint removed so the aliased target is exercised.
Without the change these fail with 3 failures and 2 errors; with it the file is
green.
bundle exec standardrbis clean for the whole repository.Branches
The same pattern is present on
main,8-1-stableand8-0-stable. This PRtargets
main. Happy to open backport PRs for the stable branches if you wouldlike them.