From 27434586aedd4cb84ec5c082852cfbb3992e83fd Mon Sep 17 00:00:00 2001 From: Bill Katz Date: Tue, 13 Aug 2024 00:00:13 -0400 Subject: [PATCH] fix _user issues with update neuronjson --- datatype/neuronjson/neuronjson.go | 59 ++++++++++++++++++++---- datatype/neuronjson/neuronjson_test.go | 63 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 8 deletions(-) diff --git a/datatype/neuronjson/neuronjson.go b/datatype/neuronjson/neuronjson.go index 025ad242..bcaf4463 100644 --- a/datatype/neuronjson/neuronjson.go +++ b/datatype/neuronjson/neuronjson.go @@ -49,6 +49,13 @@ backward-compatibility with clients that used to use the keyvalue datatype for these neuron JSON annotations. The values are assumed to be JSON data, and the queries are similar to how Firestore handles queries. +Any keys that end with _user or _time are considered metadata in describing +by who and at what time that field's value was modified. So you must not +create JSON field names ending with _user or _time unless they are used +as metadata. +Example: Do not use "my_user" as a field name. "myuser" or "myUser" is OK + and will have "myuser_user" and "myuser_time" fields auto-generated. + Note: UUIDs referenced below are strings that may either be a unique prefix of a hexadecimal UUID string (e.g., 3FA22) or a branch leaf specification that adds a colon (":") followed by the case-dependent branch name. In the case of a @@ -333,7 +340,7 @@ POST /node///key/ Updates a key-value pair, modifying the fields with the POSTed JSON fields. Note that unlike POST /key in keyvalue datatype instances, this operation updates - fields by defaults (using old fields not overwritten) rather than replacing + fields by default (using old fields not overwritten) rather than replacing the entire annotation. The replace behavior can be explicitly set if desired to match old keyvalue semantics. @@ -1256,20 +1263,59 @@ func (d *Data) GetData(ctx storage.VersionedCtx, keyStr string, fieldMap map[str return d.GetByBodyID(ctx, bodyid, fieldMap, showFields) } +func isMetaField(field string) bool { + return strings.HasSuffix(field, "_user") || strings.HasSuffix(field, "_time") +} + // update _user and _time fields for any fields newly set or modified. func updateJSON(origData, newData NeuronJSON, user string, conditionals []string, replace bool) { t := time.Now() timeStr := t.Format(time.RFC3339) + // keep track of explicitly set _user and _time fields for each field + // where a null would be empty string + fieldUser := make(map[string]string) + fieldTime := make(map[string]string) + var notString bool + for field, value := range newData { + if strings.HasSuffix(field, "_user") { + fieldUser[field[:len(field)-5]], notString = value.(string) + if notString { + dvid.Errorf("bad field %q value (not string): %v\n", field, value) + } + } else if strings.HasSuffix(field, "_time") { + fieldTime[field[:len(field)-5]], notString = value.(string) + if notString { + dvid.Errorf("bad field %q value (not string): %v\n", field, value) + } + } + } + // remove fields that are being set to null deleted_fields := make(map[string]struct{}) for field, value := range newData { if value == nil { deleted_fields[field] = struct{}{} delete(newData, field) - newData[field+"_user"] = user - newData[field+"_time"] = timeStr + dvid.Infof("deleting field %q\n", field) + if !isMetaField(field) { + // allow _user and _time fields to be set explicitly + setUser, foundUser := fieldUser[field] + setTime, foundTime := fieldTime[field] + if !foundUser { + setUser = user + } + if !foundTime { + setTime = timeStr + } + if setUser != "" { + newData[field+"_user"] = setUser + } + if setTime != "" { + newData[field+"_time"] = setTime + } + } delete(origData, field) } } @@ -1286,13 +1332,10 @@ func updateJSON(origData, newData NeuronJSON, user string, conditionals []string } } else { for field, value := range newData { - if origValue, found := origData[field]; !found || !reflect.DeepEqual(value, origValue) { + if origValue, found := origData[field]; !found || isMetaField(field) || !reflect.DeepEqual(value, origValue) { newlySet[field] = struct{}{} - if strings.HasSuffix(field, "_user") { // setting _user means we should set _time - newlySet[field[:len(field)-5]] = struct{}{} - } } - if !strings.HasSuffix(field, "_user") && !strings.HasSuffix(field, "_time") { + if !isMetaField(field) { newFields[field] = struct{}{} } } diff --git a/datatype/neuronjson/neuronjson_test.go b/datatype/neuronjson/neuronjson_test.go index 68fc99cb..3794d78a 100644 --- a/datatype/neuronjson/neuronjson_test.go +++ b/datatype/neuronjson/neuronjson_test.go @@ -658,6 +658,69 @@ func TestValidation(t *testing.T) { } } +func TestKeyvalueUser(t *testing.T) { + if err := server.OpenTest(); err != nil { + t.Fatalf("can't open test server: %v\n", err) + } + defer server.CloseTest() + + uuid, _ := initTestRepo() + name := dvid.InstanceName("annotations") + + config := dvid.NewConfig() + _, err := datastore.NewData(uuid, jsontype, name, config) + if err != nil { + t.Fatalf("Error creating new neuronjson instance: %v\n", err) + } + + // PUT a kv + value := `{"bodyid": 1000, "key1": "foo"}` + req := fmt.Sprintf("%snode/%s/%s/key/1000?u=frank", server.WebAPIPath, uuid, name) + server.TestHTTP(t, "POST", req, strings.NewReader(value)) + + // Change key1 value but force a user. + value = `{"bodyid": 1000, "key1": "moo", "key1_user": "frank"}` + req = fmt.Sprintf("%snode/%s/%s/key/1000?u=admin", server.WebAPIPath, uuid, name) + server.TestHTTP(t, "POST", req, strings.NewReader(value)) + + // Verify the key1_user is now "someone" and not "frank" + req = fmt.Sprintf("%snode/%s/%s/key/1000?show=user", server.WebAPIPath, uuid, name) + returnValue := server.TestHTTP(t, "GET", req, nil) + if !equalObjectJSON(returnValue, []byte(value), ShowUsers) { + t.Errorf("Error: expected %s, got %s\n", value, string(returnValue)) + } + + // Allow deletion of fields via null for non-replace POST + value = `{"bodyid": 1000, "key1": null, "key1_user": null}` + req = fmt.Sprintf("%snode/%s/%s/key/1000?u=deleter", server.WebAPIPath, uuid, name) + server.TestHTTP(t, "POST", req, strings.NewReader(value)) + + // Verify the both key1 and key1_user fields are deleted + req = fmt.Sprintf("%snode/%s/%s/key/1000?show=user", server.WebAPIPath, uuid, name) + returnValue = server.TestHTTP(t, "GET", req, nil) + if !equalObjectJSON(returnValue, []byte(`{"bodyid": 1000}`), ShowUsers) { + t.Errorf("Error: expected %s, got %s\n", `{"bodyid": 1000}`, string(returnValue)) + } + + // PUT back the kv + value = `{"bodyid": 1000, "key1": "foo"}` + req = fmt.Sprintf("%snode/%s/%s/key/1000?u=frank", server.WebAPIPath, uuid, name) + server.TestHTTP(t, "POST", req, strings.NewReader(value)) + + // Just delete the main field not the _user as well. + value = `{"bodyid": 1000, "key1": null}` + req = fmt.Sprintf("%snode/%s/%s/key/1000?u=deleter", server.WebAPIPath, uuid, name) + server.TestHTTP(t, "POST", req, strings.NewReader(value)) + + // Verify we deleted key1 but also recorded the deleter identity + req = fmt.Sprintf("%snode/%s/%s/key/1000?show=user", server.WebAPIPath, uuid, name) + returnValue = server.TestHTTP(t, "GET", req, nil) + expected := `{"bodyid": 1000, "key1_user": "deleter"}` + if !equalObjectJSON(returnValue, []byte(expected), ShowUsers) { + t.Errorf("Error: expected %s, got %s\n", expected, string(returnValue)) + } +} + func TestKeyvalueRequests(t *testing.T) { if err := server.OpenTest(); err != nil { t.Fatalf("can't open test server: %v\n", err)