fix(postgres/mssql/saphana): include primary key in upsert when explicitly provided #11820

CHOIJEWON · 2025-12-04T09:45:19Z

Description of change

Fixes #11556

This PR addresses multiple related issues with upsert operations when using identity/auto-increment columns as conflict
paths across PostgreSQL, MSSQL, and SAP HANA.

🐛 Problems

1. PostgreSQL: Duplicate Rows on Primary Key Upsert

When using upsert() with a primary key as the conflict path in PostgreSQL, the method creates duplicate rows instead
of updating existing ones.

Reproduction:

// First upsert with id=1
await postRepository.upsert(
    { id: 1, title: "First" },
    { conflictPaths: ['id'] }
);

// Second upsert with same id=1
await postRepository.upsert(
    { id: 1, title: "Updated" },
    { conflictPaths: ['id'] }
);

// Expected: 1 row with updated title
// Actual: 2 rows created (conflict detection failed)

2. SAP HANA/MSSQL: Identity Columns as Conflict Paths Fail

MERGE INTO queries fail when using identity columns as conflict paths:

SAP HANA: cannot insert generated identity column field id
MSSQL: Cannot insert explicit value for identity column when IDENTITY_INSERT is OFF

3. MSSQL: IDENTITY_INSERT Not Applied to MERGE Queries

When explicitly providing values for identity columns in MERGE queries:

Error: DEFAULT not allowed for IDENTITY columns in MERGE
SET IDENTITY_INSERT was only applied to INSERT queries, not MERGE

💡 Root Causes

Issue 1: PostgreSQL Auto-Increment Exclusion

In getInsertedColumns(), auto-increment primary keys were filtered out for PostgreSQL, even when users explicitly
provided values. This caused the ON CONFLICT clause to fail.

Issue 2: MERGE INTO Source Missing Conflict Columns

In createMergeIntoSourceExpression(), identity columns were excluded from the USING clause, preventing the ON clause
from matching correctly.

Additionally, createMergeIntoInsertValuesExpression() tried to insert explicit values into identity columns, which is
prohibited in SAP HANA and MSSQL.

Issue 3: Missing IDENTITY_INSERT for MERGE

The createMergeExpression() method lacked the IDENTITY_INSERT wrapper that was already present in
createInsertExpression().

✅ Solutions

Fix 1: Include Primary Keys in PostgreSQL Upserts

Applied the same isOverridingAutoIncrementBehavior() logic used for MSSQL to PostgreSQL:

// src/query-builder/InsertQueryBuilder.ts:746-754
if (
    column.isGenerated &&
    column.generationStrategy === "increment" &&
    // ... other conditions
    !(
        (this.connection.driver.options.type === "mssql" ||
            DriverUtils.isPostgresFamily(this.connection.driver)) &&
        this.isOverridingAutoIncrementBehavior(column)
    )
)

Fix 2: Support Identity Columns in MERGE Conflict Paths

2a. Include conflict columns in USING clause:

// createMergeIntoSourceExpression()
// Add conflict path columns even if auto-generated
// (needed for ON clause matching)
if (this.expressionMap.onUpdate?.conflict) {
    const conflictColumns = // ... find conflict columns
        columns = [...columns, ...additionalColumns]
}

2b. Use DEFAULT for identity columns in INSERT values:

// createMergeIntoInsertValuesExpression()
if (
    (column.isGenerated &&
        column.generationStrategy !== "uuid" &&
        !this.isOverridingAutoIncrementBehavior(column))
) {
    expression += `DEFAULT`
}

Fix 3: Add IDENTITY_INSERT to MERGE Queries

Mirrors the existing IDENTITY_INSERT handling from createInsertExpression():

