fix(postgres/mssql/saphana): include primary key in upsert by CHOIJEWON · Pull Request #11820 · typeorm/typeorm

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)

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 6151734

Category	Suggestion	Impact
Possible issue	Only add explicitly provided conflict columns When adding conflict columns for a `MERGE INTO` statement, only include those for which a non-null value is explicitly provided in the `values` set to prevent incorrect conflict detection. src/query-builder/InsertQueryBuilder.ts [1289-1303] 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, - ), - ) + // Add conflict columns that are not already in the inserted columns, + // but only when the user explicitly provided a value for them. + const additionalColumns = conflictColumns.filter((conflictColumn) => { + const alreadyIncluded = columns.some( + (col) => col.databaseName === conflictColumn.databaseName, + ) + if (alreadyIncluded) return false + return this.getValueSets().some((valueSet) => { + const value = conflictColumn.getEntityValue(valueSet) + return value !== undefined && value !== null + }) + }) + Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a subtle but critical bug where unconditionally adding conflict columns could lead to incorrect conflict detection and duplicate rows. The proposed fix correctly aligns the logic with the user's intent by only adding columns for which a value is explicitly provided.	High
Possible issue	Ensure identity insert is always disabled Wrap the MSSQL `IDENTITY_INSERT` logic in a `TRY/CATCH` block to ensure `IDENTITY_INSERT` is always turned `OFF`, even if the main query fails. src/query-builder/InsertQueryBuilder.ts [1254-1262] 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 = `BEGIN TRY +SET IDENTITY_INSERT ${tableName} ON; +${query} +SET IDENTITY_INSERT ${tableName} OFF; +END TRY +BEGIN CATCH +SET IDENTITY_INSERT ${tableName} OFF; +THROW; +END CATCH` } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a critical robustness issue where a failed `MERGE` query could leave `IDENTITY_INSERT ON` for the session, and proposes a standard `TRY/CATCH` pattern in T-SQL to ensure it is always turned off, which is a significant improvement.	Medium
Update

Previous suggestions

Suggestions up to commit 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;` } 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

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: 1598f20

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 🙏🚀

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.

gioboa

is this PR still valid? can you solve the conflicts please?

…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.

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 · 2026-02-25T02:19:54Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

🐞\ ≡ Correctness (3) ⭐ New (1)

📘\ ⚙ Maintainability (1) ⭐ New (1)

1. Undefined test variable 🐞 ≡ ⭐

Description

The new upsert test iterates over connections, but this suite defines and uses dataSources (no
connections variable exists), causing the test file to fail at runtime/compile time.

Code

test/functional/repository/basic-methods/repository-basic-methods.test.ts[R914-942]

+        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")
+                }),
+            ))

Evidence
dataSources is the only connection array defined in this test suite, while the added test uses
connections, which is not declared anywhere in the file.
test/functional/repository/basic-methods/repository-basic-methods.test.ts[28-51]
test/functional/repository/basic-methods/repository-basic-methods.test.ts[914-942]
test/functional/repository/basic-methods/repository-basic-methods.test.ts[52-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new test uses `connections.map(...)` but `connections` is not defined in this file; all other tests use `dataSources`.

### Issue Context
This causes immediate test failure and blocks CI.

### Fix Focus Areas
- test/functional/repository/basic-methods/repository-basic-methods.test.ts[914-942]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. MERGE SELECT mis-maps columns 🐞 ≡

Description

In MERGE+INSERT FROM SELECT, conflict-path columns are appended to the end of the source column
list, potentially reordering the positional mapping between SELECT outputs and source column names.
This can cause wrong ON-clause matching and/or wrong inserted values (or even column-count errors)
for MSSQL merge-into upserts.

Code

src/query-builder/InsertQueryBuilder.ts[R1274-1307]

+        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]
+            }

Evidence

The PR appends additional conflict columns to the end of the columns list used to build the MERGE
source. In the INSERT FROM SELECT path, MSSQL keeps the original SELECT query but still names its
output columns via a positional column list built from columns, so appending/reordering columns
can rename the wrong SELECT positions (or mismatch the number of SELECT outputs). For non-MSSQL, the
code re-derives targetColumns from getInsertedColumns() (excluding the newly appended conflict
columns), so those appended columns won’t be aliased into the source at all, despite being required
by the ON clause.

src/query-builder/InsertQueryBuilder.ts[1271-1307]
src/query-builder/InsertQueryBuilder.ts[1310-1359]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMergeIntoSourceExpression()` appends conflict-path columns to the end of the MERGE source `columns` list. In the `insertFromSelect` path, MSSQL relies on a positional alias column list built from `columns`, so the append can rename the wrong SELECT output positions (or cause a count mismatch). For non-MSSQL, the alias-rewrite logic uses `getInsertedColumns()` again, so the newly added conflict columns are not aliased at all.
### Issue Context
This can lead to incorrect conflict detection (wrong `ON` matches) and/or incorrect inserted values in MERGE-based upserts when `insertFromSelect` is used with conflict paths that include columns not present in `getInsertedColumns()` (e.g., generated identity PKs).
### Fix Focus Areas
- src/query-builder/InsertQueryBuilder.ts[1271-1360]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Uses deprecated this.connection 📘 ⚙ ⭐

