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

Spatial type and SRID annotations #718

Closed
wants to merge 1 commit into from

Conversation

austindrenski
Copy link
Contributor

@austindrenski austindrenski commented Nov 22, 2018

I was interested in delving deeper into our migrations code, so I picked up #717 to get a better feel for how property annotations interact with migration generation.

This is an area of the project that I'm less familiar with, but hopefully this isn't too far off the correct implementation. Any advice or feedback would be very much appreciated.


This PR follows similar work in the Sqlite provider, but is adjusted for PostGIS. Specifically, #717 asked about specifying the SRID of a geography or geometry column, but (as I understand it) the SRID type modifier can only be set after the spatial type modifier has been set (e.g. (POINT) and (POINT,4326), but not (4326)).

In general, the goal of this PR is to support the following:

builder.HasPostgresExtension("postgis");

builder.Entity<SomeTable>()
       .Property(e => e.SomePoint)
       .ForNpgsqlHasSpatialType("POINT")
       .ForNpgsqlHasSrid(4296);

builder.Entity<SomeTable>()
       .Property(e => e.SomeLocation)
       .ForNpgsqlHasSpatialType("POINT");
CREATE TABLE some_table (
    some_point    GEOMETRY  (POINT,4296),
    some_location GEOGRAPHY (POINT)
);

Closes: #717

@austindrenski austindrenski added the enhancement New feature or request label Nov 22, 2018
@austindrenski austindrenski self-assigned this Nov 22, 2018
@austindrenski austindrenski changed the title WIP: Spatial type and SRID annotations Spatial type and SRID annotations Nov 22, 2018
@roji
Copy link
Member

roji commented Nov 22, 2018

OK, I somehow missed the fact that PostGIS allowed constraining the column definition with the SRID but especially with the type (point, linestring)...!

This PR is a good first step, but it doesn't seem right for the type to be expressed via an annotation: we already have the property's CLR type which expressed this. In other words, if the user has a property of NTS Point, it seems like that should automatically create a PostGIS column of type geometry (POINT). For example, if the type is an annotation, then it's possible to define an NTS Point column with an annotation saying "polygon", which will never work.

The SRID approach, on the other hand, seems correct - the CLR type doesn't express it in any way, so an additional annotation is appropriate.

I've opened #719 to track this. @austindrenski, I do think it makes sense to take care of both issues in the same PR - do you want to tackle this here? I think this is pretty important for 2.2, so if you're not sure I can take over it - let me know.

@austindrenski
Copy link
Contributor Author

austindrenski commented Nov 22, 2018

Oh, interesting. I found myself a little lost in the new plugin model and just ran with the idea that the columns were meant to only use generic geometry and geography.

This was a fun bit of code to work on, but I'm happy to step aside if you've got an idea in mind of how to do this with the normal mappings. I'm also traveling on holiday for a few days, so I won't have much time to rework this until the weekend.

So, if you've got some time, go ahead and take over. Give me a ping and I'd be happy to help review!

@roji
Copy link
Member

roji commented Nov 22, 2018

OK, thanks @austindrenski! I'll close this PR and get to work on another one. Thanks for the effort already done here!

@roji roji closed this Nov 22, 2018
@roji
Copy link
Member

roji commented Nov 22, 2018

@austindrenski note that there are some cleanups unrelated to spatial in this PR which can be submitted in a separate PR (if it's trivial stuff such as style/nullability attributes you can also commit directly).

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

Successfully merging this pull request may close these issues.

2 participants