Query Builder onConflict with merge don't generate the correct sql

[Nisgrak]

Describe the bug
Using Query Builder method onConflict with merge don't make the ON DUPLICATE in the resulted SQL.

Also the column/s required in onConflict don't make difference in the ON DUPLICATE, so would be nice to make optional at least.

Code

let data = [
	{
		idProvince: 44,
		name: "Province 1"
	}
	{
		idProvince: 44,
		name: "Province 2"
	}
]

return em.createQueryBuilder(Poblation).insert(data).onConflict("name").merge().getFormattedQuery();

// Ouput:
// insert into `poblations` (`id_province`, `name`) values (44, 'Province 1'), (44, 'Province 2')

To Reproduce
Steps to reproduce the behavior:

1. Get the Query Builder
2. Create an insert, add onConflict and merge
3. See the resulted query

Expected behavior
Output:

insert into `poblations` (`id_province`, `name`) values (44, 'Province 1'), (44, 'Province 2') on duplicate key update `id_province` = values(`id_province`), `name` = values(`name`)

Additional context
https://knexjs.org/#Builder-onConflict
https://mariadb.com/kb/en/insert-on-duplicate-key-update/

Versions

Dependency	Version
node	14.16.1
typescript	4.2.4
mikro-orm	4.5.4
mariadb	2.5.3
@mikro-orm/mariadb	4.5.4

[B4nan]

this feature is postgres exclusive in knex

edit: actually not, the problem is in the empty merge() call without parameter

[Nisgrak]

Merge docs in knew says:
"Uses ON DUPLICATE KEY UPDATE in MySQL"

[Nisgrak]

@B4nan so I need to call merge() with params or it's a bug?

Thanks!

Edit: If I call with params it generates a wrong sql

em.createQueryBuilder(this).insert(data).onConflict("name").merge(["id_province"]).getFormattedQuery();

insert into `poblations` (`id_province`, `name`) values (44, 'Province 1'), (44, 'Province 2') on duplicate key update `0` = 'id_province'

Knex says it generates something like this:

insert into `poblations` (`id_province`, `name`) values (44, 'Province 1'), (44, 'Province 2') on duplicate key update `id_province` = values(`id_province`)

[B4nan]

Didn't know the parameter can be also array, that's actually different problem. It is not even allowed on the TS level, so the example you gave should result in TS error TS2559: Type 'string[]' has no properties in common with type 'EntityData '..

I guess it is working because you are passing the this inside the createQueryBuilder method, which itself looks also weird, it should be entity class, not instance. It's weird if that is working for you (that it compiles), because the entity name should be taken as this.name then, which is quite weird.

[B4nan]

Unfortunately the merge(['name']) with array of keys won't be supported in 4.x, as we can't update knex and this was added in knex 0.95.

https://github.com/knex/knex/blob/master/CHANGELOG.md#0950---03-march-2021

knex 0.95 is breaking as it requires node and TS version bumps, it is already working in v5, so if you feel brave enough, you could give the dev version a try. keep in mind there will be more breaking changes, but so far it was mostly just about the version bumps and internals, not public api.

The fix is available as 4.5.5-dev.12, but as mentioned above, you can either use merge() without params, or with object parameter e.g. merge({ foo: 'bar' }).

[Nisgrak]

In fact I don't wan't to give params to merge, so it's nice! I will try tomorrow.

Thanks for your work!!

[Nisgrak]

@B4nan it works like a charm!

Would be nice to make the field param in onConflict() optional, it don't affect to the output.

em.createQueryBuilder(this).insert(data).onConflict("name").merge().getFormattedQuery();
// Following are empty or non existing fields
em.createQueryBuilder(this).insert(data).onConflict("test").merge().getFormattedQuery();
em.createQueryBuilder(this).insert(data).onConflict("").merge().getFormattedQuery();

The three lines generates the same, the field param doesn't seem to affect (at least in MariaDB):

insert into `poblations` (`id_province`, `name`) values (44, 'Province 1'), (44, 'province 2') on duplicate key update `id_province` = values(`id_province`), `name` = values(`name`)

[B4nan]

I see, I thought the parameter is mandatory in knex, but apparently it is not. Will handle that as part of #1803 so subscribe there for updates.