Skip to content

GODRIVER-3081 Fix zero value detection for empty slices and maps. #1514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 1, 2024
Merged

GODRIVER-3081 Fix zero value detection for empty slices and maps. #1514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d0128c
Fix zero value detection for empty slice and map.
qingyang-hu Jan 9, 2024
a561125
update function name, doc file, and spelling.
qingyang-hu Jan 24, 2024
1d44b78
Merge branch 'master' into godriver3081
qingyang-hu Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions bson/bsoncodec/struct_codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,17 @@ func (sc *StructCodec) EncodeValue(ec EncodeContext, vw bsonrw.ValueWriter, val

encoder := desc.encoder

var zero bool
var empty bool
if cz, ok := encoder.(CodecZeroer); ok {
zero = cz.IsTypeZero(rv.Interface())
empty = cz.IsTypeZero(rv.Interface())
} else if rv.Kind() == reflect.Interface {
// isZero will not treat an interface rv as an interface, so we need to check for the
// zero interface separately.
zero = rv.IsNil()
// isEmpty will not treat an interface rv as an interface, so we need to check for the
// nil interface separately.
empty = rv.IsNil()
} else {
zero = isZero(rv, sc.EncodeOmitDefaultStruct || ec.omitZeroStruct)
empty = isEmpty(rv, sc.EncodeOmitDefaultStruct || ec.omitZeroStruct)
}
if desc.omitEmpty && zero {
if desc.omitEmpty && empty {
continue
}

Expand Down Expand Up @@ -391,12 +391,15 @@ func (sc *StructCodec) DecodeValue(dc DecodeContext, vr bsonrw.ValueReader, val
return nil
}

func isZero(v reflect.Value, omitZeroStruct bool) bool {
func isEmpty(v reflect.Value, omitZeroStruct bool) bool {
kind := v.Kind()
if (kind != reflect.Ptr || !v.IsNil()) && v.Type().Implements(tZeroer) {
return v.Interface().(Zeroer).IsZero()
}
if kind == reflect.Struct {
switch kind {
case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reflect.String does not need to be included here, it's zero condition is already v.Len() == 0. See here.

To the documentation, I agree that relfect.Array, reflect.Map, and reflect.Slice should be compared to their lengths. However, I think the naming of the function isZero is pretty confusing, as is the description of omitempty:

omitempty: If the omitempty struct tag is specified on a field, the field will not be marshalled if it is set to the zero value. Fields with language primitive types such as integers, booleans, and strings are considered empty if their value is equal to the zero value for the type (i.e. 0 for integers, false for booleans, and "" for strings). Slices, maps, and arrays are considered empty if they are of length zero. Interfaces and pointers are considered empty if their value is nil. By default, structs are only considered empty if the struct type implements the bsoncodec.Zeroer interface and the IsZero method returns true. Struct fields whose types do not implement Zeroer are never considered empty and will be marshalled as embedded documents. NOTE: It is recommended that this tag be used for all slice and map fields.

What are your thoughts on naming this function isEmpty and updating the documentation for omitempty to reflect that we are checking our own definition of "empty" and not the rigid Go definition of "IsZero":

omitempty: If the omitempty struct tag is specified on a field, the field will not be marshaled if it is set to an "empty" value. Integers, booleans, and strings are considered empty if their value is equal to the zero value for the type (i.e. 0 for integers, false for booleans, and "" for strings). Slices, maps, and arrays are considered empty if they are of length zero. Interfaces and pointers are considered empty if their value is nil. By default, structs are only considered empty if the struct type implements the bsoncodec.Zeroer interface and the IsZero method returns true. Struct fields whose types do not implement Zeroer are never considered empty and will be marshaled as embedded documents. NOTE: It is recommended that this tag be used for all slice and map fields.

I don't think there is a simple way around the bsoncore.Zeroer without a major version.

CC: @matthewdale

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this reads better in the code, but I also understand deferring this for further discussion:

		var empty bool
		if cz, ok := encoder.(CodecZeroer); ok {
			empty = cz.IsTypeZero(rv.Interface
		} else if rv.Kind() == reflect.Interface {
			empty = rv.IsNil()
		} else {
			empty = isEmpty(rv, sc.EncodeOmitDefaultStruct || ec.omitZeroStruct)
		}
		if desc.omitEmpty && empty {
			continue
		}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You are correct that reflect.String is not necessary. Including it in the condition is only to align with the original code and provide a shortcut return.
  2. isEmpty makes sense to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the more terse description of omitempty from the Marshal function documentation:

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

AFAIK, the BSON library omitempty has the same behavior.

return v.Len() == 0
case reflect.Struct:
if !omitZeroStruct {
return false
}
Expand All @@ -410,7 +413,7 @@ func isZero(v reflect.Value, omitZeroStruct bool) bool {
if ff.PkgPath != "" && !ff.Anonymous {
continue // Private field
}
if !isZero(v.Field(i), omitZeroStruct) {
if !isEmpty(v.Field(i), omitZeroStruct) {
return false
}
}
Expand Down
19 changes: 17 additions & 2 deletions bson/bsoncodec/struct_codec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,21 @@ func TestIsZero(t *testing.T) {
omitZeroStruct: true,
want: false,
},
{
description: "empty map",
value: map[string]string{},
want: true,
},
{
description: "empty slice",
value: []struct{}{},
want: true,
},
{
description: "empty string",
value: "",
want: true,
},
}

for _, tc := range testCases {
Expand All @@ -148,8 +163,8 @@ func TestIsZero(t *testing.T) {
t.Run(tc.description, func(t *testing.T) {
t.Parallel()

got := isZero(reflect.ValueOf(tc.value), tc.omitZeroStruct)
assert.Equal(t, tc.want, got, "expected and actual isZero return are different")
got := isEmpty(reflect.ValueOf(tc.value), tc.omitZeroStruct)
assert.Equal(t, tc.want, got, "expected and actual isEmpty return are different")
})
}
}
84 changes: 40 additions & 44 deletions bson/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

// Package bson is a library for reading, writing, and manipulating BSON. BSON is a binary serialization format used to
// store documents and make remote procedure calls in MongoDB. The BSON specification is located at https://bsonspec.org.
// The BSON library handles marshalling and unmarshalling of values through a configurable codec system. For a description
// of the codec system and examples of registering custom codecs, see the bsoncodec package. For additional information and
// usage examples, check out the [Work with BSON] page in the Go Driver docs site.
// The BSON library handles marshaling and unmarshaling of values through a configurable codec system. For a description
// of the codec system and examples of registering custom codecs, see the bsoncodec package. For additional information
// and usage examples, check out the [Work with BSON] page in the Go Driver docs site.
//
// # Raw BSON
//
Expand Down Expand Up @@ -38,7 +38,7 @@
// bson.D{{"foo", "bar"}, {"hello", "world"}, {"pi", 3.14159}}
// bson.M{"foo": "bar", "hello": "world", "pi": 3.14159}
//
// When decoding BSON to a D or M, the following type mappings apply when unmarshalling:
// When decoding BSON to a D or M, the following type mappings apply when unmarshaling:
//
// 1. BSON int32 unmarshals to an int32.
// 2. BSON int64 unmarshals to an int64.
Expand All @@ -62,7 +62,7 @@
// 20. BSON DBPointer unmarshals to a primitive.DBPointer.
// 21. BSON symbol unmarshals to a primitive.Symbol.
//
// The above mappings also apply when marshalling a D or M to BSON. Some other useful marshalling mappings are:
// The above mappings also apply when marshaling a D or M to BSON. Some other useful marshaling mappings are:
//
// 1. time.Time marshals to a BSON datetime.
// 2. int8, int16, and int32 marshal to a BSON int32.
Expand All @@ -71,73 +71,69 @@
// 4. int64 marshals to BSON int64 (unless [Encoder.IntMinSize] is set).
// 5. uint8 and uint16 marshal to a BSON int32.
// 6. uint, uint32, and uint64 marshal to a BSON int64 (unless [Encoder.IntMinSize] is set).
// 7. BSON null and undefined values will unmarshal into the zero value of a field (e.g. unmarshalling a BSON null or
// 7. BSON null and undefined values will unmarshal into the zero value of a field (e.g. unmarshaling a BSON null or
// undefined value into a string will yield the empty string.).
//
// # Structs
//
// Structs can be marshalled/unmarshalled to/from BSON or Extended JSON. When transforming structs to/from BSON or Extended
// Structs can be marshaled/unmarshaled to/from BSON or Extended JSON. When transforming structs to/from BSON or Extended
// JSON, the following rules apply:
//
// 1. Only exported fields in structs will be marshalled or unmarshalled.
// 1. Only exported fields in structs will be marshaled or unmarshaled.
//
// 2. When marshalling a struct, each field will be lowercased to generate the key for the corresponding BSON element.
// 2. When marshaling a struct, each field will be lowercased to generate the key for the corresponding BSON element.
// For example, a struct field named "Foo" will generate key "foo". This can be overridden via a struct tag (e.g.
// `bson:"fooField"` to generate key "fooField" instead).
//
// 3. An embedded struct field is marshalled as a subdocument. The key will be the lowercased name of the field's type.
// 3. An embedded struct field is marshaled as a subdocument. The key will be the lowercased name of the field's type.
//
// 4. A pointer field is marshalled as the underlying type if the pointer is non-nil. If the pointer is nil, it is
// marshalled as a BSON null value.
// 4. A pointer field is marshaled as the underlying type if the pointer is non-nil. If the pointer is nil, it is
// marshaled as a BSON null value.
//
// 5. When unmarshalling, a field of type interface{} will follow the D/M type mappings listed above. BSON documents
// unmarshalled into an interface{} field will be unmarshalled as a D.
// 5. When unmarshaling, a field of type interface{} will follow the D/M type mappings listed above. BSON documents
// unmarshaled into an interface{} field will be unmarshaled as a D.
//
// The encoding of each struct field can be customized by the "bson" struct tag.
//
// This tag behavior is configurable, and different struct tag behavior can be configured by initializing a new
// bsoncodec.StructCodec with the desired tag parser and registering that StructCodec onto the Registry. By default, JSON tags
// are not honored, but that can be enabled by creating a StructCodec with JSONFallbackStructTagParser, like below:
// bsoncodec.StructCodec with the desired tag parser and registering that StructCodec onto the Registry. By default, JSON
// tags are not honored, but that can be enabled by creating a StructCodec with JSONFallbackStructTagParser, like below:
//
// Example:
//
// structcodec, _ := bsoncodec.NewStructCodec(bsoncodec.JSONFallbackStructTagParser)
//
// The bson tag gives the name of the field, possibly followed by a comma-separated list of options.
// The name may be empty in order to specify options without overriding the default field name. The following options can be used
// to configure behavior:
//
// 1. omitempty: If the omitempty struct tag is specified on a field, the field will not be marshalled if it is set to
// the zero value. Fields with language primitive types such as integers, booleans, and strings are considered empty if
// their value is equal to the zero value for the type (i.e. 0 for integers, false for booleans, and "" for strings).
// Slices, maps, and arrays are considered empty if they are of length zero. Interfaces and pointers are considered
// empty if their value is nil. By default, structs are only considered empty if the struct type implements the
// bsoncodec.Zeroer interface and the IsZero method returns true. Struct fields whose types do not implement Zeroer are
// never considered empty and will be marshalled as embedded documents.
// The name may be empty in order to specify options without overriding the default field name. The following options can
// be used to configure behavior:
//
// 1. omitempty: If the omitempty struct tag is specified on a field, the field will be omitted from the marshaling if
// the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array,
// slice, map, or string.
// NOTE: It is recommended that this tag be used for all slice and map fields.
//
// 2. minsize: If the minsize struct tag is specified on a field of type int64, uint, uint32, or uint64 and the value of
// the field can fit in a signed int32, the field will be serialized as a BSON int32 rather than a BSON int64. For other
// types, this tag is ignored.
// the field can fit in a signed int32, the field will be serialized as a BSON int32 rather than a BSON int64. For
// other types, this tag is ignored.
//
// 3. truncate: If the truncate struct tag is specified on a field with a non-float numeric type, BSON doubles unmarshalled
// into that field will be truncated at the decimal point. For example, if 3.14 is unmarshalled into a field of type int,
// it will be unmarshalled as 3. If this tag is not specified, the decoder will throw an error if the value cannot be
// decoded without losing precision. For float64 or non-numeric types, this tag is ignored.
// 3. truncate: If the truncate struct tag is specified on a field with a non-float numeric type, BSON doubles
// unmarshaled into that field will be truncated at the decimal point. For example, if 3.14 is unmarshaled into a
// field of type int, it will be unmarshaled as 3. If this tag is not specified, the decoder will throw an error if
// the value cannot be decoded without losing precision. For float64 or non-numeric types, this tag is ignored.
//
// 4. inline: If the inline struct tag is specified for a struct or map field, the field will be "flattened" when
// marshalling and "un-flattened" when unmarshalling. This means that all of the fields in that struct/map will be
// pulled up one level and will become top-level fields rather than being fields in a nested document. For example, if a
// map field named "Map" with value map[string]interface{}{"foo": "bar"} is inlined, the resulting document will be
// {"foo": "bar"} instead of {"map": {"foo": "bar"}}. There can only be one inlined map field in a struct. If there are
// duplicated fields in the resulting document when an inlined struct is marshalled, the inlined field will be overwritten.
// If there are duplicated fields in the resulting document when an inlined map is marshalled, an error will be returned.
// This tag can be used with fields that are pointers to structs. If an inlined pointer field is nil, it will not be
// marshalled. For fields that are not maps or structs, this tag is ignored.
//
// # Marshalling and Unmarshalling
//
// Manually marshalling and unmarshalling can be done with the Marshal and Unmarshal family of functions.
// marshaling and "un-flattened" when unmarshaling. This means that all of the fields in that struct/map will be
// pulled up one level and will become top-level fields rather than being fields in a nested document. For example,
// if a map field named "Map" with value map[string]interface{}{"foo": "bar"} is inlined, the resulting document will
// be {"foo": "bar"} instead of {"map": {"foo": "bar"}}. There can only be one inlined map field in a struct. If
// there are duplicated fields in the resulting document when an inlined struct is marshaled, the inlined field will
// be overwritten. If there are duplicated fields in the resulting document when an inlined map is marshaled, an
// error will be returned. This tag can be used with fields that are pointers to structs. If an inlined pointer field
// is nil, it will not be marshaled. For fields that are not maps or structs, this tag is ignored.
//
// # Marshaling and Unmarshaling
//
// Manually marshaling and unmarshaling can be done with the Marshal and Unmarshal family of functions.
//
// [Work with BSON]: https://www.mongodb.com/docs/drivers/go/current/fundamentals/bson/
package bson
31 changes: 30 additions & 1 deletion bson/primitive_codecs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,36 @@ func TestDefaultValueEncoders(t *testing.T) {
docToBytes(D{{"a", primitive.Symbol("foobarbaz")}}),
nil,
},
{
"omitempty map",
struct {
T map[string]string `bson:",omitempty"`
}{
T: map[string]string{},
},
docToBytes(D{}),
nil,
},
{
"omitempty slice",
struct {
T []struct{} `bson:",omitempty"`
}{
T: []struct{}{},
},
docToBytes(D{}),
nil,
},
{
"omitempty string",
struct {
T string `bson:",omitempty"`
}{
T: "",
},
docToBytes(D{}),
nil,
},
{
"struct{}",
struct {
Expand Down Expand Up @@ -596,7 +626,6 @@ func TestDefaultValueDecoders(t *testing.T) {
llvrw = rc.llvrw
}
llvrw.T = t
// var got interface{}
if rc.val == cansetreflectiontest { // We're doing a CanSet reflection test
err := tc.vd.DecodeValue(dc, llvrw, reflect.Value{})
if !compareErrors(err, rc.err) {
Expand Down