feat: avoid adding a dummy WHERE clause into UPDATE/DELETE queries - #464
IlyaFaer wants to merge 13 commits into
Conversation
Sorry, something went wrong.
|
This PR is based on changes from the previous PR, and should be merged after merging the previous one. |
Sorry, something went wrong.
c24t
left a comment
There was a problem hiding this comment.
Sorry, something went wrong.
| self._connection.in_transaction(self.__do_execute_update, sql, params) | ||
|
|
||
| def __do_execute_update(self, transaction, sql, params, param_types=None): | ||
| sql = ensure_where_clause(sql) |
There was a problem hiding this comment.
I also realized that it's not very okay to do the check here, as __do_execute_update() will be sent to Database.run_in_transaction(), and then to Session.run_in_transaction() as func arg. This last method will start a transaction (if needed) and only then it'll run __do_execute_update() (and the check). We better run the check before starting anything. The check call moved to line 99
Sorry, something went wrong.
Adding a WHERE clause into user's query doesn't seem to be a good practise in the first place. More than that Spanner requires WHERE clause presence in UPDATE and DELETE queries (see the docs). Adding a dummy WHERE clause silently overrides the original Spanner requirement, and actually leads to what the requirement prevents: accidental updating/deleting every row.
It doesn't look correct, let's raise an error in case of no WHERE clause detected - it'll warn user that the thing he's trying to do can be dangerous. If he's okay with it, he can add WHERE true by his own hands.