Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Fix: execute() should return rows affected by operation if lastrowid is not set.#150

Merged
gvbgduh merged 2 commits intoencode:masterfrom
gnat:execute-return-rows-affected-mysql-sqlite
Mar 16, 2020
Merged

Fix: execute() should return rows affected by operation if lastrowid is not set.#150
gvbgduh merged 2 commits intoencode:masterfrom
gnat:execute-return-rows-affected-mysql-sqlite

Conversation

@gnat
Copy link
Copy Markdown
Contributor

@gnat gnat commented Oct 21, 2019

Patch closes the following issues:

Old behavior: Calls to execute() return lastrowid. For some operations this has no benefit.

New behavior: Calls to execute() where lastrowid has no benefit will now return rows affected by the operation. (Example: DELETE. So we can now see if a DELETE was successful, and how many rows were deleted). Calls which set a lastrowid value, will still retain the old behavior.

The library encode/databases depends on, aiomysql, returns rows affected, the desired behaviour this patch implements. See: https://github.com/aio-libs/aiomysql/blob/master/aiomysql/cursors.py#L244

@gnat gnat changed the title Fix: execute() should return rows affected by operation. Fix: execute() should return rows affected by operation if lastrowid is not set. Oct 21, 2019
@gnat
Copy link
Copy Markdown
Contributor Author

gnat commented Dec 10, 2019

@tomchristie Possible to get this merged?

@gvbgduh
Copy link
Copy Markdown
Contributor

gvbgduh commented Dec 11, 2019

Oh, nice! That makes sense. That would also apply for the upcoming aiopg backend.
For asyncpg it'll be a bit diff tho, as it returns exactly what postgres returns back, which is of the following form:

for insert

INSERT oid count

for update:

UPDATE count

for delete:

DELETE count

As a plain string, so simple parsing should. Would be very nice to add it to this PR as well, but I can push it a bit later with a different issue to not forget to update aiopg when it's merged.

@gnat
Copy link
Copy Markdown
Contributor Author

gnat commented Dec 20, 2019

@gvbgduh That sounds like a great plan. In the mean time, is there anything holding up getting this merged?

@gvbgduh
Copy link
Copy Markdown
Contributor

gvbgduh commented Mar 16, 2020

@gnat sorry for a delay, merging it now

@gnat
Copy link
Copy Markdown
Contributor Author

gnat commented Mar 16, 2020

Amazing, thank you @gvbgduh !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants