datastore: field name cannot end with '.'

[fredr]

I'm trying to update to the latest appengine pkg in our project (from a very old version), but running in to problems with our PropertyLoadSaver.

When embedding a type like this:

type SpecialTime struct {
	time.Time
}

and then using it in an entity saved in datastore, the datastore field will end with a dot. Which is not allowed due to this loc.

Here is a minimal repro: https://github.com/fredr/datastoreloadsavetest
it uses our current version of the appengien pkg (267c27e) and works fine, the props will be (notice the ending dot in EmbeddedTime):

[{Name:EmbeddedTime. Value:2016-12-19 10:49:23.280860142 +0000 UTC NoIndex:false Multiple:false} {Name:NormalTime Value:2016-12-19 10:49:23.280860121 +0000 UTC NoIndex:false Multiple:false}]

Updating the appengine packages to the latest will produce this error:

datastore: cannot load field "EmbeddedTime." into a "appenginetest.TestEntity": field name cannot end with '.'

[admtnnr]

@adams-sarah looks to be related to the nested entity work, would you mind taking a look?

[s-mang]

Yes. I'll look at this today. Thanks for the heads up @adamtanner.

[admtnnr]

@adams-sarah It looks like GH didn't auto-close this when you pushed your commit. Okay to close? 😁

[s-mang]

Shoot, sorry. I got sidetracked.
This is the bug I mentioned to you a few weeks back, and never got around to coming up with a good solution. The fix which references this issue never went in, I closed the CL. I discovered a bunch of edge cases.

The CL I pushed changed the naming behavior for anonymous struct fields to just eliminate the "." at the end. There are a few problems with this:

1. the appengine/datastore client will still not know how to handle any data already in datastore with a field name that ends with "."
2. the change will break compatibility between cloud.google.com/go/datastore and this package (as cloud pkgs will serialize field differently)
3. (this was already an issue, but still) you can only have one anonymous field per struct, as they will all have the same field name.

What I'd like to do at this point is to just name an anonymous struct field after it's type.
So the example above would give:
EmbeddedTime.Time
as the field name, instead of
EmbeddedTime..

What do you think @adamtanner ? This sill breaks the issues 1. and 2. I raised above.

[s-mang]

cc @zombiezen

[s-mang]

Ok @zombiezen and I have agreed we're going to move forward with this solution. EmbeddedTime.Time instead of EmbeddedTime..

It is also important to note that this bug only affects embedded time.Time and appengine.GeoPoints and redeclared builtin types, eg type MyInt int. In general, embedded structs work as expected.

@adamtanner LMK if you have any issues with this. I'm going to move forward with the CL.

[admtnnr]

Is this going to make it impossible to deserialize existing data in Datastore that happens to have one of these embedded types? If so, that's probably a blocker. Otherwise I'm completely onboard with this solution. 👍

[s-mang]

@adamtanner yes, that's correct :(. With this solution, datastore pkg won't be able to deserialize entities already in datastore with one of these embedded types.
I can't think up another reasonable, correct solution.
We could do something hacky.. preserving the old, incorrect behavior. eg. on load, if field name ends with a dot, load the value into the last anonymous field on the struct.

Or.. revert the entity values change all together?

WDYT?

[fredr]

This will be a very big problem for us if you guys decide to break backwards compatibility. All of our dates that are currently stored, are stored as an embedded struct, so we would never be able to update our appengine packages. :/

[s-mang]

Came up with a clean solution. PR #52

[broady]

So, is this a regression, or were embedded types never properly deserialized?