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

Affected rows count is inconsistent with other database drivers #600

Closed
Grauenwolf opened this issue Jan 2, 2019 · 16 comments
Closed

Affected rows count is inconsistent with other database drivers #600

Grauenwolf opened this issue Jan 2, 2019 · 16 comments

Comments

@Grauenwolf
Copy link

@Grauenwolf Grauenwolf commented Jan 2, 2019

I'm working on an ORM. And one of the things it does is check the affected rows count after performing an update using ExecuteAsNonQuery(). If the count isn't 1, it throws an error saying the row couldn't be found.

Currently I've tested this design with

  • SQL Sever (SqlClient and OleDB)
  • PostgreSQL (Npgsql)
  • SQLite (System.Data.SQLite and SQLitePCLRaw)
  • Access (OleDB)
  • MySQL (MySqlConnector and Oracle's driver)

So far MySqlConnector is the only one that doesn't return 1 when the record was found, but no changes were made.


If I add "UseAffectedRows=false" to the connection string, it will work. But this means modifying the connection string I'm given, which would be weird from the user's perspective.

I don't know what the right answer if for MySqlConnector, but I felt that I should bring the inconsistency to your attention.

@caleblloyd
Copy link
Contributor

@caleblloyd caleblloyd commented Jan 2, 2019

Pomelo EF Core modifies the connection string to work around this very issue.

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/master/src/EFCore.MySql/Extensions/MySqlDbContextOptionsExtensions.cs#L29-L30

History for the reason it is defaulted to true in the driver is in issue #104

@Grauenwolf
Copy link
Author

@Grauenwolf Grauenwolf commented Jan 2, 2019

Well if they're not the only one doing it, I don't feel so bad.

@Grauenwolf
Copy link
Author

@Grauenwolf Grauenwolf commented Jan 2, 2019

Is there a way to determine from the MySQLConnection object which mode you are in?

Or better yet, a way to override the behavior of a particular MySqlCommand object for the duration of a single query?

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 2, 2019

Is there a way to determine from the MySQLConnection object which mode you are in?

No, but an API could be added for this. Edit (see below): The MySqlConnection.ConnectionString property can be parsed with MySqlConnectionStringBuilder to determine what the setting is.

Or better yet, a way to override the behavior of a particular MySqlCommand object for the duration of a single query?

No, this is not possible, because the flag is set in the initial handshake with the server and can't be changed for the lifetime of the connection.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 3, 2019

The behaviour of the MySQL engine itself appears to be by design:

For UPDATE statements, the affected-rows value by default is the number of rows actually changed.

I'm a little surprised to hear that all other database engines count a "no-op update" as an affected row.

@caleblloyd
Copy link
Contributor

@caleblloyd caleblloyd commented Jan 3, 2019

Is there a way to determine from the MySQLConnection object which mode you are in?

Further down in the Pomelo file it does this by parsing the connection string associated with the connection. If the connection has not been opened yet then it modifies the connection string associated with the connection.

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/master/src/EFCore.MySql/Extensions/MySqlDbContextOptionsExtensions.cs#L50-L67

@Grauenwolf
Copy link
Author

@Grauenwolf Grauenwolf commented Jan 3, 2019

I'm a little surprised to hear that all other database engines count a "no-op update" as an affected row.

That's how SQL Server works. And since it was the first ADO.NET database driver, I bet everyone else just followed the same pattern.

Further down in the Pomelo file it does this by parsing the connection string associated with the connection.

That's not a perfect solution for me, but it will cover most of my use cases so I'm ok with closing this ticket if no one objects.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 11, 2019

The primary reasons for defaulting to UseAffectedRows=true were described here: #104 (comment)

I was also trying to follow the [ExecuteNonQuery documentation](https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbcommand.executenonquery?view=netframework-4.7.2 which says it returns "The number of rows affected."

However, "affected" in that documentation could either be interpreted strictly (as MySQL Server does, i.e., rows that are changed to different values), or loosely (as all other databases do, i.e., rows that are matched by the WHERE clause and are candidates for being updated). It doesn't strongly imply the MySQL interpretation.

Since making that decision, we've learnt:

  • Pomelo has to work around the default.
  • @Grauenwolf's ORM would have to work around the default.
  • Multiple issues (#403, #552) have been filed by people not expecting this behaviour change when switching client libraries.

It seems like UseAffectedRows=false may be the better default for compatibility both with MySql.Data and other ADO.NET libraries.

This is obviously a breaking change (users would have to review any ExecuteNonQuery of an UPDATE to make sure the return value is used correctly, or just change all their connection strings), but better to do it sooner rather than later (e.g., after 1.0).

@caleblloyd you helped picked the default; any thoughts on this change?

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 11, 2019

I added a test (to AdoNetApiTest) to check the return value of ExecuteNonQuery for UPDATE: mysql-net/AdoNetApiTest@eae7f1a

Only MySqlConnector and dotConnect failed:

image

This reinforces the suggestion that it would be best to match the behaviour of other ADO.NET libraries.

@caleblloyd
Copy link
Contributor

@caleblloyd caleblloyd commented Jan 11, 2019

It's a tough call. UseAffectedRows=true is in line with most other language's MySQL drivers when I researched them in #104 (comment)

UseAffectedRows=false is in line with other ADO.NET imolementations. I am curious, do the other DBMS' have the notion of Found vs. Affected rows?

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 11, 2019

Compatibility with other languages' drivers' defaults is (much) less important to me. The scenarios I'm more concerned about are:

  • Migrate from Connector/NET to MySqlConnector: currently different, would be identical behaviour with this change
  • Change from SQL Server (w/ SqlClient) to MySQL (w/ MySqlConnector): ExecuteNonQuery currently returns different values, but would return the same values with this change
  • Upgrade from MySqlConnector 0.48.2 to vNext: breaking change, but how big an impact is it?

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 11, 2019

I am curious, do the other DBMS' have the notion of Found vs. Affected rows?

It appears to be unique to MySQL AFAICT.

Most people seem to recommend changing the query to UPDATE table SET col = newVal WHERE (other conditions) AND col <> newVal if you need to find the number of rows that are actually changed to a new value.

This workaround lets you get either "found rows" (default) or "affected rows" (add the extra predicate) behaviour with UseAffectedRows=false, i.e., you can switch on a query-by-query (instead of connection-by-connection) basis (assuming the UPDATE query can be modified in that way).

@caleblloyd
Copy link
Contributor

@caleblloyd caleblloyd commented Jan 11, 2019

Changing it to UseAffectedRows=false seems like a good idea then, to improve ADO.NET and MySQL.Data compatibility.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Jan 12, 2019

Default changed in 0.49.0.

@Grauenwolf
Copy link
Author

@Grauenwolf Grauenwolf commented Jan 12, 2019

Thank you for taking a look at this. Even if the decision hadn't gone my way, I would still respect you all for the careful thought you all put into the question.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Aug 16, 2019

Here's a Connector/NET user who would have wanted the default to be true (possibly for compatibility with non-.NET tools): https://bugs.mysql.com/bug.php?id=96572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants