datastore: flatten option validation not confined to field it is defined on

[evanwht]

Client

datastore

Environment

macOS

Go Environment

go1.17.2 darwin/amd64 cloud.google.com/go/datastore v1.6.0

Code

e.g.

package main

import (
	"context"
	"fmt"
	"time"

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

type CustomTime struct {
	T time.Time
}

type B struct {
	IDs []string
}

type S struct {
	A  CustomTime `datastore:",flatten"`
	Bs []B
}

func main() {
	ctx := context.Background()
	c, err := datastore.NewClient(ctx, "my-project")
	if err != nil {
		panic(err)
	}

	key, err := c.Put(ctx, datastore.IncompleteKey("s", nil), &S{})
	if err != nil {
		panic(err)
	}
	fmt.Println(key)
}

Expected behavior

The struct is saved with the CustomTime field flattened to the property "A.T" and Bs as it normally would

Actual behavior

I receive the error datastore: flattening nested structs leads to a slice of slices: field "IDs". However this field is not supposed to be flattened. If you switch the order of S's fields to have Bs first, then I no longer receive the error. Once the order is changed you can add any other struct fields after A that do not contain a slice of slices and they are correctly saved as non-flattened structs. This leads me to believe the validation for the flatten option is in error, not the actual flattening of fields.

[crwilcox]

Confirmed. Repro below, should be added to datastore_test.go. S0 Passes today, S1 fails with /datastore/datastore_test.go:3076: putMutations(1): datastore: flattening nested structs leads to a slice of slices: field "IDs"

func TestPutMultiTypes_Flatten(t *testing.T) {
	ctx := context.Background()
	c := newTestClient(ctx, t)
	defer c.Close()

	type CustomTime struct {
		T time.Time
	}

	type B struct {
		IDs []string
	}

	type S0 struct {
		Bs []B
		A  CustomTime `datastore:",flatten"`
	}

	type S1 struct {
		A  CustomTime `datastore:",flatten"`
		Bs []B
	}

	structs := []interface{}{&S0{}, &S1{}}

	for k, s := range structs {
		_, err := putMutations([]*Key{IncompleteKey("s", nil)}, []interface{}{s})
		if err != nil {
			t.Fatalf("putMutations(%v): %v", k, err)
		}
	}
}

[crwilcox]

I have added tests that show the current behavior. I think this is might not be an error. I do understand it is odd that it works in one order but not the other, but it does, in fact, work in one direction.

I am considering this a feature request as we did protect against this intentionally. I don't have context for why, but this is certainly an intentional omission.

[evanwht]

@crwilcox Thanks for looking into it. It does seem like an odd feature to include. It makes me wonder if there is a bug that is being accounted for where sub fields of an array field get flattened inadvertently if a previous field was flattened. Whatever it is, I have worked around this for now but will follow along for updates.

[crwilcox]

. It makes me wonder if there is a bug that is being accounted for where sub fields of an array field get flattened inadvertently if a previous field was flattened.

I think this accurately captures what is going on. Also, while trying to understand I found our test for this was using a struct with defaults, and that could be flattened safely so there is now as well. In the code for walking children objects seems suspect, as the 'flatten' flag is persisted over fields. However, undoing that led to some even stranger side effects.

[evanwht]

@dmahugh Any indication on when this might be looked at?