// createMergeExpression()
if (
    this.connection.driver.options.type === "mssql" &&
    this.expressionMap.mainAlias!.hasMetadata &&
    columns.some((column) =>
        this.isOverridingAutoIncrementBehavior(column),
    )
) {
    query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF`
}

🧪 Testing

New test case added:

it("should upsert with primary key as conflict path", () => {
    // Tests that upsert with id as conflict path updates
    // instead of creating duplicates
})

Test results:

# New test
npm run test:fast -- --grep "should upsert with primary key as conflict path"
✔ should upsert with primary key as conflict path

# All upsert tests
npm run test:fast -- --grep "upsert"
✔ 13 passing

# All repository basic methods tests
npm run test:fast -- --grep "repository > basic methods"
✔ 27 passing

🔍 Impact Analysis

Affected Drivers

✅ PostgreSQL/CockroachDB: Can now use primary keys as conflict paths in upserts
✅ MSSQL: MERGE queries with identity columns now work correctly
✅ SAP HANA: MERGE queries with identity columns now work correctly
⚪ No impact: MySQL, MariaDB, SQLite, Oracle (no behavioral changes)

Breaking Changes

None. These are bug fixes that enable previously broken functionality.

Pull-Request Checklist

Code is up-to-date with the master branch
This pull request links relevant issues as Fixes #11556
There are new or updated tests validating the change
All existing tests pass
Documentation has been updated to reflect this change - N/A (bug fixes, no API changes)

…typeorm#11556) When using upsert() with a primary key as the conflict path, PostgreSQL now correctly updates existing rows instead of creating duplicates. This aligns PostgreSQL behavior with MSSQL by including auto-increment primary keys in INSERT statements when users explicitly provide ID values.

qodo-free-for-open-source-projects · 2025-12-04T09:45:39Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Logic Correctness The condition logic appears inverted. The original code excluded auto-increment columns UNLESS it was MSSQL with override behavior. The new code excludes them UNLESS it's (MSSQL OR Postgres) with override behavior. However, the parentheses grouping suggests the intent is to check if EITHER driver type is present AND override is happening. Verify this matches the intended behavior - the condition should allow inclusion when override is detected for both MSSQL and Postgres families. !( (this.connection.driver.options.type === "mssql" \|\| DriverUtils.isPostgresFamily( this.connection.driver, )) && this.isOverridingAutoIncrementBehavior(column) ) Test Coverage The test only verifies the happy path with explicit ID values. Consider adding test cases for edge scenarios such as: attempting upsert without providing an ID value (should still auto-generate), mixing explicit and auto-generated IDs in batch operations, and verifying that the sequence continues correctly after explicit ID insertion. it("should upsert with primary key as conflict path", () => Promise.all( connections.map(async (connection) => { if (!connection.driver.supportedUpsertTypes.length) return const postRepository = connection.getRepository(Post) // First upsert - should insert a new post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post" }, { conflictPaths: ["id"] }, ) // Second upsert - should update the existing post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post2" }, { conflictPaths: ["id"] }, ) // Verify only one post exists with the updated title const posts = await postRepository.find() posts.length.should.be.equal( 1, "Should have only one post, not duplicate", ) posts[0].id!.should.be.equal(1) posts[0].title.should.be.equal("Upserted post2") }), ))

qodo-free-for-open-source-projects · 2025-12-04T09:46:51Z

PR Code Suggestions ✨

Latest suggestions up to b39fa41

Category	Suggestion	Impact
Possible issue	Add proper statement separators Add newline separators and a trailing semicolon to the `mssql` batch query for better statement separation when using `IDENTITY_INSERT`. src/query-builder/InsertQueryBuilder.ts [1136-1144] if ( this.connection.driver.options.type === "mssql" && this.expressionMap.mainAlias!.hasMetadata && columns.some((column) => this.isOverridingAutoIncrementBehavior(column), ) ) { - query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF` + query = `SET IDENTITY_INSERT ${tableName} ON;\n${query};\nSET IDENTITY_INSERT ${tableName} OFF;` } Apply / Chat Suggestion importance[1-10]: 2 __ Why: The suggestion to add newlines and a trailing semicolon is a minor stylistic improvement, as the existing semicolon-separated statements are already valid for `mssql` batch execution.	Low
More

Previous suggestions

Suggestions up to commit 4f9e3a9

