Walkthrough

Adds a protected method on DataSource that deduplicates auto-generated junction entity metadatas by promoting user-defined junction metadata, remapping columns/foreign keys, updating relation references, and removing duplicate junction entries; includes tests exercising user-defined junction tables and the regression from issue #11637.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant DS as DataSource
    participant EMB as EntityMetadataBuilder
    participant DED as deduplicateJunctionEntityMetadatas()

    DS->>EMB: buildMetadatas()
    EMB-->>DS: returns entityMetadatas (auto-generated + user-defined)
    DS->>DED: deduplicateJunctionEntityMetadatas(entityMetadatas)
    DED->>DED: identify duplicate junction metadatas
    DED->>DED: promote user-defined junction metadata
    DED->>DED: remap columns & foreign keys to user metadata
    DED->>DED: update relation metadata to reference promoted metadata
    DED->>DED: remove superseded auto-generated junction metadatas
    DED-->>DS: deduplicated entityMetadatas

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix: do not create junction table metadata when it already exists #11114 — Addresses duplicate junction-entity metadata by preventing auto-creation in EntityMetadataBuilder; closely related to deduplication strategy here.
• Revert #11114 do not create junction table metadata when it already exists #11660 — Also deals with auto-generated vs user-defined junction metadata handling; overlaps with the same problem-space.

Suggested labels

comp: relations, need review help, size-m

Suggested reviewers

• alumni
• sgarner
• gioboa

Poem

🐰
I hopped through metadatas, neat and spry,
Found duplicates hiding, gave them a try.
I stitched columns and keys in a tidy line,
So junction rows hold what they should: keys that align.
Rejoice — no more NULLs — a rabbit's small rhyme.

Pre-merge checks and 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.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

• typeorm-sql-js-example

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

commit: e2d1b65

coverage: 80.919% (+0.003%) from 80.916%
when pulling e2d1b65 on ashwintemkar:fix/junction-metadata-precedence-11637
into 7de850e on typeorm:master.

@sgarner @ragrag pardon me for tagging,

I have opened this Draft PR to check whether anything more is needed out of the fix I have put up, please let me know if this is ready for review, as the core problem is fixed

Thanks for pointing those out! @ragrag , I pulled the stray junction-table regression test back out and added inline comments explaining why we prefer user-defined metadata over the synthetic junction entry. Let me know if you’d like any more detail there.

Thanks for pointing those out! @ragrag , I pulled the stray junction-table regression test back out and added inline comments explaining why we prefer user-defined metadata over the synthetic junction entry. Let me know if you’d like any more detail there.

sorry i meant to only remove the enabledDrivers: ['sqlite'] part (the line my comment was on initially) we should keep the tests for sure, rest lgtm!

so dropping the explicit enabledDrivers override I hit the Postgres-only failure I saw constraint "... does not exist", which meant the auto-generated junction metadata was still hanging around alongside the user-defined entity. That’s why I extended EntityMetadataBuilder

We now merge the auto-generated junction metadata back into the user-defined entity, reuse its columns, and drop the duplicate entry. Without that, Postgres tries to create the same PK/FKs twice and the regression resurfaces.
Relations that were wired to the synthetic junction metadata are re-pointed to the user entity so many-to-many inserts keep their FK values vet_specialty, feed_resources_categories, etc.
Existing coverage for the #11114 scenario is still passing, and the new regression test for #11637 passes on Postgres with the fix npm test -- --grep "junction".

So the earlier logic from commit 95baf98 is still there, and the new builder hook just finishes the job by reconciling the actual metadata objects.

We now merge the auto-generated junction metadata back into the user-defined entity, reuse its columns, and drop the duplicate entry. Without that, Postgres tries to create the same PK/FKs twice and the regression resurfaces. Relations that were wired to the synthetic junction metadata are re-pointed to the user entity so many-to-many inserts keep their FK values vet_specialty, feed_resources_categories, etc. Existing coverage for the #11114 scenario is still passing, and the new regression test for #11637 passes on Postgres with the fix npm test -- --grep "junction".

could we perhaps do this merging once for all conflicting junction metadatas during metaDatasMap registration (or merging such tables before the metadata generation step)? that would be a much simpler single change, you think something like that could work?

e.g:

entityMetadatasMap: new Map(
    withDeduplicatedJunctionTables(entityMetadatas).map((metadata) => [metadata.target, metadata]),
),

also it could be worth investigating if typeorm already has some internal method for metadata merging or table merging that we could instead of mergeWithUserDefinedJunctions

