future find* methods breaking changes

[pleerock]

In the next major version we plan to change current find* methods signature and implementation:

• currently find* accept very mixed arguments: it can accept both FindOptions and FindOptionsWhere, e.g. find({ id: 1 }) and find({ where: { id: 1 }}). There are two problems with this approach:

  • it introduces a tricky code in our codebase where we differentiate them based on assumption
  • we loose type hinting and a bit of type-safety when using them

  We plan to change find* signature to only accept FindOptions. It means find({ id: 1 }) won't be valid anymore. Although we loose in code beauty, we'll have a more reliable execution and better type-hinting. In return to this loose we are considering to introduce a new syntax sugar: find(Where({ id: 1 })

• currently find* accept very mixed arguments: it can accept "id", but we don't know entity id type in repository/manager, that's why we accept any type - boolean, number, string, e.g. - this approach isn't type-safe at all. This signature will be removed, instead find({ where: { id: 1 }}) should be used.

• findOne, findAndCount, findOneOrFail, findByIds will return Promise<T based on FindOptions<Selection, Relations>>> instead of Promise<Entity>. We are considering about introducing a new repository type for this purpose, in order to have ability to use find methods that return simple types.

• if findOne didn't find any rows in database, it is logically makes more sense to return null instead of undefined. Currently we return undefined and we plan to change return value to null.

• find(undefined) or find(null) or find() signatures are no more valid. Such usages are eccentric and don't make much sense.

All changes are breaking.

[nebkat]

currently find* accept very mixed arguments: it can accept "id", but we don't know entity id type in repository/manager, that's why we accept any type - boolean, number, string, e.g. - this approach isn't type-safe at all. This signature will be removed, instead find({ where: { id: 1 }}) should be used.

I think there is a side effect from previously supporting this in relations that could potentially now be removed. When @ManyToOne or an empty @JoinColumn() is used, the virtual join column that is generated for the relation has it's property name set to match the relation name:

typeorm/src/metadata-builder/RelationJoinColumnBuilder.ts

Line 145 in 2fa6231

propertyName: relation.propertyName,

For example:

@Entity()
export class Address {
    @PrimaryGeneratedColumn() id: number;
}

@Entity()
export class Contact {
    @PrimaryGeneratedColumn() id: number;
    @ManyToOne(() => Address) address: Address;
}

When we look at EntityMetadata.columns for Contact, it contains a "virtual" column with propertyPath: "address.id", databaseName: "addressId", but, incorrectly, propertyName: "address", which leads to some unusual effects. If we had manually defined an addressId column the property name would be correct, because matching is done based on the database name.

As far as I can tell this is like this so that contacts.update({ id: 1 }, { address: 2 }) or contacts.find({ address: 2 }) can be used to point to a particular address ID, but obviously this has the same type safety issues you described.

Could we remove this functionality and expect that if users want to point to particular relations by ID they must either specify the full ID map, or they must define a dedicated addressId column that they can still use the ID with? This would simply involve setting the propertyName assignment above to be the correct combination of relation name and ID column property name.