longitude-one / doctrine-spatial

Doctrine extension to persist spatial data objects.
https://lo-doctrine-spatial.readthedocs.io/en/latest/
MIT License
69 stars 21 forks source link

ST_SRID should accept 2 parameters on MySQL #100

Closed holtkamp closed 5 days ago

holtkamp commented 4 months ago

ST_SRID accepts 2 parameters on MySQL: https://dev.mysql.com/doc/refman/8.4/en/gis-general-property-functions.html#function_st-srid while https://github.com/longitude-one/doctrine-spatial/blob/721148909ebe7fa83c3e4f4875b0e934704dae91/lib/LongitudeOne/Spatial/ORM/Query/AST/Functions/Standard/StSrid.php#L53 only allows 1

This makes it impossible to set the proper SRID on a geometry in MySQL for example in comparison DQL query like:

ST_Contains(entity.geometryProperty, ST_SRID( SP_Point(:longitude, :latitude), :srid))

Note that ST_SetSRID() is not implemented on MySQL.

I think a dedicated LongitudeOne\Spatial\ORM\Query\AST\Functions\MySql\StSrid class is required which overrides the getMaxParameter() function and returns "2"?

Also so some old work of mine here: https://github.com/creof/doctrine2-spatial/issues/52#issuecomment-570930112

Alexandre-T commented 4 months ago

ISO/IEC 13249-3 defines the SRID function to return the Spatial Reference System Identifier of a geometry or a geography, when function is used with one argument.

-- PostgreSQL sample
SELECT st_srid(ST_GeomFromText('POINT(1 1)', 4326)); --4326
-- MySQL 5.7 and 8.0 samples
set @g1 = ST_GeomFromText('POINT(1 1)', 4326);
select ST_SRID (@g1); -- 4326
-- MSSQL Server
DECLARE @g geometry;
SET @g = geometry::STGeomFromText('POINT(1 1)', 4326);
SELECT @g.STSrid; --4326

However ISO/IEC 13249-3 allows a second argument to specify a new SRID, which would return a new geometry projected to that SRID. This second option is not currently supported bylongitude-one/doctrine-spatial.

-- PostGreSQL sample
SELECT st_srid(ST_GeomFromText('POINT(1 1)', 4326), 4120); 
-- [42883] ERROR: function st_srid(geometry, integer) does not exist
SELECT ST_SRID(ST_TRANSFORM(ST_GeomFromText('POINT(1 1)', 4326), 4120)); -- 4120

-- MySQL 5.7 sample failed
set @g1 = ST_GeomFromText('POINT(1 1)', 4120);
select ST_SRID(ST_SRID (@g1, 4326)); 
-- [42000][1582] Incorrect parameter count in the call to native function 'ST_SRID'

-- MySQL 8.0 working sample
set @g1 = ST_GeomFromText('POINT(1 1)', null);
set @g2 = ST_SRID(@g1, 4326);
select ST_SRID(@g2); -- 4326 

-- MSSQL Server cannot do it.
DECLARE @g1 geometry;
set @g1 = geometry::STGeomFromText('POINT(1 1)', null);
select @g1.STSrid(4326); -- 4326 --This line failed: [S0010][6506] Line 3: Could not find method 'STSrid' for type 'Microsoft.SqlServer.Types.SqlGeometry' in assembly 'Microsoft.SqlServer.Types'

The standard function StSrid must be edited to allow one or two parameters, because longitude-one/doctrine-spatial focus on ISO/IEC 13249-3.

The test should be updated with your previous sample.

I already read your links a few months ago. You're totally right! If the srid is null, st_srid won't update it.

-- PostGreSQL sample
SELECT st_srid(ST_GeomFromText('POINT(1 1)', null), 4326); --null
-- [42883] ERROR: function st_srid(geometry, integer) does not exist
SELECT ST_SRID(ST_TRANSFORM(ST_GeomFromText('POINT(1 1)', null), 4326)); -- null

-- MySQL 8.0 working sample
set @g1 = ST_GeomFromText('POINT(1 1)', null);
set @g2 = ST_SRID(@g1, 4326);
select ST_SRID(@g2); -- 4326 

And it is a major issue of this extension. SRID of an entity is not saved with MySQL. And I don't know how to implement it because of the convertToValueSQL and convertToValue limitations of doctrine/orm. (I'm using PostGreSQL, and the implementation is totally different and certainly easiest because PostGis allow this kind of WKT : SRID=4326;POINT(1 1).)

I'm looking for solutions, but it's very difficult to implement it. If I remember, in your PR you suggest to add a default SRID on platform. I think it's a better way to do it in the type. And be careful, SRID can be used with geographic types, but with geometries ones too!

holtkamp commented 4 months ago

Hi @Alexandre-T,

thanks for the structured and detailed response, really thorough!

The standard function StSrid must be edited to allow one or two parameters, because longitude-one/doctrine-spatial focus on ISO/IEC 13249-3.

Ah, so the standard ST_SRID() function should be updated, do you want me to submit a PR, or are you already working on it?

If I remember, in your PR you suggest to add a default SRID on platform. I think it's a better way to do it in the type.

Indeed: the best approach is to respect the SRID that is set for a Geometry when loading/persisting it.

However: that is indeed difficult / currently impossible?

