weiznich / diesel_async

Diesel async connection implementation
Apache License 2.0
659 stars 82 forks source link

Incoherence between Diesel and Diesel_async on MySQL/MariaDB connection (UPDATE return value) #138

Closed Soblow closed 8 months ago

Soblow commented 9 months ago

Setup

Versions

Feature Flags

Problem Description

I'm sorry in advance if this issue is badly reported, I'm a bit new to this...

Note: In this issue, MariaDB is used to describe both MariaDB & MySQL, as I think both are affected, but I only tested MariaDB.

In current Diesel version, a MySQL/MariaDB connection will see a flag activated to change the way UPDATE operations work:

https://github.com/diesel-rs/diesel/blob/ad8ef479e1c6f926b940ad9a43877fc5a993ec5f/diesel/src/mysql/connection/url.rs#L134

        // this is not present in the database_url, using a default value
        let client_flags = CapabilityFlags::CLIENT_FOUND_ROWS;

On PostgreSQL, UPDATE operations will return the number of lines matched by the operation, even if the values set are identical to the ones already present in a row. On MariaDB/MySQL, only the number of changed rows is reported by default. (Relevant StackOverflow answer: https://stackoverflow.com/a/2186952; Extract from MariaDB manual: https://dev.mysql.com/doc/c-api/8.3/en/mysql-affected-rows.html )

On Diesel, to get a consistent behavior between both engines, it seems like the flag to enable reporting of matching rows rather than modified rows is set on by default.

As Diesel-async seems to use an other way to parse the database URL, the same option isn't set by default and must be specified in the URL. If we (as I got helped by someone to read diesel-async codebase) got the code right, diesel-async uses mysql-async, which supports setting this option: https://docs.rs/mysql_async/0.32.2/mysql_async/struct.Opts.html#method.client_found_rows

What are you trying to accomplish?

I expected that the option would also be set in diesel-async to get a coherent and consistent behavior when switching between Diesel and Diesel-async. In my case, I had tests which relied on reporting affected rows, which had to be worked-around when implementing support for MariaDB.

It would be nice for either diesel-async to also enable that setting by default, or for diesel to actually implement similar parsing of URLs to allow enabling/disabling the option using the URL.

What is the expected output?

UPDATE commands should either return changed rows count, or matching rows count, similarly regardless of if you use diesel or diesel-async

What is the actual output?

UPDATE commands only report changed rows in diesel-async, while it reports matching rows in diesel

Are you seeing any additional errors?

No

Steps to reproduce

I don't have a MWE for now, but it shouldn't be too hard to produce one if needed

  1. Create a MariaDB database, with matching DB & user
  2. Create a basic table with a key and a value (like an integer) (for example, id: u32 and val: u64)
  3. Insert a value to this database ( for example, id=1 and val=42 )
  4. Update the values to the same value ( so here, update for id=1 values val=42 )
  5. The update command will return "1 row affected" on Diesel, and "0 row affected" on Diesel-Async
  6. [x] This issue can be reproduced on Rust's stable channel. (Your issue will be closed if this is not the case)
  7. [x] This issue can be reproduced without requiring a third party crate
weiznich commented 9 months ago

Thanks for taking the time to report this issue. That's really appricated.

You are right that we currently don't set the CLIENT_FOUND_ROWS flag.

Fixing that should be as easy as changing this location:

https://github.com/weiznich/diesel_async/blob/1ef43d2a8a4080c1e9e5c34056f4d26cbb2a85cc/src/mysql/mod.rs#L58

to call that function as well: https://docs.rs/mysql_async/latest/mysql_async/struct.OptsBuilder.html#method.client_found_rows

(+ Having an entry in our Changelog.md file for that change, so it's documented that this behavior changes with the next release.)

I would appropriate a PR for that.