fix: prefix relation id columns contained in embedded entities (#6977) by nebkat · Pull Request #6981 · typeorm/typeorm

nebkat · 2020-10-27T20:38:00Z

Searches embedded entity columns for relation ID column if relation column is in embedded entity. If not found, creates new relation ID with embedded metadata set to match the relation column.

Fixes: #2254
Fixes: #3132
Fixes: #3226
Fixes: #6977

Description of change

See #6977.

Currently the relation column's entityMetadata is searched for columns matching the join column, rather than embeddedMetadata (when present).

typeorm/src/metadata-builder/RelationJoinColumnBuilder.ts

Line 136 in a5eb946

    
           let relationalColumn = relation.entityMetadata.ownColumns.find(column => column.databaseName === joinColumnName);

.

This changes that behavior to match

typeorm/src/metadata-builder/EntityMetadataBuilder.ts

Lines 152 to 156 in a5eb946

    
           if (relation.embeddedMetadata) { 
        
               relation.embeddedMetadata.indices.push(index); 
        
           } else { 
        
               relation.entityMetadata.ownIndices.push(index); 
        
           }

Pull-Request Checklist

Code is up-to-date with the master branch
npm run lint passes with this change
npm run test passes with this change
This pull request links relevant issues as Fixes #0000
There are new or updated unit tests validating the change
Documentation has been updated to reflect this change
The new commits follow conventions explained in CONTRIBUTING.md

imnotjames · 2020-10-29T18:49:02Z

Looks like this change causes the oracle tests to fail?

Tests run in CircleCI in open PRs and you can see the failure here - https://circleci.com/gh/typeorm/typeorm/4727?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

… identifier for oracle Problem introduced with bfaf779, typeorm#6981

nebkat · 2020-10-29T22:11:13Z

Oracle issue with max identifier length being 30 characters, looks like it never occurred in the past because the column names always excluded the embedded prefixes. Fixed by shortening a prefix in the edge cases with long identifiers.

pleerock · 2020-10-30T06:36:41Z

            const joinColumnName = joinColumnMetadataArg ? joinColumnMetadataArg.name : this.connection.namingStrategy.joinColumnName(relation.propertyName, referencedColumn.propertyName);

-            let relationalColumn = relation.entityMetadata.ownColumns.find(column => column.databaseName === joinColumnName);
+            const relationalColumns = relation.embeddedMetadata ? relation.embeddedMetadata.columns : relation.entityMetadata.ownColumns;


what if there is an embedded, but we point to a relation that not in the embedded?

I'm not sure what you mean, I think relation.embeddedMetadata is only set if relation is a column within an embedded entity. It was somewhat confusing initially but relation refers only to the foreign key column, not the target relation entity.

This particular section is what searches for an existing relation id column, and if the relation is located inside an embedded then it only makes sense to search for the relation id column also in the embedded rather than in the base of the entity.

okay, thank you for explanation

I've had a second look at this, and in theory we could allow relation IDs to not be in the embedded entity together with the relation, but I'm not sure whether that would cause additional confusion.

@Entity() class Other { @PrimaryColumn() id: number; } @Entity() class User { @Column(() => Embed) embed: Embed; @Column() embedOther1Id: number; @ManyToOne(() => Other) embedOther2: Other; } class Embed { @ManyToOne(() => Other) other1: Other; @Column() other2Id: number; }

Both relations here could match up correctly if we want, and if they are not matched up they will lead to a duplicate column error anyway. MongoDB is an exception since it maintains the embedded objects in DB, so both relation/ID have to be in the same entity.

pleerock · 2020-10-30T06:37:13Z

    favorites: number;

-    @Column(() => Subcounters)
+    @Column(() => Subcounters, {prefix: "sub"})


Why did you change exist test? You need to create a separate test for a new use case.

The columns inside embedded entities were being incorrectly generated previously. The correct column name is countersSubcountersWatchedUserId, which is 32 characters long, and too long for Oracle DB which can only take 30 characters. Before this fix, the column name would have been watchedUserId or something, so there was no problem, but the wrong column name was going unnoticed.

This test only checks the ORM side of things anyway, so the exact name in database was irrelevant, but I can change the variable name from subcounters to sub directly if you'd prefer. I have already created a new test case for the issue #6977.

the problem is, if we merge these changes, we introduce a breaking change. Right now Oracle users have same case working (even if column name is invalid), but if we merge these changes their codebases won't be working at all. We can't tell them "hey guys just monkey patch it using "prefix" like we did it on our tests". Proper solution is to fix related Oracle issue.

For this test anyway there is no solution for Oracle unless we plan to truncate column names automatically? Could potentially introduce a more informative error during column generation if target DB is Oracle since the DB error is very vague.

we already do truncate in some other places for Oracle

Might be something worth looking at with naming strategies for the future, gives the user the option for more meaningful change than simple truncation.

pleerock · 2020-10-31T11:39:14Z

@imnotjames it looks like this will introduce a serious breaking change for the users who have same use case. Right now what we have is a bug, this is true. If we merge these changes bug will be fixed, but it will also lead to schema re-creation and data loss for users. We need to decide how much acceptable it is to merge these changes not into the next.

nebkat · 2020-10-31T12:36:16Z

I see the difficulty of maintaining the project now 🙂 Would it be worth introducing a temporary option property to relations that applies this fix until next major release? Definitely shouldn't break existing setups, but unless the next release is coming soon it might be frustrating for users and will also lead to more broken columns being initiated in new projects. In my case I have multiple userId references and it fails because of duplicate column so I can't even keep the ORM side correct.

@ManyToOne(() => UserEntity, {fixEmbeddedRelationColumn: true})?

pleerock · 2020-10-31T13:21:00Z

if we go this way, but better to make same a connection-level option

nebkat · 2020-10-31T19:08:41Z

Sounds good, if you think this is best I can implement it. I propose maybe a field in BaseConnectionOptions e.g. targetVersion: "0.3.0" which defaults to the current release, string comparison as a condition for the breaking changes, along with a strong warning for users that things are likely to break if they target future versions. Once that version is released the old code gets removed, but hopefully additional feedback can be gained from users willing to test new features.

pleerock · 2020-11-02T04:59:34Z

I think it's better just to call them "enableNextFeatures", don't think we'll really try to support more than one next breaking version.

Adds a new flag to connection options to allow user to enabled planned future breaking changes before next release. See typeorm#6981.