Closed fredr closed 7 years ago
@adams-sarah looks to be related to the nested entity work, would you mind taking a look?
Yes. I'll look at this today. Thanks for the heads up @adamtanner.
@adams-sarah It looks like GH didn't auto-close this when you pushed your commit. Okay to close? :grin:
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:
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.
cc @zombiezen
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.GeoPoint
s 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.
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. :+1:
@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?
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. :/
Came up with a clean solution. PR #52
So, is this a regression, or were embedded types never properly deserialized?
Regression.
It is only the special types mentioned above which are affected. time.Time
, appengine.GeoPoint
, and any redeclared, non-struct type, eg. type MyInt int
.
These embedded fields have always been stored in codec as "" by mistake (I can only assume). Nested field names were joined by ".". So embedded field as the leaf would be "A" + "." + "B" + "." + "" == "A.B." :(.
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:
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 (267c27e7492265b84fc6719503b14a1e17975d79) and works fine, the props will be (notice the ending dot in EmbeddedTime):
Updating the appengine packages to the latest will produce this error: