fix: do not create junction table metadata when it already exists #11114
Conversation
WalkthroughThe update modifies the logic for handling auto-generated junction entity metadata in many-to-many relations to prevent duplication when a user-defined junction table exists. It introduces new tests and entity schemas to verify correct metadata construction and relation handling with user-defined junction tables. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant DocumentRelation
participant EntityMetadataBuilder
User->>EntityMetadataBuilder: Defines many-to-many relation to Document via DocumentRelation
Document->>EntityMetadataBuilder: Defines one-to-many relation to DocumentRelation
DocumentRelation->>EntityMetadataBuilder: Defines many-to-one relation to Document
EntityMetadataBuilder->>EntityMetadataBuilder: Check for user-defined junction table
alt User-defined junction exists
EntityMetadataBuilder-->>EntityMetadataBuilder: Do not add auto-generated junction metadata
else No user-defined junction
EntityMetadataBuilder-->>EntityMetadataBuilder: Add auto-generated junction metadata
end
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/metadata-builder/JunctionEntityMetadataBuilder.ts (1)
362-364: Misplaced JSDoc comment.The JSDoc comment "Collects inverse referenced columns from the given join column args" is incorrectly placed above the
changeDuplicatedColumnNamesmethod. This method handles duplicate column name resolution, not collecting inverse referenced columns.Move this comment to the correct location or update it to describe the actual method:
- /** - * Collects inverse referenced columns from the given join column args. - */ protected changeDuplicatedColumnNames(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/metadata-builder/EntityMetadataBuilder.ts(1 hunks)src/metadata-builder/JunctionEntityMetadataBuilder.ts(3 hunks)test/github-issues/10412/entity/entities.ts(1 hunks)test/github-issues/10412/issue-10412.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/github-issues/10412/issue-10412.ts (2)
test/utils/test-utils.ts (3)
createTestingConnections(388-482)reloadTestingDatabases(500-505)closeTestingConnections(487-495)test/github-issues/10412/entity/entities.ts (1)
documentRelationEntitySchema(65-87)
🔇 Additional comments (7)
test/github-issues/10412/entity/entities.ts (2)
23-45: Well-designed test entities for the junction table scenario.The
userEntitySchemacorrectly defines a many-to-many relationship with a join table named "document_relation", which matches the explicitly defineddocumentRelationEntitySchemaentity. This setup perfectly reproduces the bug scenario where TypeORM would create duplicate metadata.
65-87: Explicit junction table entity properly defined.The
documentRelationEntitySchemaentity correctly defines the same table ("document_relation") that's referenced in the User's joinTable configuration. This dual definition (implicit via joinTable + explicit via entity schema) is exactly what was causing the metadata duplication issue.src/metadata-builder/JunctionEntityMetadataBuilder.ts (2)
297-297: Method visibility change is necessary for the fix.Changing
collectReferencedColumnsfromprotectedtopublicis required so thatEntityMetadataBuildercan access this method when reusing existing junction table metadata.
329-329: Method visibility change is necessary for the fix.Changing
collectInverseReferencedColumnsfromprotectedtopublicenablesEntityMetadataBuilderto properly collect inverse referenced columns when reusing existing junction metadata.src/metadata-builder/EntityMetadataBuilder.ts (1)
295-334: Excellent fix for the junction table metadata duplication issue.The logic correctly addresses the core problem by:
- Detection: Checks if junction table metadata already exists by comparing
metadata.target === joinTable.name- Reuse: Instead of creating new metadata, it reuses the existing metadata
- Integration: Properly registers foreign keys, join columns, and junction metadata from the existing entity
The column filtering logic (lines 315-328) correctly matches existing metadata columns with the referenced columns that would be created by the junction builder. This ensures the relation has the proper join columns without duplicating metadata.
This fix elegantly prevents the conflict described in the PR objectives where auto-created metadata would override user-defined metadata in
entityMetadatasMap.test/github-issues/10412/issue-10412.ts (2)
27-35: Effective test for verifying metadata deduplication.This test correctly verifies that the repository metadata contains exactly one relation, confirming that the fix prevents duplicate junction table metadata creation. The assertion
expect(repository.metadata.relations.length).to.be.eql(1)directly validates the core issue resolution.
37-50: Comprehensive test for the reported error condition.This test effectively verifies that the
EntityPropertyNotFoundErrormentioned in GitHub issue #10412 no longer occurs when querying with relations and where conditions. The test setup correctly uses thedocumentRelationEntitySchemaentity and includes both relation loading and filtering, which was the problematic scenario.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/data-source/DataSource.ts(1 hunks)test/github-issues/10412/issue-10412.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/data-source/DataSource.ts (4)
src/common/EntityTarget.ts (1)
EntityTarget(7-11)src/metadata/EntityMetadata.ts (1)
EntityMetadata(30-1139)src/repository/Repository.ts (1)
metadata(53-55)src/util/ObjectUtils.ts (1)
ObjectUtils(3-82)
🪛 Biome (1.9.4)
test/github-issues/10412/issue-10412.ts
[error] 11-11: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (1)
src/data-source/DataSource.ts (1)
736-760: Excellent fix for the junction table metadata conflict issue.The two-phase approach correctly prioritizes user-defined entities over auto-generated junction entities. The logic ensures that:
- User-defined entities (non-junction) are added to the map first
- Junction entities are only added if they don't conflict with existing entries
- This prevents the bug where auto-created metadata overrides user-defined metadata
The solution directly addresses the core issue described in the PR objectives and maintains backward compatibility.
|
@michaelbromley @mguida22 any chance someone can take a look at this 👀 ? it blocks upgrading from 3.13 onwards for anyone who uses junction tables |
commit: |
5279d77 to
71cee6d
Compare
@sgarner thanks 🙏🏻 all tests now work in my downstream repo, https://github.com/ragrag/typeorm/actions/runs/15789744898 |
package-lock.json
Outdated
| "version": "0.3.25", | ||
| "lockfileVersion": 3, | ||
| "requires": true, | ||
| "packages": { | ||
| "": { | ||
| "name": "typeorm", | ||
| "version": "0.3.24", | ||
| "version": "0.3.25", |
There was a problem hiding this comment.
Please revert this change
| import { expect } from "chai" | ||
| import { documentRelationEntitySchema } from "./entity/entities" | ||
|
|
||
| describe("github issues > #10412 EntityPropertyNotFoundError when joinTable is used", () => { |
There was a problem hiding this comment.
We are moving away from creating tests for specific GitHub issues. Instead we need tests which demonstrate that EntityMetadataBuilder works correctly. Unfortunately there isn't a test suite for this class yet, so one needs to be created.
Please move this test file to test/functional/entity-metadata/entity-metadata-builder.ts and adjust test descriptions appropriately for this context.
Co-authored-by: Simon Garner <simon@equalogic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/entity-metadata/entity-metadata-builder.ts(1 hunks)test/functional/entity-metadata/entity/JunctionTableEntities.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/functional/entity-metadata/entity-metadata-builder.ts
[error] 15-15: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (2)
test/functional/entity-metadata/entity/JunctionTableEntities.ts (1)
1-87: Well-structured test entity schemas for junction table testing.The entity schemas effectively model the scenario described in the PR objectives. The relationships between User, Document, and DocumentRelation (user-defined junction table) are properly configured with correct column mappings and join table definitions. This setup will adequately test the junction table metadata duplication fix.
test/functional/entity-metadata/entity-metadata-builder.ts (1)
35-58: Excellent test coverage for the junction table metadata fix.The test cases effectively validate the fix described in the PR objectives:
- The first test verifies that repository metadata correctly includes relations when user-defined junction tables are used
- The second test ensures that querying with relations doesn't throw
EntityPropertyNotFoundErrorThese tests directly address the core issues mentioned in the PR (resolving conflicts from duplicate metadata entries and preventing query errors).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/functional/entity-metadata/entity/junction-table/entities.ts (3)
3-9: Consider adding a User relation to the DocumentRelation interface.The
DocumentRelationinterface represents a junction table but only has a relation toDocument. For completeness and bidirectional navigation, consider adding auserproperty to establish the relationship to theUserentity.interface DocumentRelation { documentRelationId: number documentId: number userId?: number document?: Document + user?: User }
52-62: Remove unnecessary joinColumn specification from one-to-many relation.The
joinColumnspecification in a one-to-many relation is typically unnecessary since the foreign key constraint is defined on the "many" side (DocumentRelation entity). The foreign key relationship is already properly defined in the DocumentRelation entity schema.relations: { documentRelation: { type: "one-to-many", target: "document_relation", inverseSide: "document", - joinColumn: { - name: "document_id", - referencedColumnName: "documentId", - }, }, },
65-87: Consider adding a User relation to complete the junction table.The
DocumentRelationentity is missing a relation to theUserentity. While this might be intentional for testing specific scenarios, a complete junction table typically has relations to both entities it connects.relations: { document: { type: "many-to-one", target: "document", inverseSide: "documentRelation", joinColumn: { name: "document_id", referencedColumnName: "documentId", }, }, + user: { + type: "many-to-one", + target: "user", + joinColumn: { + name: "user_id", + referencedColumnName: "userId", + }, + }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/entity-metadata/entity-metadata-builder.ts(1 hunks)test/functional/entity-metadata/entity/junction-table/entities.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/entity-metadata/entity-metadata-builder.ts
🔇 Additional comments (1)
test/functional/entity-metadata/entity/junction-table/entities.ts (1)
23-45: Excellent junction table configuration for testing.The User entity schema correctly defines the many-to-many relationship using a custom junction table. The join table configuration properly maps to the user-defined "document_relation" entity, which is exactly what this test needs to verify the junction table metadata fix.
|
@sgarner thank you again 🙏🏻 i addressed all requested changes |
|
@sgarner apologies for the bump 🙏🏻 any updates on when we can merge this? |
Description of change
After the introduction of entityMetadatasMap in f12a95c an issue arises where a junction table defined by user in entities would have multiple metadata entries (one for entity manually defined by the user, and one for the auto-created metadata by typeorm). This would cause problems as the junction table metadata would override the user defined metadata in entityMetadatasMap.
This PR ensures metadata for junction tables already created by users do not exist twice in entityMetadatas and thus avoiding conflicts when its resolved from entityMetadatasMap. This works by always using the user defined entity metadata if it exists.
This bug exists from version 3.13 and blocks upgrading if junction tables are used.
Fixes:
#10412 (includes repro)
#10463
#10643
Apart from those issues, as a side effect of this fix, other various issues related to duplicate sql statements while generating migrations in conjunction with user defined junction tables, for example #7955, #11126 and similar issues are fixed or partially fixed
Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #0000Summary by CodeRabbit
Bug Fixes
Tests
New Features