Add outbound Postgres bindings to Go SDK #1227

jpflueger · 2023-03-03T16:58:54Z

I noticed that there were no bindings for outbound Postgres in the go sdk. I was able to generate bindings using wit-bindgen 0.3.0 by wrapping outbound-pg.wit into a temporary wit file using an interface for the rdbms-types.wit and a world for the query and execute functions. Most of the binding code was generated but I did make some modifications for better error handling and split out the functions for readability. I wanted to submit these changes as a starting point. Happy to rework whatever you suggest as this is my first time generating bindings and I'm not super familiar with C or Go.

jpflueger · 2023-03-03T17:02:39Z

Hmm. I must have done something funky to get those merge conflicts. I'll try to resolve and re-push my changes.

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

jpflueger · 2023-03-03T17:21:35Z

Hmm. I must have done something funky to get those merge conflicts. I'll try to resolve and re-push my changes.

Weirdness caused by me not signing commits so I had to rebase with a merge conflict. Fixed locally and force-pushed my branch to update, sorry for the churn.

dicej

Looks great overall -- just a couple of requests.

dicej · 2023-03-06T16:50:39Z

sdk/go/Makefile

+GENERATED_OUTBOUND_REDIS  = redis/outbound-redis.c redis/outbound-redis.h
+GENERATED_SPIN_REDIS      = redis/spin-redis.c redis/spin-redis.h
+GENERATED_KEY_VALUE       = key_value/key-value.c key_value/key-value.h
+GENERATED_OUTBOUND_PG     = postgres/outbound-pg.c postgres/outbound-pg.h


Would you please add postgres/sdk-version-go.c to SDK_VERSION_DEST_FILES below? That helps ensure the SDK version information is embedded in components built with the SDK.

Will do, I need to rebase onto v1.0 branch first and then I'll add it to avoid any merge conflicts.

dicej · 2023-03-06T16:54:19Z

examples/tinygo-outbound-pg/README.md

@@ -0,0 +1,18 @@
+# Spin component with outbound postgres in TinyGo


Would you mind adding instructions for using wit-bindgen 0.3.0 to re-generate the bindings if necessary (e.g. because we add variant cases, new functions, etc. to outbound-pg.wit)?

Yes, I'll need to change a decent amount of the code. I broke things out into different files in order to better understand how things were working but that would be tedious to someone who is just trying to update the bindings quickly.

@dicej I added instructions to the readme in the sdk/go folder. Let me know if that is too hacky to include :)

itowlson

I'm a Go ignoramus so please ignore me if I'm completely off beam

itowlson · 2023-03-07T01:49:49Z

sdk/go/postgres/db_value.go

