Skip to content

Fix #15093: Add additional field type metadata for pdo_mysql in getColumnMeta response. #15114

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix #15093: Add additional field type metadata for pdo_mysql in getColumnMeta response. #15114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba3c4c9
Add additional flags to the output returned from the getColumnMeta() …
Jul 25, 2024
19b7a9e
Add 'native_flags' value to the output returned from the getColumnMet…
Jul 25, 2024
398200f
Update expected test output for pdo_mysql bug_33689.
Jul 26, 2024
8bd06a7
Add test without output for new pdo_mysql flags.
Jul 29, 2024
7d3aba9
Fix create table statement for new pdo_mysql test.
Jul 29, 2024
fb9a2ac
Add default value to uuid column in pdo_mysql test.
Jul 29, 2024
0d19718
Add expected output for pdo_mysql test.
Jul 29, 2024
ba7c8d2
Skip the new pdo_mysql flags test if we are not testing against the m…
Jul 30, 2024
d31423e
Revert "Skip the new pdo_mysql flags test if we are not testing again…
Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions ext/mysqlnd/mysqlnd_enum_n_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,14 +348,27 @@ typedef enum mysqlnd_server_option
#define SET_FLAG 2048
#define NO_DEFAULT_VALUE_FLAG 4096
#define ON_UPDATE_NOW_FLAG 8192
#define NUM_FLAG 32768

/* The following flags are marked as internal in mysql_com.h */
#define PART_KEY_FLAG 16384
#define GROUP_FLAG 32768
#define NUM_FLAG 32768

#define IS_PRI_KEY(n) ((n) & PRI_KEY_FLAG)
#define IS_NOT_NULL(n) ((n) & NOT_NULL_FLAG)
#define IS_BLOB(n) ((n) & BLOB_FLAG)
#define IS_NUM(t) ((t) <= FIELD_TYPE_INT24 || (t) == FIELD_TYPE_YEAR || (t) == FIELD_TYPE_NEWDECIMAL)
Copy link
Member

Choose a reason for hiding this comment

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

IS_NUM condition has changed. What impact does the removed condition have?
In other words, what happens in the new logic if the value is FIELD_TYPE_YEAR or FIELD_TYPE_NEWDECIMAL?

Copy link
Author

Choose a reason for hiding this comment

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

IS_NUM was declared, but never used by mysqlnd. I suppose this would be a breaking change though for anything that imports this header file externally and uses this definition. Internally, mysqlnd sets the NUM_FLAG under the following conditions:

php-src/ext/mysqlnd/mysqlnd_wireprotocol.c

Lines 1221 to 1228 in 0e33b8d

/* Should we set NUM_FLAG (libmysql does it) ? */
if (
(meta->type <= MYSQL_TYPE_INT24 &&
(meta->type != MYSQL_TYPE_TIMESTAMP || meta->length == 14 || meta->length == 8)
) || meta->type == MYSQL_TYPE_YEAR)
{
meta->flags |= NUM_FLAG;
}

However, MySql sets the NUM_FLAG based on their definition of IS_NUM here:
https://github.com/mysql/mysql-server/blob/596f0d238489a9cf9f43ce1ff905984f58d227b6/include/mysql.h#L116-L118

Maybe the condition in mysqlnd should be changed to use the condition provided by mysql?

#define IS_NOT_NULL(n) ((n) & NOT_NULL_FLAG)
#define IS_PRI_KEY(n) ((n) & PRI_KEY_FLAG)
#define IS_UNIQUE_KEY(n) ((n) & UNIQUE_KEY_FLAG)
#define IS_MULTIPLE_KEY(n) ((n) & MULTIPLE_KEY_FLAG)
#define IS_BLOB(n) ((n) & BLOB_FLAG)
#define IS_UNSIGNED(n) ((n) & UNSIGNED_FLAG)
#define IS_ZEROFILL(n) ((n) & ZEROFILL_FLAG)
#define IS_BINARY(n) ((n) & BINARY_FLAG)
#define IS_ENUM(n) ((n) & ENUM_FLAG)
#define IS_AUTO_INCREMENT(n) ((n) & AUTO_INCREMENT_FLAG)
#define IS_TIMESTAMP(n) ((n) & TIMESTAMP_FLAG)
#define IS_SET(n) ((n) & SET_FLAG)
#define IS_NO_DEFAULT_VALUE(n) ((n) & NO_DEFAULT_VALUE_FLAG)
#define IS_ON_UPDATE_NOW(n) ((n) & ON_UPDATE_NOW_FLAG)
#define IS_NUM(n) ((n) & NUM_FLAG)


