search

twpayne / go-geom

Package geom implements efficient geometry types for geospatial applications.
BSD 2-Clause "Simplified" License
839 stars 104 forks source link

Question about the Valuer implementation of EWKB #201

Closed JanBaryla closed 1 year ago

JanBaryla commented 3 years ago

Hi,

lately I've been trying to get this library to work with sqlboiler, to basically teach it how to talk to PostGIS. Everything went pretty well, but when attempting to INSERT or UPDATE, this error https://github.com/golang/go/blob/master/src/database/sql/convert.go#L194 kept popping up.

Everything looked good, I didn't understand why the Value isn't called. I found out this switch statement https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/database/sql/driver/types.go#L241 doesn't detect ewkb.GeometryCollection that I was using as a Valuer.

The fix I ended up doing is this: https://github.com/JanBaryla/go-geom/commit/daae2eef34b19e20253170ee1b0a5c0786cd04ac. Not quite sure what the consequences could be, though, so I'm opening an Issue instead of a PR.

twpayne commented 3 years ago

It might be necessary to use *ewkb.GeometryCollection (i.e. pointer to struct) not ewkb.GeometryCollection. This allows the handling of nil values (which are represented as NULL in SQL). go-geom currently does not support NOT NULL geometries.

There's a PostGIS example that includes an INSERT statement.

Hope this helps. If the problem persists when using *ewkb.GeometryCollection please could you share code that reproduces the problem - I'm not familiar with sqlboiler (but it looks like a really cool project!).

JanBaryla commented 3 years ago

I see. You're right that nils might be tricky.

I'll see what I can do when it comes to that example, there's quite some boilerplate if you wanna use sqlboiler.

But it really just comes down to the fact that even though ewkb.GeometryCollection appears to be implementing Value, it actually doesn't (because of the pointers), and the Golang's standard library for database operations doesn't recognize it as a Valuer because of it (because instead of blindly calling Value, it does the switch on type and tries checking the type)

It seems like in order to correctly implement the interface and keep things clean with nulls, one would have to add types like ewkb.NullGeometryCollection.

twpayne commented 3 years ago

It seems like in order to correctly implement the interface and keep things clean with nulls, one would have to add types like ewkb.NullGeometryCollection.

Yes, that makes sense. To be clear, it's only *ewkb.GeometryCollection that implements sql/driver.Valuer. ewkb.GeometryCollection does not implement the interface.

If I remember correctly, when I first implemented this it just seemed simpler and without loss of functionality to implement nullable geometries by default, as I didn't see a strong reason to implement non-nullable geometries. That said, I would be very happy to add ewkb.NullGeometryCollection and friends to go-geom. If we make any non-backwards-compatible changes then we can just bump the Go module's major version.

twpayne commented 1 year ago

Hopefully this is now resolved. Please re-open if needed.