Support concrete types and custom types #3

wchargin · 2024-07-01T16:55:32Z

Previously, you could only scan into a *geom.T. Now, you can scan into a *geom.T, or a *geom.Polygon (with runtime checking), or any type that implements the GeomScanner interface. This enables end users to define their own wrapper types for interoperability with other systems, such as ORMs that require specific methods to be defined on the types that are used on model fields.

The public interface follows the extensibility pattern described in the documentation for pgtype, which is also used for first-party pgtype codecs: e.g., TimeCodec uses TimeScanner and TimeValuer.

Test Plan:
Unit tests included. I couldn't figure out how to get encoding tests to run in both text and binary formats, but I did manually verify that if you change the preferred format from binary to text then the appropriate codepath is exercised and the test still passes.

wchargin · 2024-07-01T16:55:46Z

For context: I'm trying to use go-geom and pgx-geom with an existing application built atop Gorm. While pgx-geom works by itself without issue, Gorm is proving a bit of a headache. It requires me to define custom types that implement its proprietary GormDataTypeInterface and the Scanner and Valuer interfaces directly on the type that appears in the Gorm model, as well as a pgxtype driver for those types. (It seems silly given that the other components play nicely together already, but this is the only thing I've found that works.) So this patch exists to facilitate that use case.

The support for scanning into *geom.Polygon, etc. types is not technically a prerequisite and could be dropped if you don't like it, but it's along similar lines and I think it's a nice usability improvement.

Happy to make any changes to patch style, etc. per your preferences; just let me know! Thanks for writing these libraries; they look great so far <3

twpayne

Thank you for this excellent PR!

I have a few minor nitpicks, please see the individual review comments.

Once you've made the changes, please could you squash all the commits into one with the message:

feat: support concrete types and custom types

pgxgeom.go

twpayne · 2024-07-01T20:56:48Z

pgxgeom_test.go

-				assert.Equal(t, err.Error(), "pgxgeom: got *geom.Polygon, want *geom.Point")
+				var scanArgError pgx.ScanArgError
+				assert.True(t, errors.As(err, &scanArgError))
+				assert.Equal(t, scanArgError.Err.Error(), "pgxgeom: got *geom.Polygon, want *geom.Point")


Please use assert.EqualError instead of assert.True and assert.Equal.

Hmm, okay. I was intending to test only the part of the error message that is controlled by this package rather than the "can't scan into dest[0]" prefix that pgtype itself adds. But I've switched to EqualError with the full message; done, and let me know if you see a better way to do this.

twpayne · 2024-07-01T20:58:18Z

pgxgeom.go