For now the most pragmatic approach seems to allow to set a default SRID that is used for all persisted / loaded geometries... Then the extension becomes really usable and from that we can try to work to implement it properly in the future, right?

PS: I successfully migrated from https://github.com/creof/doctrine2-spatial and am using the following branch right now: https://github.com/holtkamp/doctrine-spatial/tree/patch-srid-and-axis-order, it would be nice to prevent the need of this branch 😉

Alexandre-T commented 4 months ago

Ho @holtkamp , I'm currently working on issue #17, which will involve some major updates. Your help would be really welcome! I would really appreciate it if you could submit a pull request (PR) updating StSrid and the involved test. About the best approach, I completly agree: the best approach is to respect the SRID when loading/persisting it. However, it's quite complex due to the internal structure of BasicEntityPersister in doctrine/orm. It only allows for a single parameter, preventing me from using something like:

public function convertToDabase(string $sqlExpr, AbstractPlaform $platform): string
   // $sqlExpr is a string equal to the famous '?'
   return sprintf('ST_GeomFromText(%s, %s)', $sqlExpr, $sqlExpr);
}

For now, your solution seems like an interesting compromise. Could you please submit it as a pull request? Thank you so much for the link about axis-order!. I didn't know that we are able to set the axis order in the SRS!

PS: To respect the SRID when persisting the geo*y, I'm working on a solution to store it directly in the internal MySQL format. The internal format looks like EWKB! So I'm creating a POC to test to directly to store a binary. If POC works, it's a green flag to create a new library to generate EWKB from any SpatialInterface. If it's a red flag, I will mix your solution and another one on the AbstractType.

Alexandre-T commented 4 months ago

About your essential question,

However: that is indeed difficult / currently impossible?

I just found a solution to store the SRID!

//LongitudeOne/Spatial/DBAL/Platform/MySql.php

    public function convertToDatabaseValueSql(AbstractSpatialType $type, string $sqlExpr): string
    {
        return sprintf('UNHEX(%s)', $sqlExpr);
    }

With the below method SRID and GEOMETRY can be stored in one pass. So SRID will be stored during the persisting step. It is currently possible ! 🎉​🎉​🎉​

I now have to find a way to complete the second method:

//LongitudeOne/Spatial/DBAL/Platform/MySql.php

    public function convertToDatabaseValue(AbstractSpatialType $type, SpatialInterface $value): string|float
    {
         //$value is a geometry point(42 42) with the SRID 4326
         // STEP 1- convert SRID 4326 to hexadecimal (decimal 4326 is 0000010E6 in hexadecimal)
         // STEP 2- revert string 00000010E6 to E6100000
         // STEP 3 - As wkb-parser convert a binary to a SpatialInterface, I have to develop/find a new library to convert a SpatialInterface to an hexadecimal string. 
        //STEP 4.
        $result = (new MyConverter)->convertToHexadecimalString($value);
        // $result = 'E6100000010100000000000000000045400000000000004540'
        return $result ;
    }

Then I have to found a way to retrieve the SRID before building a SpatialInterface. I'm currently using the ST_ASBinary method:

    public function convertToPhpValueSql(AbstractSpatialType $type, $sqlExpr): string
    {
        return sprintf('ST_AsBinary(%s)', $sqlExpr);
    }

When I replace ST_AsBinary with HEX I already got 'E6100000010100000000000000000045400000000000004540'. So I have to split the string, convert E6100000 into 4326 and pack 010100000000000000000045400000000000004540 to a binary, then I will use the wkb-parser.

holtkamp commented 4 months ago

@Alexandre-T

I would really appreciate it if you could submit a pull request (PR) updating StSrid and the involved test.

Ok, will try to do that, got little time, so hopefully this month...

I just found a solution to store the SRID!

Wow, that is a really elegant approach!

And that would still allow to change the axis order in convertToPhpValueSql() by handing it over as 2nd argument to ST_AsBinary() 🎉

holtkamp commented 4 months ago

@Alexandre-T I am trying to get my test environment for MySQL up and running. Where can I find the documented phpunit.mysql.xml.dist file?

Alexandre-T commented 4 months ago

Sorry, it looks like my documentation isn't up-to-date! In the docker directory, you'll find two files: phpunit.mysql5.7.xml and phpunit.mysql5.7.xml. And the README.md files are a complement to documentation.

Alexandre-T commented 4 months ago

And that would still allow to change the axis order in convertToPhpValueSql() by handing it over as 2nd argument to ST_AsBinary() 🎉

I'm currently creating the WKB writer. Until now, I didn't realize how subtle it is! I have to replace the int SRID with an SRS class that has two properties: srid and axis_order. This is a lot of work!

petski commented 1 month ago

Hi @Alexandre-T, I've seen your work on https://github.com/longitude-one/spatial-writer and you seem almost there. How's it going? Any help needed?

Regarding "a lot of work": maybe a task for https://github.com/rectorphp/rector?

Alexandre-T commented 1 week ago

@holtkamp : Job is done. I check again, then I will merge it into main. Then, I merge into 6.0.dev to remove the deprecation for PgSql users and the "bad" compatibility.

@petski ; any help, any PR are welcomed!

Alexandre-T commented 5 days ago

Fixed since 5.0.3 👍