throwing other ideas i feel that might solve this more straightforwardly. originally entityMetaDatasMap was introduced in f12a95c to make metadata lookups faster, we could potentially explore some variants that would maintain the fast lookups that have potential to work:

1 - use the previous metadata logic that loops over entityMetadatas and adding a cache on top in favor of entityMetadatasMap, so the initial lookup will have similar result pre 3.13 and consequent lookups will be resolved from the cache to the correct metadata. essentially having entityMetadatasMap but building it in a lazy fashion (imho the most straight forward)

2 - in findMetadata, if entity found in entityMetadatasMap is a junction entity (or an entity we know is a potential duplicate junction entity from some precalculation we did) do not return it and, fallback to old logic that loops over entityMetadatas from the commit above

3 - gathering a set of user defined tables that have a junction pair in entityMetadatas, in findMetadata check if target is part of that set and if that's the case fallback to old logic that loops over entityMetadatas from the commit above

4- wild idea and not sure if it'll work but constructing entityMetadatasMap from the reverse of entityMetadatas, my guess is that junction and virtual tables are pushed to the end of entityMetadatas causing the map overriding to happen

theoretically all of those solutions should maintain the same behaviour as before the regression introduced in 3.13 (from the commit above) and would avoid potentially error prone metadata merging, let me know what you think

Hey @ragrag thanks for the detailed suggestions!

I moved the reconciliation to metadata registration:

1. we now deduplicate the auto-generated junction metadata before entityMetadatasMap is built
2. reuse the junction FK definitions on the user entity
3. repoint the relations to the user metadata.

That keeps the map fast, avoids the extra builder hook, and resolves the Postgres constraint/index failures.

Happy to iterate further!! if you spot anything else.

also for your reference

throwing other ideas i feel that might solve this more straightforwardly. originally entityMetaDatasMap was introduced in f12a95c to make metadata lookups faster, we could potentially explore some variants that would maintain the fast lookups that have potential to work:

1 - use the previous metadata logic that loops over entityMetadatas and adding a cache on top in favor of entityMetadatasMap, so the initial lookup will have similar result pre 3.13 and consequent lookups will be resolved from the cache to the correct metadata. essentially having entityMetadatasMap but building it in a lazy fashion (imho the most straight forward)

2 - in findMetadata, if entity found in entityMetadatasMap is a junction entity (or an entity we know is a potential duplicate junction entity from some precalculation we did) do not return it and, fallback to old logic that loops over entityMetadatas from the commit above

3 - gathering a set of user defined tables that have a junction pair in entityMetadatas, in findMetadata check if target is part of that set and if that's the case fallback to old logic that loops over entityMetadatas from the commit above

4- wild idea and not sure if it'll work but constructing entityMetadatasMap from the reverse of entityMetadatas, my guess is that junction and virtual tables are pushed to the end of entityMetadatas causing the map overriding to happen

theoretically all of those solutions should maintain the same behaviour as before the regression introduced in 3.13 (from the commit above) and would avoid potentially error prone metadata merging, let me know what you think

A lazy cache (option 1) and the targeted fallbacks (options 2 & 3) still leave duplicate metadata in entityMetadatas, so the schema builder continued to create duplicate PK/FK definitions in Postgres.

Reversing the insertion order (option 4) depends on incidental ordering and still risks selecting the wrong metadata.

Hence the new deduplication step keeps the fast lookups from entityMetadatasMap, avoids metadata merging inside the builder, and gives us a single, predictable place to reconcile the two metadata sources.

Amazing work!

Hence the new deduplication step keeps the fast lookups from entityMetadatasMap, avoids metadata merging inside the builder, and gives us a single, predictable place to reconcile the two metadata sources.

just wondering if we still need all the other changes done now (the change in findMetadata, and buildMetadata), shouldn't the metadata merging with deduplicateJunctionEntityMetadatas alone pre-registration with the old way fix the issue?

btw did you manage to find any typeorm internals that do the metadata merging?

just wondering if we still need all the other changes done now (the change in findMetadata, and buildMetadata),

so I initally thought that the ideas were good when this #11720 (comment) was suggested but then I thought deduplicateJunctionEntityMetadatas would bring the edge here.

shouldn't the metadata merging with deduplicateJunctionEntityMetadatas alone pre-registration with the old way fix the issue?

and you’re right that the heavy lifting happens in the new deduplicateJunctionEntityMetadatas step. I reverted the tweaks to findMetadata and to the entityMetadatasMap construction so they’re back to the exact logic we had with the synthetic junction metadata removed before the map is built, those extra guard rails aren’t needed.