Description

New code uses the deprecated connection getter (this.connection.driver.options.type) instead of
the file-standard this.dataSource, making style inconsistent and risking future breakage if
deprecated access is removed. This conflicts with the requirement to avoid inconsistent/low-signal
changes in core code.

Code

src/query-builder/InsertQueryBuilder.ts[R1246-1253]

+        // Inserting a specific value for an auto-increment primary key in mssql requires enabling IDENTITY_INSERT
+        // IDENTITY_INSERT can only be enabled for tables where there is an IDENTITY column and only if there is a value to be inserted (i.e. supplying DEFAULT is prohibited if IDENTITY_INSERT is enabled)
+        if (
+            this.connection.driver.options.type === "mssql" &&
+            this.expressionMap.mainAlias!.hasMetadata &&
+            columns.some((column) =>
+                this.isOverridingAutoIncrementBehavior(column),
+            )

Evidence
The checklist requires avoiding inconsistent style/noise. The added MSSQL IDENTITY_INSERT guard uses
this.connection even though the surrounding code uses this.dataSource, and connection is
explicitly marked deprecated in the codebase.
Rule 4: Remove AI-generated noise
src/query-builder/InsertQueryBuilder.ts[1246-1253]
src/query-builder/QueryBuilder.ts[62-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New code in `InsertQueryBuilder` uses the deprecated `this.connection` getter instead of `this.dataSource`, which is inconsistent with the surrounding style and relies on a deprecated API.

## Issue Context
`QueryBuilder.connection` is marked deprecated in favor of `dataSource`. The new MSSQL `IDENTITY_INSERT` wrapper should follow existing conventions in this file.

## Fix Focus Areas
- src/query-builder/InsertQueryBuilder.ts[1246-1255]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. IDENTITY_INSERT enabled too broadly 🐞 ≡

Description

The new MSSQL MERGE wrapper enables IDENTITY_INSERT when any value set provides an explicit identity
value, but other rows in the same batch may still omit it. This can produce invalid MERGE statements
(e.g., DEFAULT/NULL identity values) despite the code comment noting DEFAULT is prohibited under
IDENTITY_INSERT.

Code

src/query-builder/InsertQueryBuilder.ts[R1252-1262]

+        // Inserting a specific value for an auto-increment primary key in mssql requires enabling IDENTITY_INSERT
+        // IDENTITY_INSERT can only be enabled for tables where there is an IDENTITY column and only if there is a value to be inserted (i.e. supplying DEFAULT is prohibited if IDENTITY_INSERT is enabled)
+        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`
+        }

Evidence

The wrapper uses columns.some(column => isOverridingAutoIncrementBehavior(column)), and
isOverridingAutoIncrementBehavior itself returns true if *any* valueSet has a
non-null/non-undefined value for the identity PK. This means IDENTITY_INSERT may be enabled for a
multi-row upsert batch even if only some rows actually provide identity values, contradicting the
comment that DEFAULT is prohibited when IDENTITY_INSERT is enabled.

src/query-builder/InsertQueryBuilder.ts[1252-1262]
src/query-builder/InsertQueryBuilder.ts[1015-1027]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMergeExpression()` now wraps MERGE statements with `SET IDENTITY_INSERT ... ON/OFF` when *any* value set provides an explicit identity value. For multi-row upserts, this can enable IDENTITY_INSERT even when other rows omit the identity value, which can yield invalid SQL per the existing comment.
### Issue Context
`isOverridingAutoIncrementBehavior()` uses `.some(...)` across value sets, so it does not ensure the identity value is present for all rows in a batch.
### Fix Focus Areas
- src/query-builder/InsertQueryBuilder.ts[1008-1028]
- src/query-builder/InsertQueryBuilder.ts[1246-1262]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit 1598f20

Results up to commit N/A

🐞 Bugs (2)
📘 Rule violations (0)
📎 Requirement gaps (0)
🎨 UX Issues (0)

🐞\ ≡ Correctness (2)

1. MERGE SELECT mis-maps columns 🐞 ≡

Description

In MERGE+INSERT FROM SELECT, conflict-path columns are appended to the end of the source column
list, potentially reordering the positional mapping between SELECT outputs and source column names.
This can cause wrong ON-clause matching and/or wrong inserted values (or even column-count errors)
for MSSQL merge-into upserts.

Code

src/query-builder/InsertQueryBuilder.ts[R1274-1307]

+        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]
+            }

