-
Notifications
You must be signed in to change notification settings - Fork 550
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
Implementing type reflection from mysql result #1068
Conversation
0e68a95
to
e811604
Compare
This looks good! Few smaller comments, and please rebase onto current master. (It appears there are no conflicts, but we'll get confirmation of unit tests.) |
1271210
to
5b9735b
Compare
I've resolved the comments and squash the commits. Please feedback if any change is needed. cc: @sodabrew |
case MYSQL_TYPE_GEOMETRY: // char[] | ||
rb_field_type = rb_str_new_cstr("geometry"); | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand it well it will return nil
for unknown type, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we can force a unit test down the no-match path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know a unknown to test for it, would it be unknown anymore 😆?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway I've made it return 'unknown' for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking into this @sodabrew and I have found out there's no easy way how to introduce custom column type in MySQL :/
But maybe there's some way how to create table including all MySQL data types (by some meta programming) and it would be possible to check if we cover all cases there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will fail if we include MYSQL_TYPE_JSON
though. I cannot find any code to handle MYSQL_TYPE_JSON
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #693 for the history on how JSON is supported; basically it's a fall-through to char[]
.
It seems to me that the type table should be updated to explicitly include JSON so that it's not "accidentally" supported but is explictly routed through the char[]
path of the result handler.
What is the failure you're seeing if MYSQL_TYPE_JSON
is included in this table?
🤔 I was looking for any chance to keep the list of types updated when new type would be introduced (by exposing |
case MYSQL_TYPE_GEOMETRY: // char[] | ||
rb_field_type = rb_str_new_cstr("geometry"); | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we can force a unit test down the no-match path?
957b0ac
to
17297cb
Compare
Seems like |
The JSON type should be supported, what error are you seeing? |
This is actually not an error on our side but on mysql's json support. I'm using mariadb on my local machine and they don't support
|
Not sure if we should add something like this for
or just declare it if not exist
I'm inclined to do latter since it doesn't require user to install the correct header version on the client side. |
I was going to say we should do the first one, but you're right that some clients may have been compiled with an older library, and the column type number is should always be the same for compatibility reasons. |
41fcc21
to
371e120
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for updating your PR along with all the review comments.
Add
Mysql2::Result#field_types
method to get field types from result.Resolve: #1067