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

Added support for MariaDb extend type info #288

Open
wants to merge 15 commits into
base: trunk
Choose a base branch
from
15 changes: 14 additions & 1 deletion r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/Capability.java
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,11 @@ public final class Capability {
// private static final long MARIADB_CLIENT_PROGRESS = 1L << 32;
// private static final long MARIADB_CLIENT_COM_MULTI = 1L << 33;
// private static final long MARIADB_CLIENT_STMT_BULK_OPERATIONS = 1L << 34;
// private static final long MARIADB_CLIENT_EXTENDED_TYPE_INFO = 1L << 35;

/**
* 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 |
Copy link
Collaborator

@jchrys jchrys Oct 11, 2024

Choose a reason for hiding this comment

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

When I add the MARIADB_CLIENT_EXTENDED_TYPE_INFO capability to ALL_SUPPORTED, I get an error. Could you check this out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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_INFO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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..

Copy link
Collaborator

@jchrys jchrys Oct 14, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@jchrys jchrys Oct 14, 2024

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like I'm not able to progress this issue any further, I'll close this pull request if that's okay @jchrys

Copy link
Collaborator

@jchrys jchrys Oct 16, 2024

Choose a reason for hiding this comment

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

@svats0001

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep no problem, I'll leave it open.

Expand Down Expand Up @@ -309,6 +313,15 @@ public boolean isZlibCompression() {
public boolean isZstdCompression() {
return (bitmap & ZSTD_COMPRESS) != 0;
}

/**
* Checks if MariaDB extended type info enabled.
*
* @return if MariaDB extended type info enabled.
*/
public boolean isExtendedTypeInfo() {
return (bitmap & MARIADB_CLIENT_EXTENDED_TYPE_INFO) != 0;
}

/**
* Extends MariaDB capabilities.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import io.asyncer.r2dbc.mysql.constant.MySqlType;
import io.asyncer.r2dbc.mysql.message.server.DefinitionMetadataMessage;
import io.r2dbc.spi.Nullability;

import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;

import static io.asyncer.r2dbc.mysql.internal.util.AssertUtils.require;
Expand Down Expand Up @@ -53,13 +55,13 @@ final class MySqlColumnDescriptor implements MySqlColumnMetadata {

@VisibleForTesting
MySqlColumnDescriptor(int index, short typeId, String name, int definitions,
long size, int decimals, int collationId) {
long size, int decimals, int collationId, @Nullable String extendedTypeInfo) {
require(index >= 0, "index must not be a negative integer");
require(size >= 0, "size must not be a negative integer");
require(decimals >= 0, "decimals must not be a negative integer");
requireNonNull(name, "name must not be null");

MySqlTypeMetadata typeMetadata = new MySqlTypeMetadata(typeId, definitions, collationId);
MySqlTypeMetadata typeMetadata = new MySqlTypeMetadata(typeId, definitions, collationId, extendedTypeInfo);

this.index = index;
this.typeMetadata = typeMetadata;
Expand All @@ -74,7 +76,7 @@ final class MySqlColumnDescriptor implements MySqlColumnMetadata {
static MySqlColumnDescriptor create(int index, DefinitionMetadataMessage message) {
int definitions = message.getDefinitions();
return new MySqlColumnDescriptor(index, message.getTypeId(), message.getColumn(), definitions,
message.getSize(), message.getDecimals(), message.getCollationId());
message.getSize(), message.getDecimals(), message.getCollationId(), message.getExtendedTypeInfo());
}

int getIndex() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package io.asyncer.r2dbc.mysql;

import java.util.Objects;

import org.jetbrains.annotations.Nullable;

import io.asyncer.r2dbc.mysql.api.MySqlNativeTypeMetadata;
import io.asyncer.r2dbc.mysql.collation.CharCollation;

Expand Down Expand Up @@ -64,11 +68,18 @@ final class MySqlTypeMetadata implements MySqlNativeTypeMetadata {
* collationId > 0 when protocol version == 4.1, 0 otherwise.
*/
private final int collationId;

/**
* The MariaDB extended type info field that provides more specific details about column type.
*/
@Nullable
private final String extendedTypeInfo;

MySqlTypeMetadata(int typeId, int definitions, int collationId) {
MySqlTypeMetadata(int typeId, int definitions, int collationId, @Nullable String extendedTypeInfo) {
this.typeId = typeId;
this.definitions = (short) (definitions & ALL_USED);
this.collationId = collationId;
this.extendedTypeInfo = extendedTypeInfo;
}

@Override
Expand Down Expand Up @@ -105,6 +116,11 @@ public boolean isEnum() {
public boolean isSet() {
return (definitions & SET) != 0;
}

@Override
public boolean isMariaDbJson() {
return extendedTypeInfo.equals("json");
svats0001 marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public boolean equals(Object o) {
Expand All @@ -117,20 +133,22 @@ public boolean equals(Object o) {

MySqlTypeMetadata that = (MySqlTypeMetadata) o;

return typeId == that.typeId && definitions == that.definitions && collationId == that.collationId;
return typeId == that.typeId && definitions == that.definitions && collationId == that.collationId &&
Objects.equals(extendedTypeInfo, that.extendedTypeInfo);
}

@Override
public int hashCode() {
int result = 31 * typeId + (int) definitions;
return 31 * result + collationId;
return 31 * result + collationId + extendedTypeInfo.hashCode();
svats0001 marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public String toString() {
return "MySqlTypeMetadata{typeId=" + typeId +
", definitions=0x" + Integer.toHexString(definitions) +
", collationId=" + collationId +
", collationId=" + collationId +
", extendedTypeInfo=" + extendedTypeInfo +
svats0001 marked this conversation as resolved.
Show resolved Hide resolved
'}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,11 @@ public interface MySqlNativeTypeMetadata {
* @return if value is a set
*/
boolean isSet();

/**
* Checks if value is JSON for MariaDb.
*
* @return if value is a JSON for MariaDb
*/
boolean isMariaDbJson();
}
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ public static MySqlType of(MySqlNativeTypeMetadata metadata) {
case ID_VARCHAR:
case ID_VAR_STRING:
case ID_STRING:
return metadata.isBinary() ? VARBINARY : VARCHAR;
return metadata.isBinary() ? VARBINARY : (metadata.isMariaDbJson() ? JSON : VARCHAR);
case ID_BIT:
return BIT;
case ID_JSON:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ public final class DefinitionMetadataMessage implements ServerMessage {

@Nullable
private final String originColumn;

@Nullable
private final String extendedTypeInfo;

private final int collationId;

Expand All @@ -57,7 +60,7 @@ public final class DefinitionMetadataMessage implements ServerMessage {
private final short decimals;

private DefinitionMetadataMessage(@Nullable String database, String table, @Nullable String originTable,
String column, @Nullable String originColumn, int collationId, long size, short typeId,
String column, @Nullable String originColumn, @Nullable String extendedTypeInfo, int collationId, long size, short typeId,
int definitions, short decimals) {
require(size >= 0, "size must not be a negative integer");

Expand All @@ -66,6 +69,7 @@ private DefinitionMetadataMessage(@Nullable String database, String table, @Null
this.originTable = originTable;
this.column = requireNonNull(column, "column must not be null");
this.originColumn = originColumn;
this.extendedTypeInfo = extendedTypeInfo;
this.collationId = collationId;
this.size = size;
this.typeId = typeId;
Expand Down Expand Up @@ -96,6 +100,10 @@ public int getDefinitions() {
public short getDecimals() {
return decimals;
}

svats0001 marked this conversation as resolved.
Show resolved Hide resolved
public String getExtendedTypeInfo() {
return extendedTypeInfo;
}

@Override
public boolean equals(Object o) {
Expand All @@ -115,21 +123,22 @@ public boolean equals(Object o) {
table.equals(that.table) &&
Objects.equals(originTable, that.originTable) &&
column.equals(that.column) &&
Objects.equals(originColumn, that.originColumn);
Objects.equals(originColumn, that.originColumn) &&
Objects.equals(extendedTypeInfo, that.extendedTypeInfo);
}

@Override
public int hashCode() {
return Objects.hash(database, table, originTable, column, originColumn, collationId, size, typeId,
definitions, decimals);
definitions, decimals, extendedTypeInfo);
}

@Override
public String toString() {
return "DefinitionMetadataMessage{database='" + database + "', table='" + table + "' (origin:'" +
originTable + "'), column='" + column + "' (origin:'" + originColumn + "'), collationId=" +
collationId + ", size=" + size + ", type=" + typeId + ", definitions=" + definitions +
", decimals=" + decimals + '}';
originTable + "'), column='" + column + "' (origin:'" + originColumn + "'), extendedTypeInfo=" +
extendedTypeInfo + ", collationId=" +collationId + ", size=" + size + ", type=" + typeId +
svats0001 marked this conversation as resolved.
Show resolved Hide resolved
", definitions=" + definitions + ", decimals=" + decimals + '}';
}

static DefinitionMetadataMessage decode(ByteBuf buf, ConnectionContext context) {
Expand All @@ -156,7 +165,7 @@ private static DefinitionMetadataMessage decode320(ByteBuf buf, ConnectionContex
int definitions = buf.readUnsignedShortLE();
short decimals = buf.readUnsignedByte();

return new DefinitionMetadataMessage(null, table, null, column, null, 0, size, typeId,
return new DefinitionMetadataMessage(null, table, null, column, null, null, 0, size, typeId,
definitions, decimals);
}

Expand All @@ -170,6 +179,11 @@ private static DefinitionMetadataMessage decode41(ByteBuf buf, ConnectionContext
String originTable = readVarIntSizedString(buf, charset);
String column = readVarIntSizedString(buf, charset);
String originColumn = readVarIntSizedString(buf, charset);

String extendTypeInfo = null;
if (context.getCapability().isMariaDb() && context.getCapability().isExtendedTypeInfo()) {
extendTypeInfo = readVarIntSizedString(buf, charset);
}

// Skip constant 0x0c encoded by var integer
VarIntUtils.readVarInt(buf);
Expand All @@ -179,7 +193,7 @@ private static DefinitionMetadataMessage decode41(ByteBuf buf, ConnectionContext
short typeId = buf.readUnsignedByte();
int definitions = buf.readUnsignedShortLE();

return new DefinitionMetadataMessage(database, table, originTable, column, originColumn, collationId,
return new DefinitionMetadataMessage(database, table, originTable, column, originColumn, extendTypeInfo, collationId,
size, typeId, definitions, buf.readUnsignedByte());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private static MySqlRowDescriptor create(final String... names) {
MySqlColumnDescriptor[] metadata = new MySqlColumnDescriptor[names.length];
for (int i = 0; i < names.length; ++i) {
metadata[i] =
new MySqlColumnDescriptor(i, (short) 0, names[i], 0, 0, 0, 1);
new MySqlColumnDescriptor(i, (short) 0, names[i], 0, 0, 0, 1, null);
}
return new MySqlRowDescriptor(metadata);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package io.asyncer.r2dbc.mysql;

import io.asyncer.r2dbc.mysql.collation.CharCollation;
import io.asyncer.r2dbc.mysql.constant.MySqlType;

import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -28,7 +30,7 @@ class MySqlTypeMetadataTest {

@Test
void allSet() {
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, -1, 0);
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, -1, 0, null);

assertThat(metadata.isBinary()).isTrue();
assertThat(metadata.isSet()).isTrue();
Expand All @@ -39,7 +41,7 @@ void allSet() {

@Test
void noSet() {
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, 0, 0);
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, 0, 0, null);

assertThat(metadata.isBinary()).isFalse();
assertThat(metadata.isSet()).isFalse();
Expand All @@ -50,11 +52,24 @@ void noSet() {

@Test
void isBinaryUsesCollationId() {
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, -1, CharCollation.BINARY_ID);
MySqlTypeMetadata metadata = new MySqlTypeMetadata(0, -1, CharCollation.BINARY_ID, null);

assertThat(metadata.isBinary()).isTrue();

metadata = new MySqlTypeMetadata(0, -1, 33);
metadata = new MySqlTypeMetadata(0, -1, 33, null);
assertThat(metadata.isBinary()).isFalse();
}

@Test
svats0001 marked this conversation as resolved.
Show resolved Hide resolved
void mariaDbJsonReturnsCorrectMySqlType() {
MySqlTypeMetadata metadata = new MySqlTypeMetadata(254, 0, 0, "json");

assertThat(metadata.isMariaDbJson()).isTrue();
assertThat(MySqlType.of(metadata)).isEqualTo(MySqlType.JSON);

metadata = new MySqlTypeMetadata(254, 0 ,0 , null);

assertThat(metadata.isMariaDbJson()).isFalse();
assertThat(MySqlType.of(metadata)).isEqualTo(MySqlType.VARCHAR);
}
}
Loading