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

Implemented relaxed conversion logic for BooleanCodec #290

Merged
merged 10 commits into from
Nov 29, 2024

Conversation

svats0001
Copy link
Contributor

@svats0001 svats0001 commented Oct 21, 2024

@jchrys #285

Motivation:
To allow boolean values stored as strings in MySQL database such as "true", "false", "1" and "0" to be converted to their corresponding boolean values.

Modification:
BooleanCodec: Changed decode method to check if VARCHAR value is a boolean value. Changed doCanDecode to add VARCHAR.
BooleanCodecTest: Added decodeString test to ensure boolean values stored as strings are converted into the correct corresponding boolean values.

Result:
Boolean values stored as strings can now be converted to their corresponding boolean values. Drawbacks are that there could be a string column containing numeric data with values other than 0 or 1 and the column isn't used for storing boolean values at the same time the codec interprets the 0's and 1's as boolean. Only boolean values "true", "false", "1" and "0" are decoded, other possible types of boolean value strings haven't been included. Also, doCanDecode states that the VARCHAR data type can be decoded but only a small subset of this data type can be decoded and it's not possible to highlight the conditions in the doCanDecode method.

@jchrys
Copy link
Collaborator

jchrys commented Oct 28, 2024

Thanks! To closely align with MySQL Connector/J's behavior, please refer to the BooleanValueFactory logic here. Additionally, avoiding fallback on other types' decoding logic can help minimize complexity
If numeric handling is challenging, starting with non-numeric cases could be a good first step.

@svats0001
Copy link
Contributor Author

Hi @jchrys ,
I've altered the method so that it conforms with MySQL Connector/J's behaviour. I'm not sure exactly which type of exception needs to be thrown though (if a new type of exception needs to be created), or if an exception needs to be thrown at all.

@jchrys
Copy link
Collaborator

jchrys commented Nov 25, 2024

@svats0001
It looks great. let me go through it :D

@jchrys
Copy link
Collaborator

jchrys commented Nov 25, 2024

@svats0001
Could you include test cases using scientific notation, such as 1e4 and -1.34e10, in the tests? Also, please include -0. Everything else looks good to me.

@svats0001
Copy link
Contributor Author

@jchrys , I've added those tests and changed the exception now.

Copy link
Collaborator

@jchrys jchrys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
@svats0001 Thanks a lot

@jchrys jchrys added the enhancement New feature or request label Nov 29, 2024
@jchrys jchrys added this to the Next milestone Nov 29, 2024
@jchrys jchrys merged commit 673aac8 into asyncer-io:trunk Nov 29, 2024
28 checks passed
@jchrys jchrys modified the milestones: Next, 1.3.1 Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants