typeorm: 0.2.20 release breaks entity repo resolving mechanism

Issue type:

[ ] question [x] bug report [ ] feature request [*] documentation issue

Database system/driver:

[ ] cordova [ ] mongodb [ ] mssql [x] mysql / mariadb [ ] oracle [ ] postgres [ ] cockroachdb [ ] sqlite [ ] sqljs [ ] react-native [ ] expo

TypeORM version:

[X] latest [ ] @next [ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:

Before the 0.2.20 release, e.g. 0.2.19, we could use this kind of code to specify a count column in db:

  @Index()
  @Column({
    comment: "The count, without setting default option!"
  })
  private count: number = 0;

Everything works as expected, until the latest version released.

For now, if you use 0.2.20 version, during the db initialization stage, ORM will give the error like this:

The field `count` default value has not been set.

I highly recommend listing this as a breaking warning as it’s a big breaks for code. Or, is it a regression bug introducing by accident?

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 3
Comments: 23 (20 by maintainers)

Most upvoted comments

This is causing problems for me as well.

I’ve got two “Application” entities, one with schema: “account” and another with schema: “payment”

Due to the above changes references to “Application” always resolve to only one of the entities, regardless of which one is actually supplied.

I don’t think it’s that uncommon to have entities with the same name across different schemas.

josephmulholland on Oct 28, 2019

@silentroch definitely, the object reference should always be respected in the first priority as it’s the strongest anchor symbols in terms of the language level.

I saw maintainer’s replies for you, that doesn’t make too much sense for me as you cannot introduce a break change only relying on the rare or not.

We of course have some cases that we define the classes with the same name, same connection and the ORM shouldn’t force users not do that.

Think about I contributed for this repo several times about binary values as key and column, it’s definitely rare usages but I always think I need to push PRs for that, instead just making a local copy for myself, cause I believe typeORM should be a global and responsible project.

sevenryze on Oct 25, 2019

@pleerock yes, I’m sure to say that’s typeorm’s breaking change.

I dig some, I believe this issue is caused by #4804 PR, which breaks the repository metadata resolving logic.

protected findMetadata(target: Function|EntitySchema<any>|string): EntityMetadata|undefined {
        return this.entityMetadatas.find(metadata => {
            if (typeof metadata.target === "function" && typeof target === "function" && metadata.target.name === target.name) {  // <--- See this? Mark 1
                return true;
            }
            if (metadata.target === target) {
                return true;
            }
            if (target instanceof EntitySchema) {
                return metadata.name === target.options.name;
            }
            if (typeof target === "string") {
                if (target.indexOf(".") !== -1) {
                    return metadata.tablePath === target;
                } else {
                    return metadata.name === target || metadata.tableName === target;
                }
            }

            return false;
        });
    }

This logic is too open to resolve, e.g., if we have two different entities defined with the same class name - User. Then the metadata.target.name === target.name will be true even when comparing the first User with the second User, which is totally different. So every time we need the second User, this logic is gonna give us the first User, which is definitely wrong.

Could you check that? I think we at least shouldn’t use name as the comparing anchor as it’s too easy to collide.

If you agree with me, I could give a PR to fix that, but i’m not sure what’s the purpose of the #4804 .

sevenryze on Oct 25, 2019

@pleerock the ("database"."schema"."table_name") way should resolve my problem as I always manually set the entity name by hand. But I don’t know if that breaks the other areas.

What’s your thought? should I make a PR for this method? or we just revert the PR #4804 and try another way to resolve the lambda issue?

sevenryze on Nov 8, 2019

In my case I have two entities with the same name in same schema, just not to fetch extra fields in some cases.

If I pass class reference to the function, I expect that I get repository for it, but not for other, it is counterintuitive. If you want to use function name or key like database + schema + table name, there should be a fatal error on initialization if user have two entities with the same key.

silentroach on Nov 5, 2019

#3267 here is a long discussion about package death

@silentroach if you are going to continue to be toxic in every issue then this repository isn’t for you.

for me it is a very strange behavior that you pass some class reference and get repository for another, but they think it is ok 🤷‍♂

definitely, the object reference should always be respected in the first priority as it’s the strongest anchor symbols in terms of the language level.

unfortunately it’s not always true. And its especially sensed once you have serverless / lambda / microservices / monorepo / shared with browser or electron architectures and environments. Yes, in most simple projects you wouldn’t have this problem because you have only one node_modules, one representation of your entity and its symbol, one running process. But for those who organizing listed projects this is a big issue.

My suggestion to resolve this issue is to match by table path (“database”.“schema”.“table_name”). I’m not sure if it won’t bring any other problems for other users but from what I see it covers all three listed cases. Feel free to suggest something better.

pleerock on Nov 4, 2019