fix: fix entityManager.getId for custom join table

[ranjan-purbey]

Description of change

entityManager.getId currently returns undefined for an entity with composite primary key if primary key columns are also foreign keys with lazy relations, for e.g., in a custom join table. This commit tries to fix that.

The reason for getId returning undefined is that columnMetadata.getEntityValueMap doesn't take into account the fact that the value for lazy-relations is a getter which returns a Promise. This PR adds a check for this condition.

To reproduce the issue, refer https://github.com/ranjan-purbey/typeorm-composite-primary-key-test

Closes: #7736 (maybe)

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 - N/A
• The new commits follow conventions explained in CONTRIBUTING.md

[pleerock]

Please add tests to this change

[ranjan-purbey]

Test added

[pleerock]

Maybe we can simply add check for:

 && !(entity[this.relationMetadata.propertyName] instanceof Promise)

instead of !Object.getOwnPropertyDescriptor(entity, this.relationMetadata.propertyName)?.get ?

I think it will make more clear what is happening this way.

[ranjan-purbey]

Maybe we can simply add check for:

 && !(entity[this.relationMetadata.propertyName] instanceof Promise)

instead of !Object.getOwnPropertyDescriptor(entity, this.relationMetadata.propertyName)?.get ?

I think it will make more clear what is happening this way.

That will unintentionally call the getter function and thus make a database request in case it's a lazy relation

[pleerock]

@ranjan-purbey but how come instanceof check makes a call of a method? It definitely should not.

[ranjan-purbey]

@pleerock because lazy-relations are not normal functions. Instead they are getters. JavaScript getters are called as soon as the property is accessed.

For example, consider the following:

const todos = {
  get items () {
    console.log("Items property was accessed");
    return ["task 1", "task 2"];
  }
};

If we try to check if todos.items instanceof Array, just by accessing the items property the get method is called and thus console.log is invoked

[pleerock]

@ranjan-purbey thanks for the information. If so, we can merge this PR, however I would like to ask to add a bold comment on why this approach was used instead of instanceof Promise check to prevent in the future change of this code to instanceof check. Thank you!

[ranjan-purbey]

Yes I agree that a comment clarifying this will be helpful for future reference. I will add one

[ranjan-purbey]

@pleerock comment added

[pleerock]

Thanks a lot for the contribution!