GODRIVER-1765 Improve JSON marshaling/unmarshaling for bson.D, bson.M and bson.A. #1594
Conversation
qingyang-hu
force-pushed
the
godriver1765
branch
from
e3e35d4
to
bbfb128
Compare
API Change Report./bson/primitivecompatible changes(*A).UnmarshalJSON: added |
bson/primitive/primitive.go
Outdated
| dec := json.NewDecoder(bytes.NewReader(b)) | ||
| t, err := dec.Token() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if t == nil { | ||
| *d = nil | ||
| return nil | ||
| } | ||
| if v, ok := t.(json.Delim); !ok || v != '{' { | ||
| return &json.UnmarshalTypeError{ | ||
| Value: tokenString(t), | ||
| Type: reflect.TypeOf(*d), | ||
| Offset: dec.InputOffset(), | ||
| } | ||
| } | ||
| *d, err = jsonDecodeD(dec) | ||
| return err |
There was a problem hiding this comment.
[Optional] You could combine the logic in D.UnmarshalJSON and m.UnmarshalJSON using generics:
func newJsonObjDecoder[T D | M](b []byte, val *T) (*json.Decoder, error) {
dec := json.NewDecoder(bytes.NewReader(b))
t, err := dec.Token()
if err != nil {
return nil, err
}
if t == nil {
*val = nil
return nil, nil
}
if v, ok := t.(json.Delim); !ok || v != '{' {
return nil, &json.UnmarshalTypeError{
Value: tokenString(t),
Type: reflect.TypeOf(*val),
Offset: dec.InputOffset(),
}
}
return dec, nil
}
| } | ||
| } | ||
| buf.Write([]byte("}")) | ||
| return json.RawMessage(buf.Bytes()).MarshalJSON() |
There was a problem hiding this comment.
What's the purpose of returning the bytes via json.RawMessage().MarshalJSON here rather than returning them directly (i.e. return buf.Bytes(), nil)?
There was a problem hiding this comment.
Calling MarshalJSON() is to handle functions such as func (*Encoder) SetIndent correctly as tested at https://github.com/mongodb/mongo-go-driver/pull/1594/files#diff-1649d029623b36c04e8a0636bd65aab5bb1047410b1affb26206198a34a4c784R260-R263
| b, _ := json.Marshal(tc.test) | ||
| var got D | ||
| err := json.Unmarshal(b, &got) |
There was a problem hiding this comment.
Testing that JSON unmarshaling works with a document marshaled by the custom JSON marshaler can hide bugs in the custom JSON unmarshaling logic. For example, if the custom JSON marshaling code always uses a semicolon instead of a colon for a key/value separator, the JSON unmarshaling code could incorrectly expect a semicolon separator, but the tests would still pass.
We should use a known-correct JSON document (e.g. a string literal, embedded string from a text doc, or generated with a trusted reference) to test the JSON unmarshaling logic. That recommendation applies to all Unmarshal tests, but is most relevant here because both marshaling and unmarshaling are custom.
| } | ||
| } | ||
|
|
||
| func TestUnmarshalingD(t *testing.T) { |
There was a problem hiding this comment.
Optional: Consider a more descriptive test name (borrowed from testable examples name conventions).
| func TestUnmarshalingD(t *testing.T) { | |
| func TestD_UnmarshalJSON(t *testing.T) { |
| @@ -199,3 +200,334 @@ func TestDateTime(t *testing.T) { | |||
| }) | |||
| }) | |||
| } | |||
|
|
|||
| func TestMarshalingD(t *testing.T) { | |||
There was a problem hiding this comment.
Optional: Consider a more descriptive test name (borrowed from testable examples name conventions).
| func TestMarshalingD(t *testing.T) { | |
| func TestD_MarshalJSON(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestUnmarshalingA(t *testing.T) { |
There was a problem hiding this comment.
Optional: Consider a more descriptive test name (borrowed from testable examples name conventions).
| func TestUnmarshalingA(t *testing.T) { | |
| func TestA_UnmarshalJSON(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestUnmarshalingM(t *testing.T) { |
There was a problem hiding this comment.
Optional: Consider a more descriptive test name (borrowed from testable examples name conventions).
| func TestUnmarshalingM(t *testing.T) { | |
| func TestM_UnmarshalJSON(t *testing.T) { |
| @@ -223,9 +265,201 @@ type E struct { | |||
| // bson.M{"foo": "bar", "hello": "world", "pi": 3.14159} | |||
| type M map[string]interface{} | |||
|
|
|||
| // UnmarshalJSON decodes M from JSON. | |||
| func (m *M) UnmarshalJSON(b []byte) error { | |||
There was a problem hiding this comment.
Why do we need to define custom JSON unmarshaling logic for bson.M? Other than the type names being different, the JSON unmarshaling logic seems identical.
There was a problem hiding this comment.
The custom unmarshaling for bson.M and bson.A is to handle nested structures correspondingly rather than simply unmarshal into interface{} as case tested at https://github.com/mongodb/mongo-go-driver/pull/1594/files#diff-1649d029623b36c04e8a0636bd65aab5bb1047410b1affb26206198a34a4c784R374-R381
| // An A is an ordered representation of a BSON array. | ||
| // | ||
| // Example usage: | ||
| // | ||
| // bson.A{"bar", "world", 3.14159, bson.D{{"qux", 12345}}} | ||
| type A []interface{} | ||
|
|
||
| // UnmarshalJSON decodes A from JSON. | ||
| func (a *A) UnmarshalJSON(b []byte) error { |
There was a problem hiding this comment.
Why do we need to define custom JSON unmarshaling logic for bson.A? Other than the type names being different, the JSON unmarshaling logic seems identical.
GODRIVER-1765
Summary
This PR adds
json.Marshalerandjson.Unmarshalerforprimitive.D, so it can handle JSON objects properly.The PR also adds
json.Unmarshalers forprimitive.Mandprimitive.A, so JSON strings can be unmarshaled into nested structures correspondingly.After the "primitive" merging into "bson" package, we can migrate these
primitivemethods to theirbsontypes.