Add generic ArrayOf[T any] type

[arp242]

Add generic ArrayOf[T any] type

This adds a new generic ArrayOf[T any] type:

	var a pq.ArrayOf[int]
	db.QueryRow("...").Scan(&a)

This replaces all the existing Array* types, which are now implemented
with ArrayOf type aliases and marked as deprecated. The existing tests
are unchanged except for some error texts, and I believe ths should be
fully compatible.

Only GenericArray is left as-is, as re-implementing that with ArrayOf in
a way that's fully compatible is kind of tricky. I'd rather just leave
it as-is instead of spending the effort and risk breakage.

The main issue is that we rely on:

	//go:linkname convertAssign database/sql.convertAssign
	func convertAssign(dest, src any) error

Which does not exactly spark joy, however there is already a comment on
convertAssign():

	// convertAssign should be an internal detail,
	// but widely used packages access it using linkname.
	// Notable members of the hall of shame include:
	//   - ariga.io/entcache
	//
	// Do not remove or change the type signature.
	// See go.dev/issue/67401.

So if compatibility is already guaranteed because of other packages,
then I guess it's okay to join this "hall of shame". Also, it looks like
it'll probably just get exported in the future:
golang/go#62146 (comment)

Fixes #1103

[Ferada]

Tried this on a project and everything still works, replacing the deprecated types with ArrayOf is also easy 👍

Edit: Pushed suggestions below to this commit.

[Ferada]

Given that the element type is already known, wouldn't it be faster to move the type assertion out of the loop for the common cases?

That is, specialise this block for the common slice types via something like:

switch xx := any([]T(a)).(type) {
case []bool:
	...
case []uint8:
	b = appendAllUint(xx, b, del)
case []uint16:
	b = appendAllUint(xx, b, del)
...
default:
	// only use reflection on individual elements here
	for i, aa := range a {
		...
	}
}

The duplication for the signed and unsigned types could also be minimised with something like:

func appendAllUint[T uint8 | uint16 | uint32 | uint64](xx []T, b []byte, del []byte) []byte {
	for i, aa := range xx {
		if i > 0 {
			b = append(b, del...)
		}
		b = strconv.AppendUint(b, uint64(aa), 10)
	}
	return b
}

// similar for the other types

[AGWA]

I tried to use ArrayOf with the following type, which expects to be scanned from a bytea value:

type SHA256Digest [32]byte 
        
func (digest *SHA256Digest) Scan(src any) error {
        switch src := src.(type) {
        case []byte:
                if len(src) != len(digest) {
                        return fmt.Errorf("cannot scan SHA256Digest from %d bytes (%d bytes expected)", len(src), len(digest))
                }
                copy(digest[:], src)
                return nil
        case nil:
                return errors.New("cannot scan SHA256Digest from NULL column")
        default:
                return fmt.Errorf("cannot convert column of type %T to SHA256Digest", src)
        }       
}

Unfortunately, it doesn't work because the value which ArrayOf[T].scan passes to convertAssign is formatted as text (e.g. \x001122) instead of being the raw bytes that you normally get when scanning a bytea value, so my Scan method fails.

This could be fixed if ArrayOf[T].scan passed a string to convertAssign instead of a []byte:

err := convertAssign(&b[i], string(v))

Then, my Scan method could add a case for string which parses the textual format.

I believe passing a string is more correct than []byte because PostgreSQL array elements are always text, not raw bytes. It also seems more consistent with how ArrayOf[any] is treated as string.

My type doesn't work with GenericArray either, but fixing it there would break compatibility. Since you're leaving GenericArray as-is, this seems like a good opportunity to make this change.

P.S. Many thanks for taking over maintainership of this package!

[arp242]

@AGWA The Scan() method gets called for every array element, so using that SHA256Digest.Scan() with ArrayOf (or GenericArray) is not really correct: that method should process a single PostgreSQL array element, not the entire array.

This is how GenericArray works, so I copied that to ArrayOf. It kind of makes sense, but I also have to admit that I was confused for a bit about this myself after coming back to this after several months. At the very least it should be documented properly (I added a TODO comment in my rebase). There are also some changes in the upcoming Go 1.27 surrounding this that may help, but I haven't really looked at them yet. I've been holding off merging this until Go 1.27 for that reason.

Using an array (rather than slice) should probably work, and check the length. Then your Scan() method becomes superfluous and you can use ArrayOf[[32]byte] or ArrayOf[SHA256Digest].

Passing []byte or string to convertAssign() seems unrelated? I think you're just "using it wrong" (which is not your fault, as it is more confusing than it should be).

[AGWA]

@arp242 Thanks for looking at this.

I think I'm using things correctly. Let me try to explain my use case better.

Currently, my SHA256Digest type works great for scanning a single (non-array) 32-byte bytea value.
The Scan method gets called with a 32-byte []byte containing the raw bytes. This code works as expected:

var digest SHA256Digest
if err := db.QueryRow(`SELECT '\x01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b'::bytea`).Scan(&digest); err != nil {
	log.Fatal(err)
}
fmt.Printf("%x\n", digest) // prints 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b