+// GeomScanner enables PostGIS geometry/geography values to be scanned into
+// arbitrary Go types. For more context, see section "Extending Existing
+// PostgreSQL Type Support" of the README for jackc/pgx/v5/pgtype.
+type GeomScanner interface {


Does GeomScanner need to be exported? If not, please rename it to geomScanner.

Good question. I think that they should be exported. Reasons: (1) it is semantically part of the public API and should be documented so that users know what methods they need to implement on their custom types; (2) this follows the pattern set by first-party codecs like pgtype.TimeValuer / pgtype.TimeScanner, which are exported; and (3) in my code that declares custom types that implement these interfaces, I am declaring

var _ pgxgeom.GeomValuer = &MyMultiLineString{} var _ pgxgeom.GeomScanner = &MyMultiLineString{}

to check at compile time that I have implemented the signatures correctly, which also requires that these symbols be exported.

If you disagree, let me know and I'm happy to amend; I have a preference but do not feel strongly about it.

pgxgeom.go

twpayne · 2024-07-01T21:03:17Z

pgxgeom_test.go

+
+		g, err := ewkb.Unmarshal(bytes)
+		assert.NoError(t, err)
+		assert.Equal(t, g.(*geom.Point).FlatCoords(), []float64{1, 2})


Please use

assert.Equal(t, mustNewGeomFromWKT(t, "POINT(1 2)", 4326), geom)

instead of testing FlatCoords().

Done. This family of changes gives rise to "mustNewGeomFromWKT - srid always receives 4326 (unparam)", so I have removed that lint rule, too.

pgxgeom_test.go

twpayne · 2024-07-01T21:03:56Z

pgxgeom_test.go

+
+				err = conn.QueryRow(ctx, polygonQuery, pgx.QueryResultFormats{format}).Scan(&point)
+				assert.Error(t, err)
+				assert.Equal(t, err, error(pgx.ScanArgError{Err: errCustomPointScan}))


Please use assert.ErrorEquals instead.

Done, with the caveat that we're now testing the string value instead of the error instance.

wchargin

Thanks for the quick review! Updated and squashed. I'm inferring from your request that you want the squash commit to be entirely without commit body (i.e., subject line only), but let me know if I misunderstood and I would be happy to add it back.

pgxgeom.go

wchargin · 2024-07-01T22:46:13Z

pgxgeom_test.go

-				assert.Equal(t, err.Error(), "pgxgeom: got *geom.Polygon, want *geom.Point")
+				var scanArgError pgx.ScanArgError
+				assert.True(t, errors.As(err, &scanArgError))
+				assert.Equal(t, scanArgError.Err.Error(), "pgxgeom: got *geom.Polygon, want *geom.Point")


Hmm, okay. I was intending to test only the part of the error message that is controlled by this package rather than the "can't scan into dest[0]" prefix that pgtype itself adds. But I've switched to EqualError with the full message; done, and let me know if you see a better way to do this.

wchargin · 2024-07-01T22:47:57Z

pgxgeom_test.go

+
+				err = conn.QueryRow(ctx, polygonQuery, pgx.QueryResultFormats{format}).Scan(&point)
+				assert.Error(t, err)
+				assert.Equal(t, err, error(pgx.ScanArgError{Err: errCustomPointScan}))


Done, with the caveat that we're now testing the string value instead of the error instance.

wchargin · 2024-07-01T22:48:28Z

pgxgeom.go

+// GeomScanner enables PostGIS geometry/geography values to be scanned into
+// arbitrary Go types. For more context, see section "Extending Existing
+// PostgreSQL Type Support" of the README for jackc/pgx/v5/pgtype.
+type GeomScanner interface {


Good question. I think that they should be exported. Reasons: (1) it is semantically part of the public API and should be documented so that users know what methods they need to implement on their custom types; (2) this follows the pattern set by first-party codecs like pgtype.TimeValuer / pgtype.TimeScanner, which are exported; and (3) in my code that declares custom types that implement these interfaces, I am declaring

var _ pgxgeom.GeomValuer = &MyMultiLineString{} var _ pgxgeom.GeomScanner = &MyMultiLineString{}

to check at compile time that I have implemented the signatures correctly, which also requires that these symbols be exported.

If you disagree, let me know and I'm happy to amend; I have a preference but do not feel strongly about it.

pgxgeom.go

wchargin · 2024-07-01T22:53:46Z

pgxgeom_test.go

+
+		g, err := ewkb.Unmarshal(bytes)
+		assert.NoError(t, err)
+		assert.Equal(t, g.(*geom.Point).FlatCoords(), []float64{1, 2})


Done. This family of changes gives rise to "mustNewGeomFromWKT - srid always receives 4326 (unparam)", so I have removed that lint rule, too.

pgxgeom_test.go

twpayne · 2024-07-01T23:09:34Z

Thank you! All your reasoning makes sense. This is a fantastic PR :)

wchargin · 2024-07-01T23:25:05Z

Thank you! I am glad that you find it suitable. And thanks again for the quick review and the solid libraries!

twpayne reviewed Jul 1, 2024

View reviewed changes

wchargin force-pushed the custom-types branch from 06340f1 to 508f010 Compare July 1, 2024 22:59

wchargin commented Jul 1, 2024

View reviewed changes

feat: support concrete types and custom types

267fac6

wchargin force-pushed the custom-types branch from 508f010 to 267fac6 Compare July 1, 2024 23:01

twpayne merged commit 8f38929 into twpayne:master Jul 1, 2024
2 checks passed

twpayne mentioned this pull request Jul 1, 2024

Add initial geography support #4

Merged

wchargin deleted the custom-types branch July 1, 2024 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support concrete types and custom types #3

Support concrete types and custom types #3

wchargin commented Jul 1, 2024

wchargin commented Jul 1, 2024

twpayne left a comment

twpayne Jul 1, 2024

wchargin Jul 1, 2024

twpayne Jul 1, 2024

wchargin Jul 1, 2024

twpayne Jul 1, 2024

wchargin Jul 1, 2024

twpayne Jul 1, 2024

wchargin Jul 1, 2024

wchargin left a comment

wchargin Jul 1, 2024

wchargin Jul 1, 2024

wchargin Jul 1, 2024

wchargin Jul 1, 2024

twpayne commented Jul 1, 2024

wchargin commented Jul 1, 2024

Support concrete types and custom types #3

Support concrete types and custom types #3

Conversation

wchargin commented Jul 1, 2024

wchargin commented Jul 1, 2024

twpayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twpayne commented Jul 1, 2024

wchargin commented Jul 1, 2024