google-cloud-go icon indicating copy to clipboard operation
google-cloud-go copied to clipboard

datastore: PropertyLoadSaver not working correctly with nil map type on Put operations

Open ccontavalli opened this issue 3 years ago • 1 comments

This is likely related to the fix for issue #2611, @IlyaFaer

Client

datastore

Env

Firestore on datastore mode linux, using bazel

Go env

go 1.15.14 cloud.google.com/go/datastore v1.5.0

Code

As per #2611:

package main

import(
	"fmt"
	"cloud.google.com/go/datastore"
)

type Map map[int]int

func (_ *Map) Load(_ []datastore.Property) error {
	return nil
}

func (_ *Map) Save() ([]datastore.Property, error) {
	return []datastore.Property{}, nil
}

type Struct struct {
	Map Map
}

func main() {
	s := &Struct{}
	fmt.Println(datastore.SaveStruct(s))
}

After the fix in #2611 the code above works. However, if you try to actually store s into datastore:

ds.Put(context.Background(), key, s)

ds.Put will fail with an error like datastore: invalid Value type []datastore.Property for a Property with Name "Map".

If Map is non null, with:

s := &Struct{Map: Map{}}

the Put command will then succeed.

So, a nil map implementing PropertyLoadSaver causes Put to fail with a cryptic message. A non-nil map works.

Note that according to the manual:

An entity's contents can also be represented by any type that implements the PropertyLoadSaver interface. This type may be a struct pointer, but it does not have to be.

So, nil or non-nil, once the PropertyLoadSaver interfce is implemented, the type should work.

Note also that for a normal struct, the struct pointer can be nil or non nil, and Put will work either way, but not for a custom type backed by a map.

I suspect that propertiesToProto needs to be updated to handle this cleanly? Similarly to the referenced PR.

ccontavalli avatar Aug 22 '21 11:08 ccontavalli

(fixed a few minor errors in my original text) @crwilcox any update? need help reproducing the problem?

ccontavalli avatar Sep 15 '21 18:09 ccontavalli

@ccontavalli,

ds.Put(context.Background(), key, s) no longer fails. Please confirm and close the issue.

bhshkh avatar Jun 06 '23 10:06 bhshkh