But when I try to use it with ArrayOf and scan a bytea[], it fails instead of printing two lines as expected:

var digests pq.ArrayOf[SHA256Digest]
if err := db.QueryRow(`SELECT ARRAY['\x01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b'::bytea, '\xb5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'::bytea]`).Scan(&digests); err != nil {
	log.Fatal(err)
}
for _, digest := range digests {
	fmt.Printf("%x\n", digest)
}

It fails because Scan is called with a 66 byte []byte containing \x followed by 64 hex characters, rather than 32 raw bytes.

If I change Scan like this, then it works with ArrayOf:

 func (digest *SHA256Digest) Scan(src any) error {
 	switch src := src.(type) {
 	case []byte:
-		if len(src) != len(digest) {
-			return fmt.Errorf("cannot scan SHA256Digest from %d bytes (%d bytes expected)", len(src), len(digest))
-		}
-		copy(digest[:], src)
-		return nil
+		_, err := hex.Decode(digest[:], src[2:])
+		return err
 	case nil:
 		return errors.New("cannot scan SHA256Digest from NULL column")
 	default:
 		return fmt.Errorf("cannot convert column of type %T to SHA256Digest", src)
 	}
 }

But now it can't scan a non-array bytea anymore.

I could define two different types, SHA256Digest and SHA256DigestArrayItem, each with a different Scan implementation, but ideally I could use a single type and Scan could distinguish whether it's being called with raw bytes or a hex string. For example:

 func (digest *SHA256Digest) Scan(src any) error {
 	switch src := src.(type) {
 	case []byte:
		if len(src) != len(digest) {
			return fmt.Errorf("cannot scan SHA256Digest from %d bytes (%d bytes expected)", len(src), len(digest))
		}
		copy(digest[:], src)
		return nil
	case string:
		hexString, ok := strings.CutPrefix(src, `\x`)
		if !ok {
			return fmt.Errorf("cannot scan SHA256 from string without \\x prefix")
		}
		rawBytes, err := hex.DecodeString(hexString)
		if err != nil {
			return fmt.Errorf("cannot scan SHA256 from this string because it contains invalid hex: %s", err)
		}
		if len(rawBytes) != len(digest) {
			return fmt.Errorf("cannot scan SHA256 from %d bytes (%d bytes expected)", len(rawBytes), len(digest))
		}
		copy(digest[:], rawBytes)
		return nil
	case nil:
		return errors.New("cannot scan SHA256Digest from NULL column")
	default:
		return fmt.Errorf("cannot convert column of type %T to SHA256Digest", src)
	}
 }

My suggestion to pass a string to convertAssign instead of a []byte would enable this, but maybe there's a better way?

[arp242]

Ehm, yes, you are right – I had my mental model set to another project I worked on and I got confused. Thanks for explaining.

When I tried to just change it to a string this afternoon some other tests broke, I didn't look in to why though – may be a simple fix (or not).

Essentially the issue can be summarised as "make sure a Scan() method behaves identical for both regular (non-array) and array cases" – it doesn't matter if it's a string or []byte as such, it matters that they're different.

[AGWA]

That is a correct summary.

When I tried changing this a few weeks ago, only one test broke and it was trivial (it was looking for []uint8 in an error message instead of string) but I see you've made some changes since then so I will take another look.

[AGWA]

It's still just the one test. Here's the complete diff:

diff --git a/array.go b/array.go
index db10c49..478628b 100644
--- a/array.go
+++ b/array.go
@@ -113,7 +113,7 @@ func (a *ArrayOf[T]) scan(src []byte) error {
                                continue // Just use zero value
                        }
 
-                       err := convertAssign(&b[i], v)
+                       err := convertAssign(&b[i], string(v))
                        if err != nil {
                                return fmt.Errorf("pq: array index %d: %s", i, strings.TrimPrefix(err.Error(), "sql/driver: "))
                        }
diff --git a/array_test.go b/array_test.go
index 0c32087..4fc7053 100644
--- a/array_test.go
+++ b/array_test.go
@@ -590,7 +590,7 @@ func TestArrayOf(t *testing.T) {
 
                {&ArrayOf[int]{}, `{1,NULL,2}`, `array index 1: cannot convert NULL to int`},
                {&ArrayOf[string]{}, `{"a",NULL,"b"}`, `array index 1: cannot convert NULL to string`},
-               {&ArrayOf[int]{}, `{"asd"}`, `array index 0: converting driver.Value type []uint8 ("asd") to a int: invalid syntax`},
+               {&ArrayOf[int]{}, `{"asd"}`, `array index 0: converting driver.Value type string ("asd") to a int: invalid syntax`},
                {&ArrayOf[time.Time]{}, `{"2020-02-03 19:20:21Z",NULL,"2021-02-03 19:20:21Z"}`, `array index 1: cannot convert NULL to time.Time`},
                {&ArrayOf[time.Time]{}, `{"asd"}`, `array index 0: invalid timestamp`},
        }