googleapis / googleapis/google-cloud-go
datastore: LoadStruct failing on unrecognized properties makes roll backs unsafe
- 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
Assessment
This issue has not been assessed yet.