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

Recognise GEOMCOLLECTION geometry type (MySQL 8) #33

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

gbuckingham89
Copy link
Contributor

We'd been seeing a Brick\Geo\ExceptionGeometryEngineException with the message "Unknown geometry type: GEOMCOLLECTION" in our application in some environments.

After a bit of digging, it seems that in MySQL 8, the types GeometryCollection and GeomCollection can both be used for collections of geometry objects, with GeomCollection now being "the preferred type name": https://dev.mysql.com/doc/refman/8.0/en/gis-class-geometrycollection.html

This is a simple to PR to recognise the GeomCollection geometry type in the getProxyClassName() method of \Brick\Geo\Engine\DatabaseEngine to prevent such exceptions from being thrown.

@gbuckingham89 gbuckingham89 changed the title Recognise GEOMCOLLECTION geometry type Recognise GEOMCOLLECTION geometry type (MySQL 8) Aug 17, 2021
@BenMorel
Copy link
Member

Hi, thank you for reporting this!

The problem is actually bigger than that: if your DatabaseEngine is not configured to use proxies, Geometry::fromText() may fail as well if GeomCollection is used in the WKT.

Could you please paste the result of the following queries on an environment that fails:

SELECT ST_ASText(ST_GeomFromText('GEOMCOLLECTION()'));
SELECT ST_GeometryType(ST_GeomFromText('GEOMCOLLECTION()'));

@BenMorel
Copy link
Member

OK I gave it a try on MySQL 8, and here is the result:

mysql> SELECT ST_ASText(ST_GeomFromText('GEOMCOLLECTION()'));
ERROR 3037 (22023): Invalid GIS data provided to function st_geomfromtext.

mysql> SELECT ST_GeometryType(ST_GeomFromText('GEOMCOLLECTION()'));
ERROR 3037 (22023): Invalid GIS data provided to function st_geomfromtext.

mysql> SELECT ST_ASText(ST_GeomFromText('GEOMETRYCOLLECTION()'));
+----------------------------------------------------+
| ST_ASText(ST_GeomFromText('GEOMETRYCOLLECTION()')) |
+----------------------------------------------------+
| GEOMETRYCOLLECTION EMPTY                           |
+----------------------------------------------------+
1 row in set (0.00 sec)

mysql> SELECT ST_GeometryType(ST_GeomFromText('GEOMETRYCOLLECTION()'));
+----------------------------------------------------------+
| ST_GeometryType(ST_GeomFromText('GEOMETRYCOLLECTION()')) |
+----------------------------------------------------------+
| GEOMCOLLECTION                                           |
+----------------------------------------------------------+
1 row in set (0.00 sec)

In summary: GEOMCOLLECTION does not exist as a WKT type, only as a return value of ST_GeometryType(). This value is only used in getProxyClassName(), so your PR is perfectly relevant! 👍

Could you please just add this comment at the end of the line:

/* MySQL 8 - https://github.com/brick/geo/pull/33 */

So that we keep some context in the code?

@gbuckingham89
Copy link
Contributor Author

gbuckingham89 commented Aug 17, 2021

Hey Ben,

Thanks for the quick response and sorry I didn't get to run the SQL before you! I have just run it on our dev environments and confirm we saw the same results as you.

Good news that it's only the getProxyClassName() change that's needed! 🎉

I've added the the comment in as requested.

Thanks again!

Apologies if there wasn't enough detail in the PR originally - just helping out on a project while the main dev is away, so it's my first time working with your package and spatial MySQL - I've only worked with PostGIS before (with mstaack/laravel-postgis).

@BenMorel BenMorel merged commit a576496 into brick:master Aug 17, 2021
@BenMorel
Copy link
Member

Released as 0.6.2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants