Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include affected rows in logging even when query is done #1754

Merged
merged 4 commits into from Apr 8, 2022

Conversation

david-mcgillicuddy-moixa
Copy link
Contributor

Closes #1570 (or at least will do once it's finished)

This is just a quick PoC, only affecting sqlite for now, but I'd be happy to flesh this out if you think this is a reasonable approach.

Since this is hard to test I made a little demo app here to explore how this interacts with, in particular, transactions and COMMIT statements: https://github.com/david-mcgillicuddy-moixa/sqlx-log-test

@abonander
Copy link
Collaborator

This looks good! A couple notes:

While the choice was previously made to use usize here, it doesn't really make sense since this stuff isn't tied to vector lengths or anything like that. I think u64 makes more sense (and works better with SqliteQueryResult::rows_affected()) if you don't mind doing that refactor.

Are you still interested in covering the other databases?

@david-mcgillicuddy-moixa
Copy link
Contributor Author

Great. No problem to switch to u64, and I'll do the other DBs, assuming the approach the same as sqlite change. I reserve the right to bail if there's some horrifying fountain of complexity of course, but that seems pretty unlikely to me.

@david-mcgillicuddy-moixa
Copy link
Contributor Author

Done, sqlite is tested on the same sqlite repo as previously, don't really have the setup to manually test the other DBs. All the different db feature flags still compile, at least.
Also noticed a small bug with a few warnings

warning: panic message contains an unused formatting placeholder
   --> sqlx-core/src/mssql/protocol/type_info.rs:539:49
    |
539 |                 _ => unreachable!("invalid size {} for int"),
    |                                                 ^^
    |
    = note: this message is not used as a format string when given without arguments, but will be in Rust 2021
help: add the missing argument
    |
539 |                 _ => unreachable!("invalid size {} for int", ...),
    |                                                            +++++
help: or add a "{}" format string to use the message literally
    |
539 |                 _ => unreachable!("{}", "invalid size {} for int"),
    |                                   +++++

so fixed that while I was here.

There was also a slight inconsistency between the naming choice of changes, rows_affected, and affected_rows but whatever.

@abonander
Copy link
Collaborator

There was also a slight inconsistency between the naming choice of changes, rows_affected, and affected_rows but whatever.

I think that's just the terminology of the respective databases. Nothing's perfectly consistent in the SQL world.

@abonander abonander merged commit 6efc39f into launchbadge:master Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging always reports rows: 0 for inserts
2 participants