datastore: datastore.put does not allow any non-empty value in the slice if the first value is empty with `omitempty` option

[dmotylev]

Client

Datastore

Environment

MacOS

Go Environment

$ go version

go version go1.19.2 darwin/arm64

$ go env

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/dmotylev/Library/Caches/go-build"
GOENV="/Users/dmotylev/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/dmotylev/go/pkg/mod"
GONOPROXY="github.com/redsift/*"
GONOSUMDB="github.com/redsift/*"
GOOS="darwin"
GOPATH="/Users/dmotylev/go"
GOPRIVATE="github.com/redsift/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.19.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.19.2/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.19.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/dmotylev/Workshop/redsift/smart/go.mod"
GOWORK="/Users/dmotylev/Workshop/redsift/smart/go.work"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/r4/lm381q1x3mq60xmkmxz5z1l00000gn/T/go-build743168484=/tmp/go-build -gno-record-gcc-switches -fno-common"

Code

package main

import (
    "context"
    "fmt"

    "cloud.google.com/go/datastore"
)

const (
    projectId = "SOME"
)

type T struct {
    Values []string `datastore:",noindex,omitempty" json:"values,omitempty"`
}

func test(client *datastore.Client, key string, want T) {
    k := datastore.NameKey("test", key, nil)

    fmt.Println("\nTEST", key)

    _, err := client.Put(context.TODO(), k, &want)
    if err != nil {
        fmt.Printf("datastore.Put(%s) %s\n", key, err)
        return
    }

    var got T
    err = client.Get(context.TODO(), k, &got)

    if err != nil {
        fmt.Printf("datastore.Get(%s) %s\n", key, err)
        return
    }

    fmt.Printf("want: %#v\n got: %#v\n", want, got)
}

func main() {
    client, err := datastore.NewClient(context.TODO(), projectId)
    if err != nil {
        panic(err)
    }

    test(client, "empty-strings", T{Values: []string{"", "", ""}})
    test(client, "non-empty-strings", T{Values: []string{"s0", "s1", "s2"}})
    test(client, "1st-item-is-empty-string", T{Values: []string{"", "s1", "s2"}})
    test(client, "2nd-item-is-empty-string", T{Values: []string{"s0", "", "s2"}})

    // Output:
    // TEST empty-strings
    // want: main.T{Values:[]string{"", "", ""}}
    // got: main.T{Values:[]string(nil)}
    //
    // TEST non-empty-strings
    // want: main.T{Values:[]string{"s0", "s1", "s2"}}
    // got: main.T{Values:[]string{"s0", "s1", "s2"}}
    //
    // TEST 1st-item-is-empty-string
    // datastore.Put(1st-item-is-empty-string) datastore: unexpected property "Values" in elem 1 of slice
    //
    // TEST 2nd-item-is-empty-string
    // want: main.T{Values:[]string{"s0", "", "s2"}}
    // got: main.T{Values:[]string{"s0", "s2"}}
}

Expected behavior

The datastore put method should allow the mixing of empty and non-empty values in the array in any order.

Actual behavior

It does not allow any non-empty value if the first value is empty.

Additional context

Tested with version 1.0.0 and 1.9.0

P.S.

Not saving empty slice is expected behaviour of omitempty, but removing empty values from the slice is not. Perhaps it is an another issue

[telpirion]

(Wow, great repro snippet @dmotylev . Thanks!)

[csilvers]

Not saving empty slice is expected behaviour of omitempty, but removing empty values from the slice is not. Perhaps it is an another issue

I agree that this is a separate issue, and it has bitten us at Khan Academy as well. I see this code in https://github.com/googleapis/google-cloud-go/blob/9799e024c6e12a7df694fe68f3f0a539d519ee1b/datastore/save.go#L120:

// When we recurse on the derefenced pointer, omitempty no longer applies:
// we already know the pointer is not empty, it doesn't matter if its referent
// is empty or not.
opts.omitEmpty = false

It seems to me that the same logic should apply to slices: omitempty on a slice means (to me anyway) "do not store the slice if it's the empty slice", not "discard all zero values inside the slide." So the code should be setting opts.omitEmpty in saveSliceProperty.

(Of course, that's a breaking change and may not be safe to make at this point. :-( )

[bhshkh]

Possible fix added in private repository https://github.com/bhshkh/google-cloud-go/pull/4 . Needs more testing. Will pick up later