/*
Expand Down
35 changes: 33 additions & 2 deletions ext/pdo_mysql/mysql_statement.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,15 +793,45 @@ static int pdo_mysql_stmt_col_meta(pdo_stmt_t *stmt, zend_long colno, zval *retu
if (IS_PRI_KEY(F->flags)) {
add_next_index_string(&flags, "primary_key");
}
if (F->flags & MULTIPLE_KEY_FLAG) {
if (IS_MULTIPLE_KEY(F->flags)) {
add_next_index_string(&flags, "multiple_key");
}
if (F->flags & UNIQUE_KEY_FLAG) {
if (IS_UNIQUE_KEY(F->flags)) {
add_next_index_string(&flags, "unique_key");
}
if (IS_BLOB(F->flags)) {
add_next_index_string(&flags, "blob");
}
if (IS_UNSIGNED(F->flags)) {
add_next_index_string(&flags, "unsigned");
}
if (IS_ZEROFILL(F->flags)) {
add_next_index_string(&flags, "zerofill");
}
if (IS_BINARY(F->flags)) {
add_next_index_string(&flags, "binary");
}
if (IS_ENUM(F->flags)) {
add_next_index_string(&flags, "enum");
}
if (IS_AUTO_INCREMENT(F->flags)) {
add_next_index_string(&flags, "auto_increment");
}
if (IS_TIMESTAMP(F->flags)) {
add_next_index_string(&flags, "timestamp");
}
if (IS_SET(F->flags)) {
add_next_index_string(&flags, "set");
}
if (IS_NO_DEFAULT_VALUE(F->flags)) {
add_next_index_string(&flags, "no_default_value");
}
if (IS_ON_UPDATE_NOW(F->flags)) {
add_next_index_string(&flags, "on_update_now");
}
if (IS_NUM(F->flags)) {
add_next_index_string(&flags, "num");
}
str = type_to_name_native(F->type);
if (str) {
add_assoc_string(return_value, "native_type", str);
Expand All @@ -827,6 +857,7 @@ static int pdo_mysql_stmt_col_meta(pdo_stmt_t *stmt, zend_long colno, zval *retu
add_assoc_long(return_value, "pdo_type", param_type);

add_assoc_zval(return_value, "flags", &flags);
add_assoc_long(return_value, "native_flags", F->flags);
add_assoc_string(return_value, "table", (char *) (F->table?F->table : ""));

PDO_DBG_RETURN(SUCCESS);
Expand Down
3 changes: 3 additions & 0 deletions ext/pdo_mysql/tests/bug_33689.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ Array
[flags] => Array
(
[0] => not_null
[1] => no_default_value
[2] => num
)

[native_flags] => 36865
[table] => test_33689
[name] => bar
[len] => 11
Expand Down
170 changes: 170 additions & 0 deletions ext/pdo_mysql/tests/gh15093.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
--TEST--
GH-15093: Add support for all flags from mysql in the PDO MySql driver in the getColumnMeta function.
--EXTENSIONS--
pdo_mysql
--SKIPIF--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
MySQLPDOTest::skip();
Copy link
Member

Choose a reason for hiding this comment

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

MySQLPDOTest::skipNotMySQLnd();

if (MYSQLPDOTest::isPDOMySQLnd()) die("skip libmysql only test");

By using the above codes, you can separate the tests for mysqlnd and libmysql.
IMHO, these features are less common and less likely to be bug-reported, so it is desirable to be able to cover all cases with tests.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the code to use skipNotMySQLnd(), but in retrospect I think you were suggesting to write two separate tests. Given the differences regarding the NUM_FLAG above, separating them does seem like a good idea at first. However, one would expect the two to be functionally equivalent in this test.

?>
--FILE--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
$db = MySQLPDOTest::factory();

$db->exec("
CREATE TABLE `gh_15093` (
`id` INT NOT NULL AUTO_INCREMENT,
`uuid` BINARY(16) DEFAULT (uuid_to_bin(uuid())),
`blob` BLOB,
`ts` TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
`set` SET('one', 'two'),
`enum` ENUM('a', 'b', 'c'),
`num` INT(11) UNSIGNED ZEROFILL DEFAULT 0,
PRIMARY KEY(`id`),
UNIQUE KEY `UUID` (`uuid`)
)
");

$stmt = $db->prepare('SELECT `id`, `uuid`, `blob`, `ts`, `set`, `enum`, `num` FROM gh_15093');
$stmt->execute();

$n = $stmt->columnCount();
$meta = [];

for ($i = 0; $i < $n; ++$i) {
$m = $stmt->getColumnMeta($i);

// libmysql and mysqlnd will show the pdo_type entry at a different position in the hash
// and will report a different type, as mysqlnd returns native types.
unset($m['pdo_type']);
Copy link
Member

@devnexen devnexen Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

note: that or that might be ways around it. e.g. sorting. even for the type display, you re using EXPECTF after all.

Copy link
Author

Choose a reason for hiding this comment

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

This section was pulled from another test. As stated in the comment, the value of the pdo_type may differ based on which driver is used. I only modify the flags and added a native_flags attribute, which is really what the test is meant to cover.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, anyhow the maintainer will review all properly soon :)

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to split the tests per driver and check the value of pdo_type in the test to ensure I am not breaking existing functionality?
These behaviors don't change from driver to driver, just the values ​​change, so I think it's okay to just check it with mysqlnd testing.


$meta[$i] = $m;
}

print_r($meta);
?>
--CLEAN--
<?php
require_once __DIR__ . '/inc/mysql_pdo_test.inc';
$db = MySQLPDOTest::factory();
$db->exec('DROP TABLE IF EXISTS gh_15093');
?>
--EXPECTF--
Array
(
[0] => Array
(
[native_type] => LONG
[flags] => Array
(
[0] => not_null
[1] => primary_key
[2] => auto_increment
[3] => num
)

[native_flags] => 49667
[table] => gh_15093
[name] => id
[len] => 11
[precision] => 0
)

[1] => Array
(
[native_type] => STRING
[flags] => Array
(
[0] => unique_key
[1] => binary
)

[native_flags] => 16516
[table] => gh_15093
[name] => uuid
[len] => 16
[precision] => 0
)

[2] => Array
(
[native_type] => BLOB
[flags] => Array
(
[0] => blob
[1] => binary
)

[native_flags] => 144
[table] => gh_15093
[name] => blob
[len] => 65535
[precision] => 0
)

[3] => Array
(
[native_type] => TIMESTAMP
[flags] => Array
(
[0] => binary
[1] => timestamp
[2] => on_update_now
)

[native_flags] => 9344
[table] => gh_15093
[name] => ts
[len] => 19
[precision] => 0
)

[4] => Array
(
[native_type] => STRING
[flags] => Array
(
[0] => set
)

[native_flags] => 2048
[table] => gh_15093
[name] => set
[len] => 28
[precision] => 0
)

[5] => Array
(
[native_type] => STRING
[flags] => Array
(
[0] => enum
)

[native_flags] => 256
[table] => gh_15093
[name] => enum
[len] => 4
[precision] => 0
)

[6] => Array
(
[native_type] => LONG
[flags] => Array
(
[0] => unsigned
[1] => zerofill
[2] => num
)

[native_flags] => 32864
[table] => gh_15093
[name] => num
[len] => 11
[precision] => 0
)

)
Loading