Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dmotylev opened this issue Nov 3, 2022 · 3 comments · May be fixed by bhshkh/google-cloud-go#4
Assignees
Labels
api: datastore Issues related to the Datastore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@dmotylev
Copy link

dmotylev commented Nov 3, 2022

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

@dmotylev dmotylev added the triage me I really want to be triaged. label Nov 3, 2022
@product-auto-label product-auto-label bot added the api: datastore Issues related to the Datastore API. label Nov 3, 2022
@codyoss codyoss changed the title datastore.put does not allow any non-empty value in the slice if the first value is empty with omitempty option datastore: datastore.put does not allow any non-empty value in the slice if the first value is empty with omitempty option Nov 3, 2022
@telpirion
Copy link
Contributor

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

@telpirion telpirion added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Nov 3, 2022
@telpirion telpirion assigned triplequark and unassigned telpirion Nov 4, 2022
@csilvers
Copy link

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

opts.omitEmpty = false
:

// 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
Copy link
Contributor

bhshkh commented Aug 30, 2024

Possible fix added in private repository bhshkh#4 . Needs more testing. Will pick up later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants