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

Can't convert Bool to Int32 (TINYINT) #782

Closed
danielgindi opened this issue Mar 22, 2020 · 13 comments
Closed

Can't convert Bool to Int32 (TINYINT) #782

danielgindi opened this issue Mar 22, 2020 · 13 comments

Comments

@danielgindi
Copy link
Contributor

@danielgindi danielgindi commented Mar 22, 2020

With Oracle's MySql.Data, when you read a TINYINT column - you can read it as a bool or as an Int32 - either way works.

And it only makes sense as TINYINT is not explicitly a boolean, it's just so tiny that it can only hold 0 or 1... And it's called TINYINT.

Seems like an easy fix.

It currently throws: Can't convert Bool to Int32
From: MySqlConnector\Core\Row.cs:line 224

For me - this was an unexpected breaking change when trying the transition to this library.

danielgindi added a commit to danielgindi/MySqlConnector that referenced this issue Mar 22, 2020
danielgindi added a commit to danielgindi/MySqlConnector that referenced this issue Mar 22, 2020
…l-net#782)

Signed-off-by: Daniel Cohen Gindi <danielgindi@gmail.com>
@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 23, 2020

And it only makes sense as TINYINT is not explicitly a boolean

No, but TINYINT(1) (note the explicit display width) is an alias for BOOL.

it's just so tiny that it can only hold 0 or 1

It can hold values from -128 to 127. You didn't add any tests for the new code (to compare against Connector/NET), but it seems like this PR could lead to data loss (coercing all non-zero values to 1).

For me - this was an unexpected breaking change when trying the transition to this library.

Can you provide more details about your use case? Why do you have TINYINT(1) columns (not TINYINT) that are storing integral values (but presumably just 0 and 1)?

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 23, 2020

You didn't add any tests for the new code (to compare against Connector/NET), but it seems like this PR could lead to data loss (coercing all non-zero values to 1).

Connector/NET exhibits the same behaviour (changing all non-zero values to 1 when reading as an int).

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 23, 2020

@bgrainger What I implemented will produce the same result as Connector/NET.

As for the use case - does it matter?
If this library is supposed to be a drop in replacement to Connector/NET then it should expose the same behavior, at least for data validity.
The real life scenario is that there is a mature system that works with data in certain manners, and the drop in replacement does not really work due to differences in how MySqlConnector parses data.

Also this happens with Row.GetDecimal - it expects specifically a decimal, where it may be called on any type of numeric field in MySql.Data and cast correctly.

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 23, 2020

And same goes for GetString. If you take a look at System.Data.Sql, you'll see that MySql.Data is not the only one casting anything to anything that matches. Microsoft also did this, and their connector is the role model for all connectors.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 23, 2020

GetString