+
+func (n DbValue) GetBoolean() bool {
+	if g, w := n.Kind(), DbValueKindBoolean; g != w {
+		panic(fmt.Sprintf("Attr kind is %v, not %v", g.String(), w.String()))


I don't know Go database idioms but panicking on the wrong type seems unforgiving. Would it make sense to have a Get and TryGet function for each type, e.g. GetBoolean() bool and TryGetBoolean() bool, bool for the if val, ok := v.TryGetBoolean(); ok { ... } idiom?

Are there existing Go database API patterns we could try to emulate, or is our WIT just too far away from them?

I think this is just how the tinygo binding generator in wit-bindgen works. The idea is that the correct way to get the value is ask what the type is first and then extract the value using the appropriate getter. It's like poor-man's pattern matching. So if the app developer skips the first step and just assumes the type, it's a programmer error, hence the panic. By that reasoning, making the developer check for errors in the second step would be redundant.

Agreed with what Joel said here. With the lack of generics in Go usually this method is accompanied by a check like this:

if dbValue.Kind() == DbValueKindBoolean { dbBool := dbValue.GetBoolean() }

My understand of the go idioms is that a tuple is essentially used like a Result<T> in rust where a function like func x() (bool, Error) is equivalent to fn x() -> Result<bool, Error> in the Rust world.

All that being said, this is generated code so our request will need to go to wit-bindgen.

itowlson · 2023-03-07T02:00:51Z

sdk/go/postgres/db_value.go

+	n.kind = DbValueKindBinary
+}
+
+func DbValueDbNull() DbValue {


Should we have something like an IsNull function? What sort of experience should we expect for loading a value from a NULLable column, especially if the developer wants graceful failure rather than panic?

(Again this is me being ignorant of How Things Are in Go.)

Technically in Go, null == nil so our language is a little off but we could add a function like this in an additional layer of our SDK. I did find that check necessary, especially on nullable columns where the column type is actually string but there was no value so the kind in our sdk was DbValueKindNull instead of string with a nil value.

func (v *DbValue) IsNull() bool { return v.Kind() == DbValueKindBoolean }

Let me clarify what I'm driving at. Consider a People table with two (interesting) columns, Age (integer) and MiddleName (string). Both are NULLable in the database (to reflect unknown age and people who don't have a middle name).

I want to query for the set of People and process that list, for example rendering to HTML (printing unknown for NULL ages and - for no middle name), or marshalling to JSON (where I guess I'd omit the NULL fields), but for generality going via some intermediate Go structure.

How (generally) do I do this?

If memory serves, for the intermediate structure to allow nil, its values would have to be pointers e.g. { Age *int, MiddleName *string }. Is that right? If so, what is the outline for populating them, something like:

var age if v.Kind() == DbNull { age = nil } else { age = &v.GetInt() } person := Person { Age: age, ... }

Is that right (modulo Go syntax!), and if so, does it seem like what a Go programmer would expect to right?

To be clear, I am not suggesting it's not! I'm not familiar with Go idioms - for all I know this is exactly what a Go programmer would find familiar. I'm just confirming that we are happy with whatever the pattern for nullable columns is.

Hope this makes more sense now - sorry for the somewhat muddled question first time around!

If memory serves, for the intermediate structure to allow nil, its values would have to be pointers e.g. { Age *int, MiddleName *string }. Is that right?

I do believe that is right, yes. When I created a struct without pointers, the struct is initialized with default values (i.e. 0 for int, "" for string, etc.)

itowlson · 2023-03-07T02:03:48Z

sdk/go/postgres/pg_types.go

+
+func (n PgError) GetConnectionFailed() string {
+	if g, w := n.Kind(), PgErrorKindConnectionFailed; g != w {
+		panic(fmt.Sprintf("Attr kind is %v, not %v", g.String(), w.String()))


Again panics seem quite harsh, especially given that the developer can't expect errors the way they can data types... is the expectation that the app would call Kind() before calling the relevant Get function?

At least the way that I've used it and the way the generated code works, I'm always forced to check the kind before calling this function.

itowlson · 2023-03-07T02:04:57Z

sdk/go/postgres/pg_types.go

+	return n.val.(string)
+}
+
+func (n *PgError) SetConnectionFailed(v string) {


Not quite sure where an app would use these functions - could they be private?

The set functions could be private yes but this is just how wit-bindgen works as it stands today. I'm unclear on how we could customize this behavior. We could wrap the generated code better but that would incur a maintenance cost.

Mossaka · 2023-03-07T19:17:10Z

Hey I am glad that you were able to use the wit-bindgen-go generator to generate the bindings! I would greatly appreicate any feedback to improve the binding's ergonomics as I didn't pay too much attention to it as I was trying to get the it done as soon as possible.

radu-matei · 2023-03-08T14:13:37Z

Hey, @Mossaka — I think this is still using the C bindings generator.

jpflueger · 2023-03-08T16:39:09Z

@radu-matei actually this is using both. I used our pinned version of wit-bindgen to generate the C bindings. Then I took our wit files and added a default world so it was compatible with the newer wit-bindgen version, used the wit-bindgen-go generator and copied the go files over. I did this as an experiment so I didn't have to write the bindings myself so this is definitely a hack.

@Mossaka it was pretty easy to use once I got our WIT into the right format. I do have some small feedback for the generated code and I'll file issues in the wit-bindgen repo so you can keep track of them there.

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

vdice · 2023-11-09T16:42:47Z

@jpflueger Outbound Postgres for the Go SDK landed in #1922. Shall we close this?

jpflueger requested review from dicej and adamreese March 3, 2023 16:58

jpflueger force-pushed the feature/go-outbound-pg branch from 5465a9c to 16b616b Compare March 3, 2023 17:01

Justin Pflueger added 7 commits March 3, 2023 10:13

bindgen for outbound-pg with go sdk

b325a55

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

save progress

e32144e

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

copy-pasta some code from wit-bindgen 0.3.0 output

eab284d

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

adding stdlib for c.malloc

9501def

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

improved error handling

61dfa1d

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

add better error messages for types

5b99c4b

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

add basic example

7088ec0

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

jpflueger force-pushed the feature/go-outbound-pg branch from 16b616b to 7088ec0 Compare March 3, 2023 17:20

dicej reviewed Mar 6, 2023

View reviewed changes

itowlson reviewed Mar 7, 2023

View reviewed changes

Justin Pflueger added 3 commits March 13, 2023 16:05

reworking the bindings and adding instructions

cf3c3f7

improved error handling

423d9b1

Signed-off-by: Justin Pflueger <justin.pflueger@fermyon.com>

Merge branch 'main' into feature/go-outbound-pg

4a0ac83

jpflueger closed this Nov 9, 2023

		@@ -0,0 +1,18 @@
		# Spin component with outbound postgres in TinyGo

Add outbound Postgres bindings to Go SDK #1227

Add outbound Postgres bindings to Go SDK #1227

Uh oh!

Conversation

jpflueger commented Mar 3, 2023

Uh oh!

jpflueger commented Mar 3, 2023

Uh oh!

jpflueger commented Mar 3, 2023

Uh oh!

dicej left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mossaka commented Mar 7, 2023

Uh oh!

radu-matei commented Mar 8, 2023

Uh oh!

jpflueger commented Mar 8, 2023

Uh oh!

vdice commented Nov 9, 2023

Uh oh!

Uh oh!