datastore: property length enforced for nested noindex fields on structs implementing PropertyLoadSaver

[someone1]

Client Datastore v1.3.0

Environment Go 1.13 AppEngine

Go Environment $ go version

go version go1.13.15 linux/amd64

$ go env

GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vscode/.cache/go-build"
GOENV="/home/vscode/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/workspace/project/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build520294866=/tmp/go-build -gno-record-gcc-switches"

Code

package main

import (
    "context"
    "log"
    "strings"

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

type B struct {
    Value string `datastore:"v"`
}

type A struct {
    B []B `datastore:"bs,noindex"`
}

func (b *B) Load(ps []datastore.Property) error {
    return datastore.LoadStruct(b, ps)
}

func (b *B) Save() ([]datastore.Property, error) {
    return datastore.SaveStruct(b)
}

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    ds, err := datastore.NewClient(ctx, "")
    if err != nil {
        log.Fatal(err)
    }

    m := &A{
        B: []B{
            {Value: strings.Repeat("a", 2000)},
        },
    }

    if _, err := ds.Put(ctx, datastore.IncompleteKey("test", nil), m); err != nil {
        log.Fatal(err)
    }

    log.Println("it worked!")
}

Expected behavior Logs output "It worked"

Actual behavior Output:

2020/10/01 22:48:05 datastore: datastore: string property too long to index for a Property with Name "v" at index 0 for a Property with Name "bs"
exit status 1

Additional context

Removing the PropertyLoadSaver implementation from B will work as expected, where the parent noindex option applies to the nested fields, despite their lack of the noindex option.

This only seems to affect nested structs, if I added noindex to B.Value and tried to save m.B[0] in the example above, it would not enforce the max length despite implementing the PropertyLoadSaver interface.

[tritone]

Thanks for your report!

@benwhitehead any thoughts on what we should do with this? Does the noindex tagging in the example look correct to you?

I'm also wondering if it's even worth having the client side validation at https://github.com/googleapis/google-cloud-go/blob/2cf2adb116492d7f7a743c75aa66f774b4aa71c8/datastore/save.go#L386 , if the API will also reject this.

[BenWhitehead]

If the api is performing the validation anyway, I'd prefer to defer to it since it is the ultimate authority and can take care of the nestedness and things since it has to store the values anyway.

[someone1]

Wanted to chime in that the package vs API error aside - the issue seems rooted in the fact that nested structs that implement the PropertyLoadSaver interface lose the noIndex detail along the way - are there other ramifications to this? Would the API know to ignore the noIndex option on nested properties where the parent has noIndex: true defined?

Doing some digging it seems that if a struct implements the PLS interface, then the saveOpts from the parent aren't propagated through since the package will just call Save without passing the options used by an potential parent like it otherwise would for structs that don't implement the PLS interface.

Would a simple fix be to do something similar to the flatten property and loop through the returned properties and properly set noIndex? If so, I could throw up a PR for this!

[someone1]

@tritone @BenWhitehead - I'd be happy to submit a PR with the above implemented - would this be the right approach?

[BenWhitehead]

I don't don't have the go knowledge to be able to do the review on my own. I'll defer to @tritone or @crwilcox if they have the bandwidth to help with the review.