btw did you manage to find any typeorm internals that do the metadata merging?

And also I didn’t find an existing helper in TypeORM that performs this reconciliation. I checked the metadata builders and schema helpers, but nothing exposed the junction metadata we needed to reuse.

so currently
deduplicateJunctionEntityMetadatas runs right after buildEntityMetadatas, strips out the auto generated junction entries, and rewires any relations that were pointing at them and because the duplicates are gone, findMetadata’s original implementation is returning the correct user defined entity without the additional filtering.

for now I think this can go in as the original findMetadata logic behaves exactly like pre‑3.13, and both the #11114 functional coverage and the #11637 regression test pass npm test -- --grep "entity-metadata-builder" and npm test -- --grep "junction"

so currently deduplicateJunctionEntityMetadatas runs right after buildEntityMetadatas, strips out the auto generated junction entries, and rewires any relations that were pointing at them and because the duplicates are gone, findMetadata’s original implementation is returning the correct user defined entity without the additional filtering.

coool, seems like this covers all the edges so looks good from my side, perhaps other maintainers are more familiar in the matter of metadata merging

so currently deduplicateJunctionEntityMetadatas runs right after buildEntityMetadatas, strips out the auto generated junction entries, and rewires any relations that were pointing at them and because the duplicates are gone, findMetadata’s original implementation is returning the correct user defined entity without the additional filtering.

coool, seems like this covers all the edges so looks good from my side, perhaps other maintainers are more familiar in the matter of metadata merging

alright so should I mark this ready for review?, I mean turn into actual PR from draft?

alright so should I mark this ready for review?, I mean turn into actual PR from draft?

sure feel free to!

@ragrag any idea by when this is merging in ?

Hi @gioboa do my changes check out? and can this pr be merged in?

Hey @gioboa and @ragrag , since @sgarner and @alumni might take some time to review according to their availability, as all checks have passed, I had a small request, as maintainers could y'all add a hacktoberfest accepted tag onto this PR, since I have participated in the hacktoberfest, I think adding the tag might count as contribution, just want to see if that works.

thanks a tonne @gioboa

PR Code Suggestions ✨

 const existingForeignKeySignatures = new Set(
     userMetadata.foreignKeys.map((foreignKey) => {
         const columnSignature = foreignKey.columns
             .map((column) => column.databaseName)
             .sort()
             .join(",")
-        return `${columnSignature}->${foreignKey.referencedEntityMetadata.tablePath}`
+        return `${columnSignature}->${foreignKey.referencedEntityMetadata.tablePath}:${foreignKey.onDelete}:${foreignKey.onUpdate}:${foreignKey.deferrable}`
     }),
 )

+if (userMetadata.tableMetadataArgs.type && userMetadata.tableMetadataArgs.type !== "junction") {
+    throw new TypeORMError(
+        `Cannot promote entity "${userMetadata.name}" to junction table: entity is already defined as type "${userMetadata.tableMetadataArgs.type}".`,
+    )
+}
 userMetadata.tableMetadataArgs.type = "junction"
 userMetadata.tableType = "junction"
 userMetadata.isJunction = true
 userMetadata.ownerColumns = ownerColumns
 userMetadata.inverseColumns = inverseColumns
 userMetadata.foreignKeys.push(...foreignKeysToRegister)

 const mapColumn = (column: ColumnMetadata): ColumnMetadata => {
     const match = userMetadata.columns.find(
         (candidate) =>
             candidate.databaseName === column.databaseName,
     )
     if (!match) {
         throw new TypeORMError(
             `Column "${column.databaseName}" referenced by the join table "${junctionMetadata.tableName}" was not found in the user-defined entity "${userMetadata.name}".`,
         )
     }
 
-    match.referencedColumn = column.referencedColumn
-    match.relationMetadata = column.relationMetadata
+    const clonedColumn = Object.create(Object.getPrototypeOf(match))
+    Object.assign(clonedColumn, match)
+    clonedColumn.referencedColumn = column.referencedColumn
+    clonedColumn.relationMetadata = column.relationMetadata
 
-    return match
+    return clonedColumn
 }

The test file must have .test.ts extension to be considered.

sure @gioboa I will look into the changes needed

PR Code Suggestions ✨

No code suggestions found for the PR.

Code Review by Qodo (Alpha)

🐞 Bugs (2) 📘 Rule Violations (0) 📎 Requirement Gaps (0) 💡 Suggestions (0)

I fixed the test file name.

okay, so is this pr now waiting only for approvals right?