-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Revert #11114 do not create junction table metadata when it already exists #11660
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.
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
Conversation
WalkthroughThe metadata builder now always appends auto-generated junction entity metadata for many-to-many relations. Previously, it conditionally skipped appending when a non-junction entity shared the same table name. Associated functional tests and entity schema fixtures for the junction-table scenario were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant EMB as EntityMetadataBuilder
participant Rel as Relation (Many-to-Many)
participant JMF as Junction Metadata Factory
App->>EMB: buildEntityMetadatas()
EMB->>Rel: inspect relations
alt Junction needed
EMB->>JMF: create junction metadata
note right of JMF: Auto-generated junction entity metadata
JMF-->>EMB: junctionMetadata
par Old flow (before)
note over EMB: If user-defined non-junction entity<br/>shared the same table name, skip push
EMB-->>EMB: Conditional append
and New flow (now)
note over EMB: Always append generated<br/>junction metadata
EMB->>EMB: Unconditional append
end
else No junctions
EMB-->>App: proceed
end
EMB-->>App: entityMetadatas (includes generated junctions)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/metadata-builder/EntityMetadataBuilder.ts (1)
318-318: Add a brief comment explaining why duplicates are intentionally allowed again.Helps future readers understand the revert context and avoid reintroducing the guard.
this.computeInverseProperties( junctionEntityMetadata, entityMetadatas, ) + // Intentional revert of #11114: always append generated junction metadata. + // Context: fixes regressions discussed in #11637; dedup will be reevaluated later. entityMetadatas.push(junctionEntityMetadata)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/metadata-builder/EntityMetadataBuilder.ts(1 hunks)test/functional/entity-metadata/entity-metadata-builder.ts(0 hunks)test/functional/entity-metadata/entity/junction-table/entities.ts(0 hunks)
💤 Files with no reviewable changes (2)
- test/functional/entity-metadata/entity-metadata-builder.ts
- test/functional/entity-metadata/entity/junction-table/entities.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Release preview build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: tests-linux (20) / sap
- GitHub Check: tests-linux (20) / oracle
- GitHub Check: tests-linux (20) / mysql_mariadb_latest
- GitHub Check: tests-linux (18) / sap
- GitHub Check: tests-linux (20) / mssql
- GitHub Check: tests-linux (18) / mysql_mariadb_latest
- GitHub Check: tests-linux (18) / oracle
- GitHub Check: tests-linux (18) / mssql
- GitHub Check: tests-windows / sqlite
- GitHub Check: tests-linux (20) / cockroachdb
🔇 Additional comments (1)
src/metadata-builder/EntityMetadataBuilder.ts (1)
318-318: Revert restores unconditional junction metadata push — code check OK; run cross‑driver testsentityMetadatas.push(junctionEntityMetadata) is present at src/metadata-builder/EntityMetadataBuilder.ts:318. No other matches for dedup/duplicate/skip related to junction metadata were found in src. Run cross‑driver functional tests (many‑to‑many with synchronize on/off) to confirm #11637 is resolved and no duplicate-entity regressions surface.
…ists (typeorm#11114)" (typeorm#11660) This reverts commit 3c26cf1.
This reverts commit 3c26cf1.
The PR #11114 fixes legitimate bugs, but unfortunately also introduces regressions for certain use-cases, as described in #11637, which was introduces in v0.3.26.
For now, we should revert the changes and then take the time to review how #11114 can be implemented & tested in such a way as it does not introduce this regression.
Description of change
Just a revert of the commit referenced above, nothing else.
Pull-Request Checklist
masterbranchFixes #00000Summary by CodeRabbit