googleapis / googleapis/google-cloud-go

datastore: LoadStruct failing on unrecognized properties makes roll backs unsafe

Open
#1,372 6 comments 5 reactions 1 assignee Assigned to @bhshkh View on GitHub
api: datastore type: feature request
Dominant language
Go
Stars
4.5k
Forks
1.6k
Avg merge
1d 13h
Merged PRs (30d)
109

Description

The behavior of `LoadStruct` is to report an error if the datastore row has any unrecognized properties. This is particularly problematic since it means that roll backs of a service that uses datastore is inherently unsafe.

The problematic scenario:
1. Add a field to an entity.
2. Even if we're not using the new field yet, `SaveStruct` still encodes them into properties with the field type's zero value.
3. Deploy new service. This will start adding the new field to some entities in datastore as they are written.
4. Roll back the service.
5. When we try to read a row that was written by the newer version (during the time it was running, before the roll back), an error occurs because the row has an unrecognized property.

This means that adding fields to an entity effectively poisons the datastore in a way that prevents roll backs.

I understand the behavior, since it does feel like an error that we could be dropping data (though, TBH, if `encoding/json` is okay throwing away data, perhaps so should this library so that it's using familiar idioms/semantics).

The bigger problem, in my opinion, is that the APIs do not provide any good way to work around the issue or safely continue processing. The current way to detect this case is to try to type-assert the returned error to `*datastore.ErrFieldMismatch` and then see if the `Reason` field is `"no such struct field"`. Relying on a particular reason string feels particularly brittle. Worse: there is no way to know what properties were unrecognized since `LoadStruct` *only reports the last error*. So if there were multiple unrecognized properties, only the last gets reported to the client. It would be far better if `ErrFieldMismatch` had an `Unknown []Property` field which could be inspected, to gather *all* unknown properties.

I have worked around this by embedding a sort of "mixin" type into every datastore entity struct in my application. However, in order to make this safe and work correctly, I had to *fork* this client library. My fork does exactly as described above: adds a `Unknown []Property` field to the `ErrFieldMismatch` type and changes `structPLS.Load` to record *all* unrecognized properties there. That way the entity struct can actually *retain* these unrecognized properties and add them to the set of saved properties, in the event the entity is written back to datastore. That way the application does not blow up on unknown properties, but also won't lose them, if an entity is read/updated/written in a transaction.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.