brick / geo

GIS geometry library for PHP
MIT License
220 stars 31 forks source link

PDO Statement preparation failure on MySQL #30

Closed jon48 closed 3 years ago

jon48 commented 3 years ago

Hello,

I am trying to use the library in one of my projects, for both its GeoJson support, and its geometry operations. However, when trying the operations on geometry, using the PDOEngine against a MySQL database, the engine fails to prepare the statement, with an error :

This operation is not supported by the geometry engine. …\vendor\brick\geo\src\Exception\GeometryEngineException.php:28

with the underlying SQL error

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '?, ?), ST_GeomFromWKB(_binary ?, ?))'

Trying to dig in, this seems to be the _binary keyword with is causing issue, and I am wondering whether this should be binary instead. https://github.com/brick/geo/blob/212181205835d916b83d3f2a726ccb396e487c44/src/Engine/PDOEngine.php#L100

Example

brick/geo version: 0.6.0 PHP Version: 8.0.3 MySQL Database version: 8.0.15

My code is basically

GeometryEngineRegistry::set(new PDOEngine(DB::connection()->getPdo()));
$point = Point::xy(1, 1);
return $point->equals($point);

which raises an exception at the following line https://github.com/brick/geo/blob/212181205835d916b83d3f2a726ccb396e487c44/src/Engine/PDOEngine.php#L49

with

$query = 'SELECT ST_Equals(ST_GeomFromWKB(_binary ?, ?), ST_GeomFromWKB(_binary ?, ?))'

Indeed, I get an error when running in MySQL

PREPARE stmt1 FROM 'SELECT ST_Equals(ST_GeomFromWKB(_binary ?, ?), ST_GeomFromWKB(_binary ?, ?))';

whereas the below runs successfully, and my code runs as well if I modify the code accordingly.

PREPARE stmt1 FROM 'SELECT ST_Equals(ST_GeomFromWKB(binary ?, ?), ST_GeomFromWKB(binary ?, ?))';
BenMorel commented 3 years ago

Hi, are you sure you're running MySQL 8.0? I can't replicate it here, and I successfully used brick/geo on MySQL 8.0 myself.

According to the docs, _binary is a character set introducer that can be applied to a string literal, whereas BINARY is an operator that converts an expression to a binary string.

I'm not sure if it would make a difference to use BINARY instead, but anyway _binary should work so I'd like to dig this further.

BenMorel commented 3 years ago

Sorry, closed by mistake.

Can you please try if it works with the following code, it's possible that _binary does not work with non-emulated prepares:

$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
jon48 commented 3 years ago

Thanks for the feedback. I can confirm I am using MySQL 8.0: I actually tried against 2 instances I have with the same results: 8.0.15 on a Windows 10 PC, and 8.0.23-0ubuntu0.20.04.1 on a server.

However, setting $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, true); allows the query to run successfully, so this seems to be the native prepare that does not accept the _binary inducer.

I am not an expert of PDO or MySQL beyond the standard interactions, and I tried to find some details about characters set inducers in PREPARE statements on the web, without much luck.

If I focus on running the commands in MySQL directly, I can use the inducers in queries, but do not seem to be able to use them in PREPARE statement (which would be coherent with PDO failing when using the native PREPARE). Maybe this make senses as the character sets seems to be linked fundamentally to the parameters, not the statement itself, and this may be something to deal when binding rather than preparing the statement (which is done anyway, as the parameters values are assigned the PDO::PARAM_LOB data type, when adequate). But again, I am not expert, so this is just speculation.

mysql> select @@version;
+-------------------------+
| @@version               |
+-------------------------+
| 8.0.23-0ubuntu0.20.04.1 |
+-------------------------+
1 row in set (0.00 sec)

mysql> SELECT _binary X'40' col;
+----------+
| col      |
+----------+
| 0x40     |
+----------+
1 row in set (0.00 sec)

mysql> PREPARE stat1 FROM 'SELECT _binary ? col';
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '? col' at line 1

mysql> PREPARE stat1 FROM 'SELECT ? col';
Query OK, 0 rows affected (0.00 sec)
Statement prepared

Same result with another inducer

mysql> SELECT _utf8mb4  X'40' col;
+-----+
| col |
+-----+
| @   |
+-----+
1 row in set (0.00 sec)

mysql> PREPARE stat1 FROM 'SELECT _utf8mb4 ? col';
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '? col' at line 1
BenMorel commented 3 years ago

This makes sense, _binary is supposed to be applied to a string literal, which it is when run on emulated statements (placeholders are converted to strings), but not when using native prepared statements.

I'll see if everything works alright when using BINARY with different connection charsets (I'm a bit worried by the conversion word), and I'll ship a fix.

Thank you for the bug report!

jon48 commented 3 years ago

I will trust your experience for the fix forward. ☺️

I know that the package is still not in a 1.0 stage, but thanks for having put it together and working on it, I was struggling to find complete packages for working on geographical data, and this one seems very promising so far!

BenMorel commented 3 years ago

Fixed & released in 0.5.1 and 0.6.1!

Backported in v0.5 as this is a rather serious bug, and this version is the last compatible with PHP 7.2 & 7.3.

jon48 commented 3 years ago

Thanks! I updated my project, and this is running nicely now!