#873 | implement delete with join

[Egor-Koldasov]

Fix for the issue #873
The issue is that joins are not included in delete queries.
Examples can be found in tests: https://github.com/knex/knex/compare/master...Egor-Koldasov:fix/873-delete-join?expand=1#diff-9a54bb8c91fbb672179c076a18af9db40b73bc5c4a3e3ea7cc89c48ea8ebeadfR9944

Notes on the solution:

1. Solution does not include the ability to delete multiple tables. To me, it is not clear, how the interface should be expanded to include this option. It would be great to have a recommendation from collaborators: Is it impossible to 'delete join'? #873 (comment)
   It does solve other issues, allowing to delete values from a table by filtering values from another joined table.
   It also helps with unexpected queries and potential data loss mentioned here: Is it impossible to 'delete join'? #873 (comment)

2. Postgres, SQLite and Oracle do not support joins on delete. I did not include any custom error throws for them, instead, I allowed the query to build the same way and let the database report the error. Tests for error are included.

3. The project has a linter added to the commit hook. It updated the whole file: https://github.com/knex/knex/compare/master...Egor-Koldasov:fix/873-delete-join?expand=1#diff-9a54bb8c91fbb672179c076a18af9db40b73bc5c4a3e3ea7cc89c48ea8ebeadf
   I'm not sure why previous commits have not done this, since it is automatic.

4. I'm having issues with successfully running 1 test: https://github.com/knex/knex/blob/master/test/integration/query/selects.js#L1361
   The timeout error is not being caught properly leading to a general "Timeout of 5000ms exceeded." error.
   It is not related to this update, but it's better to let know.

Thanks.

[kibertoad]

Do you see downsides of using this approach for PostgreSQL - https://www.postgresqltutorial.com/postgresql-delete-join/?

[Egor-Koldasov]

@kibertoad I did not think about it since it has a different syntax.
The best solution for PostgreSQL I think would be to add direct support for USING syntax like a .using method, it can use the same interface with the current .join.
Overall, internally transforming from .join might be convenient and helpful too. Among downsides, maybe a small confusion from query transformation.

[kibertoad]

Fair. Converting joins to non-joins is likely to be confusing.

[elhigu]

In this case it would seem to be quite trivial crossdb compatibility thing to implement... though, maybe native syntax should be allowed too.

Also tests for error cases where people tries to create .join queries for unsupported databases seem to be missing.

[kibertoad]

@Egor-Koldasov Could you add these in a follow-up PR?

[kibertoad]

@OlivierCavadenti Delete join support with USING syntax for PG at this point is probably the largest gap in functionality still missing for 1.0.0. If you'll have some spare time, any chance you could take a look at it?

[OlivierCavadenti]

@kibertoad yeah I can work on it soon. As I understand, and to be sure, we go on a syntax from :

knex
	.select('xyz')
	.from('my_table')
	.leftJoin('users', 'users.user_id', 'my_table.user_id')
        .where('my_table.criteria', '<', 10)
	.delete();

to in PG (no hidden converting left join to using ?):

knex
	.select('xyz')
	.from('my_table')
	.using('users')
        .where('my_table.criteria', '<', 10)
        .andWhere('users.user_id', 'my_table.user_id')
	.delete(); 

[kibertoad]

Yes, exactly!

[kibertoad]

I'm having issues with successfully running 1 test: https://github.com/knex/knex/blob/master/test/integration/query/selects.js#L1361
The timeout error is not being caught properly leading to a general "Timeout of 5000ms exceeded." error.
It is not related to this update, but it's better to let know.

This is a known flaky test, feel free to ignore.

[kibertoad]

Thanks!

[elhigu]

Is this valid SQL or just generates error from database? If feature is not supported by database, knex should probably throw an error and tell about it or otherwise we will get bug reports about this feature not working.