Category	Suggestion	Impact
Possible issue	Apply IDENTITY_INSERT to all inserts Extract the MSSQL `IDENTITY_INSERT` logic into a reusable method and apply it to both `createMergeExpression` and `createInsertExpression` to ensure consistent behavior for all insert operations. src/query-builder/InsertQueryBuilder.ts [1136-1144] -if ( - this.connection.driver.options.type === "mssql" && - this.expressionMap.mainAlias!.hasMetadata && - columns.some((column) => - this.isOverridingAutoIncrementBehavior(column), - ) -) { - query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF` +// Extract to a method that can be reused in both createInsertExpression and createMergeExpression +protected wrapWithIdentityInsert(query: string, tableName: string, columns: ColumnMetadata[]): string { + if ( + this.connection.driver.options.type === "mssql" && + this.expressionMap.mainAlias!.hasMetadata && + columns.some((column) => + this.isOverridingAutoIncrementBehavior(column), + ) + ) { + return `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF` + } + return query } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant oversight where the `IDENTITY_INSERT` logic for MSSQL is only applied to `MERGE` statements, leaving regular `INSERT` statements with explicit identity values to fail.	Medium
General	Check both databaseName and propertyPath Update the conflict column filtering logic to check both `databaseName` and `propertyPath` when determining which columns to add to the `MERGE` source. src/query-builder/InsertQueryBuilder.ts [1179-1185] const additionalColumns = conflictColumns.filter( (conflictColumn) => !columns.some( (col) => - col.databaseName === conflictColumn.databaseName, + col.databaseName === conflictColumn.databaseName \|\| + col.propertyPath === conflictColumn.propertyPath, ), ) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that comparing columns by `databaseName` alone is insufficient and `propertyPath` should also be checked for robustness, preventing potential bugs where a conflict column might be missed.	Medium

Suggestions up to commit 842661f

Category	Suggestion	Impact
Possible issue	Fix PostgreSQL primary key exclusion logic Correct the logic for excluding auto-increment columns in PostgreSQL. The current change incorrectly prevents `upsert` operations with manually provided primary keys by wrongly applying MSSQL-specific behavior to PostgreSQL. src/query-builder/InsertQueryBuilder.ts [743-754] !(this.connection.driver.options.type === "spanner") && !(this.connection.driver.options.type === "oracle") && !DriverUtils.isSQLiteFamily(this.connection.driver) && !DriverUtils.isMySQLFamily(this.connection.driver) && !(this.connection.driver.options.type === "aurora-mysql") && !( - (this.connection.driver.options.type === "mssql" \|\| - DriverUtils.isPostgresFamily( - this.connection.driver, - )) && + this.connection.driver.options.type === "mssql" && this.isOverridingAutoIncrementBehavior(column) +) && +!( + DriverUtils.isPostgresFamily(this.connection.driver) && + column.isGenerated && + !this.isOverridingAutoIncrementBehavior(column) ) Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical logic flaw introduced in the PR, where PostgreSQL is incorrectly grouped with MSSQL. This change would break `upsert` operations with manually provided primary keys on PostgreSQL, as demonstrated by the new test case. The proposed fix correctly separates the logic for the two database systems.	High

Suggestions up to commit 97ee592

Category	Suggestion	Impact
General	Add test case for auto-incrementing ID Enhance the new `upsert` test by adding a case for auto-incrementing primary keys where the ID is not provided, ensuring the change did not cause a regression. test/functional/repository/basic-methods/repository-basic-methods.test.ts [876-904] it("should upsert with primary key as conflict path", () => Promise.all( connections.map(async (connection) => { if (!connection.driver.supportedUpsertTypes.length) return const postRepository = connection.getRepository(Post) // First upsert - should insert a new post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post" }, { conflictPaths: ["id"] }, ) // Second upsert - should update the existing post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post2" }, { conflictPaths: ["id"] }, ) - // Verify only one post exists with the updated title - const posts = await postRepository.find() + // Third upsert - should insert a new post with an auto-generated id + await postRepository.upsert( + { title: "New post" }, + { conflictPaths: ["id"] }, + ) + + // Verify posts + const posts = await postRepository.find({ order: { id: "ASC" } }) posts.length.should.be.equal( - 1, - "Should have only one post, not duplicate", + 2, + "Should have two posts", ) posts[0].id!.should.be.equal(1) posts[0].title.should.be.equal("Upserted post2") + posts[1].id!.should.be.greaterThan(1) + posts[1].title.should.be.equal("New post") }), )) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a missing test case for auto-incrementing primary keys, which improves the test's robustness by ensuring no regressions were introduced.	Medium
General	Refactor complex condition into a method Refactor the complex conditional logic for excluding auto-increment columns into a new helper method, `isIncrementColumnMustBeExcluded`, to improve code clarity. src/query-builder/InsertQueryBuilder.ts [748-754] -!( - this.connection.driver.options.type === "mssql" && - this.isOverridingAutoIncrementBehavior(column) -) +!this.isIncrementColumnMustBeExcluded(column) Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a complex conditional that could be refactored into a helper method, which would improve code readability and maintainability.	Low

OSA413 · 2025-12-04T13:21:57Z

Hello! Why did you close the PR?

CHOIJEWON · 2025-12-05T01:09:57Z

@OSA413

I noticed that the tests I added were failing for both SAP and MSSQL, so I temporarily closed the PR because it seems like some adjustments are needed. I also have a few personal commitments at the moment, so I’ll revisit and fix the issues as soon as I can!

pkg-pr-new · 2025-12-05T02:00:43Z

typeorm-sql-js-example

npm i https://pkg.pr.new/typeorm/typeorm@11820

commit: b39fa41

CHOIJEWON · 2025-12-05T02:16:14Z

Test Failures Analysis

While adding tests for #11556, I discovered two pre-existing bugs in the MERGE INTO implementation that were previously untested. Based on my analysis, these failures appear to be not caused by my changes but were revealed by the new test case.

However, I acknowledge that I might be missing something in my analysis. If my changes are indeed causing these issues, I'm happy to investigate further and provide a more comprehensive fix.

Bug 1: SAP HANA - Missing column in MERGE source

Test Case

should upsert with primary key as conflict path

Error Message

QueryFailedError: invalid column name: mergeIntoSource.id: line 1 col 282 (at pos 281)
  at SapQueryRunner.query (src\driver\sap\SapQueryRunner.ts:235:23)
  at InsertQueryBuilder.execute (src\query-builder\InsertQueryBuilder.ts:164:33)

Root Cause

In createMergeIntoSourceExpression() (InsertQueryBuilder.ts:1139-1245), SAP HANA uses the MERGE INTO syntax which requires all conflict path columns to be present in the USING clause.

However, getInsertedColumns() excludes auto-increment primary keys for SAP by default (lines 740-756), even when users explicitly provide ID values.

Flow of the issue:

User calls upsert({ id: 1, title: "..." }, { conflictPaths: ["id"] })
getInsertedColumns() returns columns WITHOUT id (because SAP is not in the condition at line 749)
createMergeIntoSourceExpression() generates USING clause without id column
MERGE query's ON clause tries to reference mergeIntoSource.id
Column doesn't exist in the USING clause → SQL error

Why it wasn't caught before

This test scenario (using primary key as conflict path) didn't exist before
SAP was never included in the condition at line 749 that checks isOverridingAutoIncrementBehavior()
The bug existed in the original codebase but was never triggered

Evidence that this is pre-existing

I tested by reverting my changes completely:

git stash
# Run test with original code
npm run test:fast -- --grep "should upsert with primary key as conflict path"

Result: Same failure occurs, confirming this is not caused by my changes.

Proposed Fix

Add SAP to the condition at line 749 in getInsertedColumns():

!(
    (this.connection.driver.options.type === "mssql" ||
        this.connection.driver.options.type === "sap" ||  // Add SAP
        DriverUtils.isPostgresFamily(this.connection.driver)) &&
    this.isOverridingAutoIncrementBehavior(column)
)

Bug 2: MSSQL - DEFAULT not allowed for IDENTITY columns in MERGE

Test Case

Same test: should upsert with primary key as conflict path

Error Message

QueryFailedError: Error: DEFAULT or NULL are not allowed as explicit identity values.
  at /home/runner/work/typeorm/typeorm/src/driver/sqlserver/SqlServerQueryRunner.ts:277:30

Environment: CI Linux environment (ubuntu-18)

Root Cause

In createMergeIntoInsertValuesExpression() (InsertQueryBuilder.ts:1263-1269), the code unconditionally uses DEFAULT for all generated columns with generationStrategy !== "uuid":

if (
    (column.isGenerated && column.generationStrategy === "uuid" && ...) ||
    (column.isGenerated && column.generationStrategy !== "uuid")  // ← Problem
) {
    expression += `DEFAULT`  // ← Uses DEFAULT even when user provided explicit value
}

The issue:

When a user provides { id: 1 }, the id column is included in getInsertedColumns()
This is because MSSQL already checks isOverridingAutoIncrementBehavior() in the original code (line 749)
However, createMergeIntoInsertValuesExpression() still generates DEFAULT instead of using the actual value from mergeSourceAlias.id
MSSQL's IDENTITY columns cannot accept DEFAULT as an explicit value → SQL error

Why it wasn't caught before

MSSQL was already checking isOverridingAutoIncrementBehavior() in the original code (line 749)
This means the bug existed in the original codebase for MSSQL
But this test scenario (MERGE INTO with primary key conflict path) didn't exist, so the bug in createMergeIntoInsertValuesExpression() was never triggered

Proposed Fix

In createMergeIntoInsertValuesExpression(), check if user explicitly provided a value before using DEFAULT:

if (
    (column.isGenerated &&
        column.generationStrategy === "uuid" &&
        this.connection.driver.isUUIDGenerationSupported()) ||
    (column.isGenerated &&
        column.generationStrategy !== "uuid" &&
        !this.isOverridingAutoIncrementBehavior(column))  // Add this check
) {
    expression += `DEFAULT`
} else {
    expression += `${mergeSourceAlias}.${this.escape(column.databaseName)}`
}

Summary

My Changes (in this PR)

// src/query-builder/InsertQueryBuilder.ts:748-753
!(
-   this.connection.driver.options.type === "mssql" &&
+   (this.connection.driver.options.type === "mssql" ||
+       DriverUtils.isPostgresFamily(this.connection.driver)) &&
    this.isOverridingAutoIncrementBehavior(column)
)

What my changes do:

✅ Fix the original issue [Bug] Can't not update row with upsert() methond when conflict key is primary key #11556 for PostgreSQL
✅ Add test coverage that revealed pre-existing MERGE INTO bugs
✅ Align PostgreSQL behavior with MSSQL (which already had this logic)

What my changes DON'T do:

❌ Introduce new bugs
❌ Break existing functionality

The Real Issues

Both test failures are caused by pre-existing bugs in the MERGE INTO implementation:

SAP: Never checked if user provided explicit ID values → conflict columns missing from MERGE source
MSSQL: createMergeIntoInsertValuesExpression() always uses DEFAULT for generated columns, ignoring user-provided values

These bugs existed before my changes but were never caught because:

No tests covered the scenario of using primary key as conflict path with MERGE INTO
The bugs only manifest when getInsertedColumns() includes auto-increment primary keys

Next Steps

I see a few options:

Option A: Fix all bugs in this PR

Include fixes for both SAP and MSSQL MERGE INTO bugs
Keeps all related fixes together
May expand the scope of this PR

Option B: Fix SAP only in this PR

Add SAP to my existing changes
Handle MSSQL MERGE bug separately (since it's a different issue)

Option C: Separate PRs

Keep this PR focused on PostgreSQL fix only
Create separate PR(s) for MERGE INTO bugs
Temporarily skip the failing test for SAP/MSSQL

Verification Request

I want to be completely sure my analysis is correct. If reviewers spot anything I'm missing or if my changes are actually causing these issues, please let me know. I'm happy to:

Run additional tests to verify the root cause
Provide more detailed debugging information
Adjust my approach if needed
Investigate further if there's something I overlooked

The last thing I want is to introduce bugs while fixing another issue. Your feedback is greatly appreciated!

qodo-free-for-open-source-projects · 2025-12-05T02:17:13Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Logic Correctness The condition logic appears inverted. The original code excluded auto-increment columns UNLESS it was MSSQL with override behavior. The new code excludes them UNLESS it's (MSSQL OR Postgres) with override behavior. However, the parentheses grouping suggests the intent is to check if EITHER driver type is present AND override is happening. Verify this matches the intended behavior - the condition should allow inclusion when override is detected for both MSSQL and Postgres families. !( (this.connection.driver.options.type === "mssql" \|\| DriverUtils.isPostgresFamily( this.connection.driver, )) && this.isOverridingAutoIncrementBehavior(column) ) Test Coverage Gap The test only verifies the happy path with explicit ID values. Consider adding test cases for edge scenarios such as: (1) upsert without explicit ID to ensure auto-increment still works normally, (2) upsert with explicit ID on non-Postgres drivers to verify no regression, and (3) verify the sequence continues correctly after explicit ID insertion. it("should upsert with primary key as conflict path", () => Promise.all( connections.map(async (connection) => { if (!connection.driver.supportedUpsertTypes.length) return const postRepository = connection.getRepository(Post) // First upsert - should insert a new post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post" }, { conflictPaths: ["id"] }, ) // Second upsert - should update the existing post with id=1 await postRepository.upsert( { id: 1, title: "Upserted post2" }, { conflictPaths: ["id"] }, ) // Verify only one post exists with the updated title const posts = await postRepository.find() posts.length.should.be.equal( 1, "Should have only one post, not duplicate", ) posts[0].id!.should.be.equal(1) posts[0].title.should.be.equal("Upserted post2") }), ))

qodo-free-for-open-source-projects · 2025-12-05T02:18:06Z

PR Code Suggestions ✨

No code suggestions found for the PR.

CHOIJEWON · 2025-12-05T16:21:29Z

@gioboa

If you don’t mind, could you please take a look at the issue when you have a moment?

gioboa

Some tests failed, I merged develop into this branch to align it and trigger the tests

gioboa

Few tests are failing, can you verify them please? @CHOIJEWON

CHOIJEWON · 2025-12-06T03:25:44Z

@gioboa

comment: #11820 (comment)

I’m not sure if my previous comment might have been missed, but I did some analysis on why the tests are failing.
When you have a chance, could you please take a look?
Just to clarify — I’m genuinely asking in case the comment didn’t show up on your side. The test failures are valid 🥲

gioboa

I see, option A is the best one I guess.
Btw we can't merge a PR with failed tests, the button is disabled 🙈
Thanks for your great help 🙏🚀

Both SAP HANA and MSSQL do not allow inserting explicit values into IDENTITY columns. This commit fixes MERGE INTO upsert when using primary keys or other identity columns as conflict paths. The fix ensures identity columns are: 1. Included in USING clause (needed for ON clause conflict detection) 2. Set to DEFAULT in INSERT clause (instead of explicit values) Changes: - createMergeIntoSourceExpression: Add conflict path columns to USING clause even when auto-generated (for ON clause matching) - createMergeIntoInsertValuesExpression: Check isOverridingAutoIncrementBehavior before inserting values, use DEFAULT for identity columns Fixes errors: - SAP HANA: 'cannot insert generated identity column field id' - MSSQL: 'Cannot insert explicit value for identity column when IDENTITY_INSERT is OF'

…lumns Fixes 'DEFAULT not allowed for IDENTITY columns in MERGE' error when explicitly providing values for auto-increment primary keys in MERGE queries. This mirrors the existing IDENTITY_INSERT handling in createInsertExpression() for consistency. The SET IDENTITY_INSERT wrapper is only applied when: - Using MSSQL driver - Entity has metadata - User explicitly provides values for identity columns

qodo-free-for-open-source-projects · 2025-12-08T07:23:32Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Logic Complexity The condition at lines 749-753 has become complex with nested driver checks. The logic combines MSSQL and PostgreSQL family checks with `isOverridingAutoIncrementBehavior()`. Consider extracting this into a helper method for better readability and maintainability. Also verify that the parentheses grouping is correct - the current structure means the auto-increment override check only applies to MSSQL and PostgreSQL, which may not be the intended behavior. !( (this.connection.driver.options.type === "mssql" \|\| DriverUtils.isPostgresFamily( this.connection.driver, )) && this.isOverridingAutoIncrementBehavior(column) ) Potential Side Effect The `createMergeIntoSourceExpression` method now modifies the columns list by adding conflict path columns. This change affects MERGE INTO statements for SAP HANA and potentially other drivers. Verify that this behavior is correct for all drivers that use MERGE INTO syntax, not just PostgreSQL. The comment mentions SAP HANA specifically, but the change applies to all drivers using this method. let columns = this.getInsertedColumns() // For MERGE INTO, we need to include conflict path columns in the source // even if they are auto-generated (e.g., identity columns in SAP HANA) // because they are needed for the ON clause (conflict detection) if ( this.expressionMap.onUpdate?.conflict && this.expressionMap.mainAlias!.hasMetadata ) { const conflictPaths = Array.isArray( this.expressionMap.onUpdate.conflict, ) ? this.expressionMap.onUpdate.conflict : [this.expressionMap.onUpdate.conflict] const conflictColumns = this.expressionMap.mainAlias!.metadata.columns.filter( (column) => conflictPaths.includes(column.databaseName) \|\| conflictPaths.includes(column.propertyPath), ) // Add conflict columns that are not already in the inserted columns const additionalColumns = conflictColumns.filter( (conflictColumn) => !columns.some( (col) => col.databaseName === conflictColumn.databaseName, ), ) if (additionalColumns.length > 0) { columns = [...columns, ...additionalColumns] } } Incomplete Condition At line 1316, the condition checks for non-UUID generated columns and whether auto-increment is being overridden. However, this doesn't account for other generation strategies like 'rowid' or 'identity'. Verify that this logic correctly handles all possible generation strategies, especially for PostgreSQL's SERIAL and IDENTITY columns. (column.isGenerated && column.generationStrategy !== "uuid" && !this.isOverridingAutoIncrementBehavior(column))

qodo-free-for-open-source-projects · 2025-12-08T07:24:46Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent session state corruption on failure To prevent MSSQL session state corruption, handle `SET IDENTITY_INSERT` at the driver level with a `try...finally` block during query execution, rather than by concatenating it into the query string. src/query-builder/InsertQueryBuilder.ts [1136-1144] if ( this.connection.driver.options.type === "mssql" && this.expressionMap.mainAlias!.hasMetadata && columns.some((column) => this.isOverridingAutoIncrementBehavior(column), ) ) { - query = `SET IDENTITY_INSERT ${tableName} ON; ${query}; SET IDENTITY_INSERT ${tableName} OFF` + this.expressionMap.extraParameters.identityInsert = true } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant robustness issue where a query failure could leave the MSSQL session in a corrupted state, which is a critical flaw in the new logic.	Medium
High-level	PR scope exceeds its description The PR includes undocumented changes for the `MERGE INTO` statement, which is outside the described scope of fixing PostgreSQL `upsert`. The PR description should be updated to include these changes for clarity and proper review. Examples: src/query-builder/InsertQueryBuilder.ts [1156-1190] let columns = this.getInsertedColumns() // For MERGE INTO, we need to include conflict path columns in the source // even if they are auto-generated (e.g., identity columns in SAP HANA) // because they are needed for the ON clause (conflict detection) if ( this.expressionMap.onUpdate?.conflict && this.expressionMap.mainAlias!.hasMetadata ) { const conflictPaths = Array.isArray( ... (clipped 25 lines) src/query-builder/InsertQueryBuilder.ts [1314-1316] (column.isGenerated && column.generationStrategy !== "uuid" && !this.isOverridingAutoIncrementBehavior(column)) Solution Walkthrough: Before: // src/query-builder/InsertQueryBuilder.ts // Only MSSQL allows overriding auto-increment PKs in inserts protected getInsertedColumns(): ColumnMetadata[] { // ... if (column.isGenerated && column.generationStrategy === "increment") { if (this.connection.driver.options.type === "mssql" && this.isOverridingAutoIncrementBehavior(column)) { // keep column } else { return false; // filter out column } } // ... } // MERGE INTO logic does not account for conflict paths protected createMergeIntoSourceExpression(...) { const columns = this.getInsertedColumns(); // ... uses columns } After: // src/query-builder/InsertQueryBuilder.ts // Both MSSQL and Postgres family drivers allow overriding auto-increment PKs protected getInsertedColumns(): ColumnMetadata[] { // ... if (column.isGenerated && column.generationStrategy === "increment") { if ((this.connection.driver.options.type === "mssql" \|\| DriverUtils.isPostgresFamily(this.connection.driver)) && this.isOverridingAutoIncrementBehavior(column)) { // keep column } else { return false; // filter out column } } // ... } // MERGE INTO logic now includes conflict path columns protected createMergeIntoSourceExpression(...) { let columns = this.getInsertedColumns(); if (this.expressionMap.onUpdate?.conflict) { // Add conflict columns to `columns` if not already present } // ... uses columns } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the PR contains undocumented changes to the `MERGE INTO` logic, which affects drivers other than PostgreSQL and is a significant omission from the PR description.	Medium
General	Improve performance of conflict column lookup Improve the performance of conflict column lookup by using direct metadata methods like `findColumnWithPropertyPath` or `findColumnByDatabaseName` instead of iterating through all columns. src/query-builder/InsertQueryBuilder.ts [1165-1176] const conflictPaths = Array.isArray( this.expressionMap.onUpdate.conflict, ) ? this.expressionMap.onUpdate.conflict : [this.expressionMap.onUpdate.conflict] -const conflictColumns = - this.expressionMap.mainAlias!.metadata.columns.filter( - (column) => - conflictPaths.includes(column.databaseName) \|\| - conflictPaths.includes(column.propertyPath), - ) +const conflictColumns = conflictPaths.flatMap((path) => { + const column = + this.expressionMap.mainAlias!.metadata.findColumnWithPropertyPath( + path, + ) ?? + this.expressionMap.mainAlias!.metadata.findColumnByDatabaseName( + path, + ) + return column ? [column] : [] +}) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion offers a valid performance improvement by replacing an O(N*M) lookup with a more direct and efficient approach, enhancing code readability and maintainability.	Low
More

CHOIJEWON · 2025-12-08T07:28:08Z

@gioboa

The PR has been updated!
I made some additional adjustments and updated the description accordingly.
If you have some time, I would really appreciate your review 🙌

coveralls · 2025-12-08T07:34:08Z

coverage: 80.875% (+0.006%) from 80.869%
when pulling b39fa41 on CHOIJEWON:fix/postgres-upsert
into 35ca51a on typeorm:master.

Suggestions addressed.

qodo-free-for-open-source-projects bot added the Review effort 2/5 label Dec 4, 2025

CHOIJEWON marked this pull request as draft December 4, 2025 09:51

CHOIJEWON closed this Dec 4, 2025

CHOIJEWON reopened this Dec 5, 2025

CHOIJEWON marked this pull request as ready for review December 5, 2025 02:16

Merge branch 'master' into fix/postgres-upsert

842661f

gioboa requested review from alumni, michaelbromley, naorpeled and pkuczynski December 5, 2025 17:48

gioboa reviewed Dec 5, 2025

View reviewed changes

gioboa previously requested changes Dec 5, 2025

View reviewed changes

gioboa reviewed Dec 6, 2025

View reviewed changes

CHOIJEWON marked this pull request as draft December 8, 2025 02:00

CHOIJEWON added 2 commits December 8, 2025 12:40

CHOIJEWON marked this pull request as ready for review December 8, 2025 07:23

qodo-free-for-open-source-projects bot added the Review effort 3/5 label Dec 8, 2025

Merge branch 'master' into fix/postgres-upsert

4f9e3a9

gioboa added the Marked For Double Check label Dec 10, 2025

pkuczynski approved these changes Dec 10, 2025

View reviewed changes

pkuczynski changed the title ~~fix(postgres): include primary key in upsert when explicitly provided~~ fix(postgres/mssql/saphana): include primary key in upsert when explicitly provided Dec 10, 2025

gioboa removed the Marked For Double Check label Dec 10, 2025

gioboa self-requested a review January 7, 2026 21:39

gioboa requested review from alumni and removed request for alumni January 7, 2026 21:40

Merge branch 'master' into fix/postgres-upsert

b39fa41

Uh oh!

fix(postgres/mssql/saphana): include primary key in upsert when explicitly provided #11820

Are you sure you want to change the base?

fix(postgres/mssql/saphana): include primary key in upsert when explicitly provided #11820

Conversation

CHOIJEWON commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

🐛 Problems

1. PostgreSQL: Duplicate Rows on Primary Key Upsert

2. SAP HANA/MSSQL: Identity Columns as Conflict Paths Fail

3. MSSQL: IDENTITY_INSERT Not Applied to MERGE Queries

💡 Root Causes

Issue 1: PostgreSQL Auto-Increment Exclusion

Issue 2: MERGE INTO Source Missing Conflict Columns

Issue 3: Missing IDENTITY_INSERT for MERGE

✅ Solutions

Fix 1: Include Primary Keys in PostgreSQL Upserts

Fix 2: Support Identity Columns in MERGE Conflict Paths

Fix 3: Add IDENTITY_INSERT to MERGE Queries

🧪 Testing

🔍 Impact Analysis

Affected Drivers

Breaking Changes

Pull-Request Checklist

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 4, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

OSA413 commented Dec 4, 2025

Uh oh!

CHOIJEWON commented Dec 5, 2025

Uh oh!

pkg-pr-new bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CHOIJEWON commented Dec 5, 2025

Test Failures Analysis

Bug 1: SAP HANA - Missing column in MERGE source

Test Case

Error Message

Root Cause

Why it wasn't caught before

Evidence that this is pre-existing

Proposed Fix

Bug 2: MSSQL - DEFAULT not allowed for IDENTITY columns in MERGE

Test Case

Error Message

Root Cause

Why it wasn't caught before

Proposed Fix

Summary

My Changes (in this PR)

The Real Issues

Next Steps

Option A: Fix all bugs in this PR

Option B: Fix SAP only in this PR

Option C: Separate PRs

Verification Request

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 5, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-free-for-open-source-projects bot commented Dec 5, 2025

PR Code Suggestions ✨

Uh oh!

CHOIJEWON commented Dec 5, 2025

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

CHOIJEWON commented Dec 6, 2025

CHOIJEWON commented Dec 4, 2025 •

edited

Loading

qodo-free-for-open-source-projects bot commented Dec 4, 2025 •

edited

Loading

pkg-pr-new bot commented Dec 5, 2025 •

edited

Loading

coveralls commented Dec 8, 2025 •

edited

Loading