Added support for MariaDb extend type info #288
Conversation
r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/message/server/DefinitionMetadataMessage.java
Outdated
Show resolved
Hide resolved
r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/message/server/DefinitionMetadataMessage.java
Outdated
Show resolved
Hide resolved
r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlTypeMetadata.java
Outdated
Show resolved
Hide resolved
r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlTypeMetadata.java
Outdated
Show resolved
Hide resolved
r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlTypeMetadata.java
Outdated
Show resolved
Hide resolved
r2dbc-mysql/src/test/java/io/asyncer/r2dbc/mysql/MySqlTypeMetadataTest.java
Show resolved
Hide resolved
|
Hi @jchrys , I appreciate the feedback and I'll incorporate those changes in a new commit as well as the checkstyle changes. I'm having some temporary issues running checkstyle in my Eclipse IDE so as soon as I figure that out I'll resubmit the changes in some time. |
|
Hi, @svats0001. |
|
Hi @jchrys , |
There was a problem hiding this comment.
@svats0001
Thanks for all your hard work! I was doing a final check, and I encountered an error when I enabled it. Could you take a look?
| /** | ||
| * Receive extended column type information from MariaDB to find out more specific details about column type. | ||
| */ | ||
| private static final long MARIADB_CLIENT_EXTENDED_TYPE_INFO = 1L << 35; | ||
| // private static final long MARIADB_CLIENT_CACHE_METADATA = 1L << 36; | ||
|
|
||
| private static final long ALL_SUPPORTED = CLIENT_MYSQL | FOUND_ROWS | LONG_FLAG | CONNECT_WITH_DB | |
There was a problem hiding this comment.
When I add the MARIADB_CLIENT_EXTENDED_TYPE_INFO capability to ALL_SUPPORTED, I get an error. Could you check this out?
There was a problem hiding this comment.
private static final long ALL_SUPPORTED = CLIENT_MYSQL | FOUND_ROWS | LONG_FLAG | CONNECT_WITH_DB |
NO_SCHEMA | COMPRESS | LOCAL_FILES | IGNORE_SPACE | PROTOCOL_41 |
INTERACTIVE | SSL |
TRANSACTIONS | SECURE_SALT | MULTI_STATEMENTS | MULTI_RESULTS |
PS_MULTI_RESULTS |
PLUGIN_AUTH | CONNECT_ATTRS | VAR_INT_SIZED_AUTH | SESSION_TRACK |
DEPRECATE_EOF | ZSTD_COMPRESS | MARIADB_CLIENT_EXTENDED_TYPE_INFOThere was a problem hiding this comment.
When I was testing it the MariaDB versions >= 10.5 didn't seem to return the extended_type_info capability which is what confused me as I manually enabled these capabilities in the test and it didn't work because the packet didn't contain the extended information. In this case the capability is enabled by default but the packet doesn't contain the extended information so if anything you shouldn't include the extended_type_info capability in ALL_SUPPORTED and instead rely on the server returning the capability if enabled. But then the capability should have been turned off if not enabled by the server should it not..
There was a problem hiding this comment.
Oh, I understand. In my setup(and github), the capability variable represents the server-side capability, which is provided directly by the server. This capability includes extended_type_info, but the capability.extendMariaDb line here removes this flag due to the influence of ALL_SUPPORTED.
There was a problem hiding this comment.
@svats0001 Could you try using MariaDB version 11.5.2? Alternatively, you could run the test with the VM options -Dtest.db.type=mariadb -Dtest.db.version=11.5.2. The official Docker image supports the flag by default.
There was a problem hiding this comment.
Oh I overlooked that fact about ALL_SUPPORTED, that makes sense.
I've tried with version 11.5.2 as well and it's not working either. The fact that all the tests run fine without trying to read extended_type_info suggests that the column definition packets don't contain this information. In the docs for extended_type_info, it does say 'while string has data' which may mean that it only returns the data for specific columns, so I'll play around with that some more by testing conditionally on the value of the first byte. Also, even versions < 10.5 return the extended_type_info flag because the tests fail for these versions too which is weird.
There was a problem hiding this comment.
It looks like I'm not able to progress this issue any further, I'll close this pull request if that's okay @jchrys
There was a problem hiding this comment.
Would it be possible to keep this PR open so that I can continue working on it when I have some availability? I believe this work is valuable and deserves to be delivered.
There was a problem hiding this comment.
Yep no problem, I'll leave it open.
Hi @mirromutth ,
#254
Motivation:
If Capability.MARIADB_CLIENT_EXTENDED_TYPE_INFO is set, the Column Defintion Packet will contain extended type information. MariaDb returns JSON data as VARCHAR (MYSQL_TYPE_STRING) so reading the extended type information can be useful to identify the correct MySqlType.
Modification:
Capability: Uncommented MARIADB_CLIENT_EXTENDED_TYPE_INFO and added method that checks if this capability is enabled in bitmap.
MySqlColumnDescriptor: Altered constructor parameter to include nullable extendedTypeInfo field that is used to create MySqlTypeMetadata.
MySqlTypeMetadata: Added nullable extendedTypeInfo field and added isMariaDbJson method to check if the value of extendedTypeInfo equals 'json'. Included extendedTypeInfo in equals, hashcode and toString methods.
MySqlNativeTypeMetadata: Added isMariaDbJson method to check if the value of extendedTypeInfo equals 'json'.
MySqlType: In the MySqlType.of method, if type_id is ID_VARCHAR, ID_VAR_STRING or ID_STRING, returns VARBINARY if data is binary and if not returns JSON if isMariaDbJson() is true, otherwise returns VARCHAR.
DefinitionMetadataMessage: Added nullable extendedTypeInfo field. Added getter for this field and included it in hashcode, equals and toString methods. In decode41 method, if isMariaDb and isExtendedTypeInfo capabilities are enabled, added extendTypeInfo field that reads extended type information from buf.
MySqlRowDescriptorTest: Added null in MySqlColumnDescriptor argument to accommodate changes made to MySqlColumnDescriptor.
MySqlTypeMetadataTest: Added test method that checks if metadata isMariaDbJson is true/false depending on the value of extendedTypeInfo. Also checks if MYSQL_TYPE_STRING returns correct MySqlType (JSON/VARCHAR) depending on if extendedTypeInfo is 'json' or not.
Result:
If extended type information capability is enabled for MariaDb and a MariaDb JSON column is returned, the column should have a MySqlType of JSON rather than VARCHAR.
The drawbacks are that the extendedTypeInfo field has been added to several classes even though it's often null, which is always the case when using MySql and often the case when using MariaDb (only not null when extended type info capability is enabled and using protocol 4.1). Also even when it's not null, it only affects MySqlType when the value is 'json'.
Added a new test for MySqlTypeMetadata.