Query builder update generates broken SQL

[marano]

Issue type:

[x] bug report

Database system/driver:

[x] postgres

TypeORM version:

[x] latest

const ids = [1]
const query = manager
  .createQueryBuilder()
  .update(Entity)
  .set({ brandId: 2 })
  .where(`"brandId" IN (:...ids)`, { ids })
console.log(query.getSql())

This logs:

query: UPDATE "image_classification" SET WHERE "brandId" IN ($1) -- PARAMETERS: [1]

Expected result:
Generated SQL to include object passed to set method.

[marano]

I've figured this works:

const brand = await Brand.findOne(2)
...
.set({ brand })

But it seems really weird that typeorm won't let me set the id field directly. Also it shouldn't fail silently. Because of that I will let this issue open for further discussion.

[vlapo]

I think you have defined brand relation in your entity and do not have brandId column defined as well. So typeorm escaped unknown columns.

Try add brandId column definition to your entity:

@Column()
brandId: number;

And it should worked as expected.

Also this should work:

.set({ brand: { id: brandId } })

[pleerock]

yeah so we might need to report some error if user passed unknown properties in the object being passed within set method?

[vlapo]

I think it should be good manner to throw error in case final map of values to update is empty. In UpdateQueryBuilder.getValueSet() is small empty check and throw. But later in createUpdateExpression() is this map filtered and possible ends with empty array of values to change.

Also It should be nice to allow update "columns generated by relations" as in this case where brandId is generated by ORM.

[pleerock]

@vlapo I think we shall check both emptiness and wrong properties in.

[vlapo]

Sure I agree. But what about join column not defined in entity as property like brandId in this issue?

Maybe we should allow update brandId even if it is not defined in entity.

Example:

@Entity()
export class Post {

    @PrimaryGeneratedColumn()
    id: number;

    @ManyToOne(type => Author, author => author.posts)
    @JoinColumn()
    author: Author;
}

const query = connection.manager
            .createQueryBuilder()
            .update(Post)
            .set({ authorId: 2 })
            .where({ id: 1 });
console.log(query.getSql());

This should be valid way to update author in post.

[pleerock]

no, I don't think so. This design isn't clear. What is authorId in there? It means we would allow to specify any database column as well? Why only database column and property name? What about other expressions UPDATE SET supports?

In terms of this issue we only need to add those exceptions and close it. If we want to extend update query builder functionality, we need to create a separate issue and describe a new proposal of update query builder that covers all cases.

[vlapo]

Understand. But still I would love to see this case Update relation only with "id" in some FAQ. I will think about it and try improve docs with PR. Just because this isn't first issue and question about this.

For this issue I will create PR and handle emptiness and wrong properties 👍