Evidence

The PR appends additional conflict columns to the end of the columns list used to build the MERGE
source. In the INSERT FROM SELECT path, MSSQL keeps the original SELECT query but still names its
output columns via a positional column list built from columns, so appending/reordering columns
can rename the wrong SELECT positions (or mismatch the number of SELECT outputs). For non-MSSQL, the
code re-derives targetColumns from getInsertedColumns() (excluding the newly appended conflict
columns), so those appended columns won’t be aliased into the source at all, despite being required
by the ON clause.

src/query-builder/InsertQueryBuilder.ts[1271-1307]
src/query-builder/InsertQueryBuilder.ts[1310-1359]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMergeIntoSourceExpression()` appends conflict-path columns to the end of the MERGE source `columns` list. In the `insertFromSelect` path, MSSQL relies on a positional alias column list built from `columns`, so the append can rename the wrong SELECT output positions (or cause a count mismatch). For non-MSSQL, the alias-rewrite logic uses `getInsertedColumns()` again, so the newly added conflict columns are not aliased at all.
### Issue Context
This can lead to incorrect conflict detection (wrong `ON` matches) and/or incorrect inserted values in MERGE-based upserts when `insertFromSelect` is used with conflict paths that include columns not present in `getInsertedColumns()` (e.g., generated identity PKs).
### Fix Focus Areas
- src/query-builder/InsertQueryBuilder.ts[1271-1360]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. IDENTITY_INSERT enabled too broadly 🐞 ≡

Description

The new MSSQL MERGE wrapper enables IDENTITY_INSERT when any value set provides an explicit identity
value, but other rows in the same batch may still omit it. This can produce invalid MERGE statements
(e.g., DEFAULT/NULL identity values) despite the code comment noting DEFAULT is prohibited under
IDENTITY_INSERT.

Code

src/query-builder/InsertQueryBuilder.ts[R1252-1262]

+        // Inserting a specific value for an auto-increment primary key in mssql requires enabling IDENTITY_INSERT
+        // IDENTITY_INSERT can only be enabled for tables where there is an IDENTITY column and only if there is a value to be inserted (i.e. supplying DEFAULT is prohibited if IDENTITY_INSERT is enabled)
+        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`
+        }

Evidence