I'm not sure what you're referring to. Tests for GetString (https://mysql-net.github.io/AdoNetResults/#GetString_throws_for_maximum_Boolean ff) clearly show that SqlClient does not permit conversions from integral types when calling GetString. This library follows that pattern, instead of being bug-compatible with Connector/NET: https://mysqlconnector.net/tutorials/migrating-from-connector-net/#implicit-conversions

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 23, 2020

If this library is supposed to be a drop in replacement to Connector/NET then it should expose the same behavior, at least for data validity.

It is drop-in for many common scenarios, but does deliberately make breaking API changes to fix bugs in (or make improvements to) Connector/NET's behaviour.

As for the use case - does it matter?

Yes; I'd like to understand it better to decide whether it's reasonable (IMO) for a breaking change to occur in this code path or not.

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 23, 2020

Microsoft also did this, and their connector is the role model for all connectors.

Agreed, so I added Boolean->X conversions to my ADO.NET tests. You can walk through the results by searching for for_zero_Boolean on this page: https://mysql-net.github.io/AdoNetResults/#GetInt16_throws_for_zero_Boolean

SqlClient (all versions), npgsql, and MySqlConnector don't permit an implicit conversion from Boolean to int (or any other data type).

Moreover, while GetInt32(0) succeeds in Connector/NET, GetFieldValue<int>(0) throws, so it's not even internally consistent.

The pattern of this library has generally been to follow ADO.NET best practices, not to reproduce every Connector/NET bug; hence I'm still interested in learning more about the use case for this behaviour change.

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 23, 2020

GetString

I'm not sure what you're referring to. Tests for GetString (https://mysql-net.github.io/AdoNetResults/#GetString_throws_for_maximum_Boolean ff) clearly show that SqlClient does not permit conversions from integral types when calling GetString. This library follows that pattern, instead of being bug-compatible with Connector/NET: https://mysqlconnector.net/tutorials/migrating-from-connector-net/#implicit-conversions

Sorry, my mistake. Indeed System.Data does throw for it.

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 23, 2020

Microsoft also did this, and their connector is the role model for all connectors.

Agreed, so I added Boolean->X conversions to my ADO.NET tests. You can walk through the results by searching for for_zero_Boolean on this page: https://mysql-net.github.io/AdoNetResults/#GetInt16_throws_for_zero_Boolean

SqlClient (all versions), npgsql, and MySqlConnector don't permit an implicit conversion from Boolean to int (or any other data type).

Moreover, while GetInt32(0) succeeds in Connector/NET, GetFieldValue<int>(0) throws, so it's not even internally consistent.

The pattern of this library has generally been to follow ADO.NET best practices, not to reproduce every Connector/NET bug; hence I'm still interested in learning more about the use case for this behaviour change.

Okay so I've found instances in the code (our codebase) where a programmer casted from a GetInt32, to a 0/1 enum, to give it more semantic meaning instead of using a boolean in the code.
I've already stumbled on two of those in the code by chance, and I have not way to know if there are more and then in production we will get exceptions because the code expects GetInt32 to work for that case.

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 25, 2020

And it only makes sense as TINYINT is not explicitly a boolean

No, but TINYINT(1) (note the explicit display width) is an alias for BOOL.

I've checked this, and TINYINT(1) is not an alias for BOOL, but the other way around:
https://dev.mysql.com/doc/refman/8.0/en/other-vendor-data-types.html

If someone wanted to store bits, like 1/0 columns, that have no true/false meaning - but just 0/1 - and tried to get them with GetInt (or even GetByte) then he would get an exception, which is unexpected.

Also it's a bit weird to me that you can GetBoolean an integer with a weird cast from anything != 0 to bool, but can't get a TINYINT as an int.

You do not have to agree with me, but these are just some points for consideration :-)

@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 25, 2020

If someone wanted to store bits, like 1/0 columns, that have no true/false meaning - but just 0/1 - and tried to get them with GetInt (or even GetByte) then he would get an exception, which is unexpected.

There are two ways to accomplish this: use BIT(1) or TINYINT (not TINYINT(1), which is reserved as an alias of (not for! 😀) BOOL; this is mentioned in the docs: https://mysqlconnector.net/tutorials/best-practices/#avoid-tinyint-1).

Also it's a bit weird to me that you can GetBoolean an integer with a weird cast from anything != 0 to bool, but can't get a TINYINT as an int.

You can already get a TINYINT as an int. You can't currently get a TINYINT(1) as an int if TreatTinyAsBoolean = true (which is the default).

(I understand it may be confusing that TINYINT is handled differently than TINYINT(1) but it's a necessary complication that many MySQL clients use due to MySQL's lack of a true BOOL data type.)

Thanks for your patience in this discussion; I'm agreeing that it's reasonable (particularly for backwards compatibility with Connector/NET, and symmetry with calling GetBoolean on integer columns) to allow GetIntXX to be called on Boolean columns.

@danielgindi
Copy link
Contributor Author

@danielgindi danielgindi commented Mar 25, 2020

Yes I see the issue with other RDBMSs having a bool type and then it makes absolutely no sense to fetch as an int (unless you are a c++ programmer, or even a VB programmer that expects it to be equal to 0/-1 which is incompatible with anything).

In MySql I feel there are too many aliases. I think that they could have treated BOOL and TINYINT(1) exactly the same, while preserving a meta data mentioning that it’s a BOOL so you can distinguish in certain cases.

And yeah I have a lot of patience, we are locked at home and my girls are climbing on me, so you know either way you leave this event with lots of patience...

bgrainger added a commit that referenced this issue Mar 26, 2020
Allow reading `bool` as integer, like `MySql.Data` allows (Fixes #782)
@bgrainger
Copy link
Member

@bgrainger bgrainger commented Mar 26, 2020

Fixed in 0.63.0.

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

2 participants