Skip to content

fix: specify columns for metadata-less multi-value inserts #11855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Cprakhar wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: specify columns for metadata-less multi-value inserts #11855

Cprakhar wants to merge 1 commit into from
+109 −44

Conversation

@Cprakhar
Copy link
Contributor

@Cprakhar Cprakhar commented Dec 18, 2025

Description of change

  • Fix InsertQueryBuilder to always emit column lists for metadata-less multi-row inserts.
  • Add regression coverage for manager.insert() and QueryBuilder inserts without entity metadata.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • This pull request links relevant issues as Fixes #3837
  • There are new or updated tests validating the change (tests/**.test.ts)
  • Documentation has been updated to reflect this change (docs/docs/**.md)

Fixes #3837

@qodo-free-for-open-source-projects
Copy link

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

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

3837 - PR Code Verified

Compliant requirements:

  • Fix manager.insert() to generate correct SQL when inserting multiple rows into a table without entity metadata
  • Ensure column names are specified in the INSERT query for multi-row inserts
  • Remove or fix the if (valueSets.length === 1) check that causes columns to be omitted for multi-row inserts
  • Support inserting data using connection.manager.insert() with table name as string

Requires further human verification:

  • Verify that the generated SQL matches the expected format INSERT INTO difficulties (type) VALUES ('Easy'), ('Medium'), ('Hard') through actual database execution
  • Confirm that auto-generated columns (like id) are properly handled and generated by the database
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Column Order Consistency

The new getColumnNamesFromValueSets method iterates through all value sets and adds columns in the order they are encountered. If different value sets have columns in different orders or some value sets are missing columns that others have, this could lead to misaligned VALUES clauses where column positions don't match the column list order.

 
protected getColumnNamesFromValueSets(
    valueSets: ObjectLiteral[],
): string[] {
    const columns: string[] = []
    valueSets.forEach((valueSet) => {
        Object.keys(valueSet).forEach((columnName) => {
            if (columns.indexOf(columnName) === -1) {
                columns.push(columnName)
            }
        })
    })
    return columns
}
Missing Column Handling

When building the VALUES expression, the code assumes all columns from getColumnNamesFromValueSets exist in every valueSet. If a valueSet is missing a column that exists in another valueSet, valueSet[columnName] will be undefined, which may not be handled correctly and could lead to incorrect SQL generation or runtime errors.

 
columnNames.forEach((columnName, columnIndex) => {
    if (columnIndex === 0) {
        expression += "("
    }

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 18, 2025

commit: 0503d1d

@qodo-free-for-open-source-projects
Copy link

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

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use a Set for better performance

Refactor getColumnNamesFromValueSets to use a Set instead of an array with
indexOf for improved performance when collecting unique column names.

src/query-builder/InsertQueryBuilder.ts [934-946]

 protected getColumnNamesFromValueSets(
     valueSets: ObjectLiteral[],
 ): string[] {
-    const columns: string[] = []
+    const columns = new Set<string>()
     valueSets.forEach((valueSet) => {
         Object.keys(valueSet).forEach((columnName) => {
-            if (columns.indexOf(columnName) === -1) {
-                columns.push(columnName)
-            }
+            columns.add(columnName)
         })
     })
-    return columns
+    return Array.from(columns)
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a performance bottleneck in the new getColumnNamesFromValueSets function and proposes a more efficient implementation using a Set, which is a valid and good practice for collecting unique items.

Low
  • More

@coveralls
Copy link

coveralls commented Dec 18, 2025

Coverage Status

coverage: 80.801% (+0.006%) from 80.795%
when pulling 0503d1d on Cprakhar:fix/metadata-less-insert-columns
into 22ed3ec on typeorm:master.

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

Reviewers

@alumni alumni Awaiting requested review from alumni

@sgarner sgarner Awaiting requested review from sgarner

@naorpeled naorpeled Awaiting requested review from naorpeled

@michaelbromley michaelbromley Awaiting requested review from michaelbromley

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Manager insert query with auto-generated columns

2 participants

@Cprakhar @coveralls