The wrapper uses columns.some(column => isOverridingAutoIncrementBehavior(column)), and
isOverridingAutoIncrementBehavior itself returns true if *any* valueSet has a
non-null/non-undefined value for the identity PK. This means IDENTITY_INSERT may be enabled for a
multi-row upsert batch even if only some rows actually provide identity values, contradicting the
comment that DEFAULT is prohibited when IDENTITY_INSERT is enabled.

src/query-builder/InsertQueryBuilder.ts[1252-1262]
src/query-builder/InsertQueryBuilder.ts[1015-1027]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMergeExpression()` now wraps MERGE statements with `SET IDENTITY_INSERT ... ON/OFF` when *any* value set provides an explicit identity value. For multi-row upserts, this can enable IDENTITY_INSERT even when other rows omit the identity value, which can yield invalid SQL per the existing comment.
### Issue Context
`isOverridingAutoIncrementBehavior()` uses `.some(...)` across value sets, so it does not ensure the identity value is present for all rows in a batch.
### Fix Focus Areas
- src/query-builder/InsertQueryBuilder.ts[1008-1028]
- src/query-builder/InsertQueryBuilder.ts[1246-1262]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

CHOIJEWON · 2026-02-25T02:24:10Z

@gioboa

I’ve resolved the merge conflicts, but I’m encountering a status 522 error during the Coveralls parallel build step.
Could you please check if this is something on the CI/Coveralls side?

gioboa

Manually triggered the pipeline 🤞

CHOIJEWON · 2026-02-26T01:17:35Z

@gioboa

The tests are passing successfully, but the Coveralls Parallel step continues to fail 😭. Is it possible that this is related to my code, or might there be another cause? I’d appreciate any guidance or suggestions.

gioboa

Coverall infrastructure is down since yesterday so it's not your fault, we need to wait until the problem is fixed on their side 🥲

OSA413 · 2026-02-26T08:04:32Z

yeah https://status.coveralls.io/

# Conflicts: # src/query-builder/InsertQueryBuilder.ts

sonarqubecloud · 2026-04-11T09:11:43Z

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

qodo-free-for-open-source-projects · 2026-04-11T09:16:13Z

Persistent review updated to latest commit 1598f20

michaelbromley · 2026-09-18T15:27:00Z

This repository was recently restructured into a pnpm workspace, and the typeorm package moved from the repo root into packages/typeorm (PR #12854). Everything moved as a pure rename with no content changes, so your branch should replay across the move cleanly once git's rename detection limit is raised:

git fetch origin
git -c merge.renameLimit=999999 -c diff.renameLimit=999999 rebase origin/master

If your branch has diverged enough that a full rebase isn't worth it, scripts/migrate-pr-to-packages-layout.sh --replay (run on top of master) rewrites the paths of your net diff instead. Both approaches, plus a path-mapping table and a prompt you can hand to a coding agent, are documented in scripts/migrate-pr-to-packages-layout.md.

Sorry for the disruption, and thanks for your patience.

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

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 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

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

gioboa requested changes Feb 24, 2026

View reviewed changes

CHOIJEWON added 3 commits February 25, 2026 10:33

CHOIJEWON force-pushed the fix/postgres-upsert branch from b39fa41 to 6151734 Compare February 25, 2026 02:11

gioboa reviewed Feb 25, 2026

View reviewed changes

gioboa reviewed Feb 26, 2026

View reviewed changes

pkuczynski removed Review effort 2/5 labels Mar 13, 2026

Merge remote-tracking branch 'origin/master' into work-11820

1598f20

# Conflicts: # src/query-builder/InsertQueryBuilder.ts

pkuczynski changed the title ~~fix(postgres/mssql/saphana): include primary key in upsert when explicitly provided~~ fix(postgres/mssql/saphana): include primary key in upsert Apr 11, 2026

pkuczynski added bug comp: query builder labels Apr 13, 2026

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

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

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

qodo-free-for-open-source-projects Bot commented Feb 25, 2026 •

edited

Loading