[BACK-815] Implement datum update with historical tracking #579
Conversation
There was a problem hiding this comment.
This PR needs some more work:
- We ought to prevent changing certain fields of datum, at the very least we need to ensure the
idand thetypefields do not change as updates are made - I'd check with product and the clients of the API (i.e. Tapani, Darin and Gerrit) that it's ok to make all types revisionable
- In the Jira ticket comments, Darin brought up the question how to handle deletions. We should probably implement tombstone records
- The query which fetches datum by id is not indexed, which will result in a full collection scan. This won't work, because the production database has over 7B records. I left an inline comment with a suggestion.
- I'd like to understand better how this functionality affects deduplication. I think we should schedule a meeting with Darin to discuss this
- I left few inline comments how to improve the code quality
- The implementation ought to be unit tested
| return d.History | ||
| } | ||
|
|
||
| func (d *Datum) SetHistory(history *[]interface{}) { |
There was a problem hiding this comment.
I think you should add an implementation similar to the other methods here
| @@ -51,6 +51,7 @@ type Base struct { | |||
| DeviceTime *string `json:"deviceTime,omitempty" bson:"deviceTime,omitempty"` | |||
| GUID *string `json:"guid,omitempty" bson:"guid,omitempty"` | |||
| ID *string `json:"id,omitempty" bson:"id,omitempty"` | |||
| History *[]interface{} `json:"history,omitempty" bson:"history,omitempty"` | |||
There was a problem hiding this comment.
This makes absolutely all data types are revisionable - this is a big change to the data model and we probably need to confirm this change is ok with Gerrit (who works on Uploader) and Darin (who works on Loop).
Another option might be to restrict this only to certain data types or even to certain fields (e.g. to the fields of the structs that embed Base). I'm pretty sure we want to disallow changing the id of a datum.
On a different note, I don't think the type here should be a pointer to a slice, because the empty value of a slice is nil. By using an interface we lose type safety. I believe the type here ought to be []data.Datum.
There ought to be validation logic that ensures the type of each entry in the history is valid and of the same type as the base type.
| structureValidator "github.com/tidepool-org/platform/structure/validator" | ||
| ) | ||
|
|
||
| func DataSetsDataUpdate(dataServiceContext dataService.Context) { |
There was a problem hiding this comment.
A big chunk of the code here is "business logic" and doesn't belong to the API layer. The api layer ought to ensure the request params and payload are valid and make sure the currently authenticated user is authorized. The rest of the logic ought to go into the business layer (e.g. in data/service/service/client.go).
| // and its history get flattened into a history array and added to the new entry | ||
| existingHistory := *dbDatum.GetHistory() | ||
| dbDatum.SetHistory(nil) | ||
| existingHistory = append(existingHistory, dbDatum) |
There was a problem hiding this comment.
We ought to ensure datums of a given type can only be replaced by datums of the same type. For example, the code here allows to have a datum with type food with one or many history values of type basal.
No description provided.