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

[qingyang-hu]

GODRIVER-3081

Summary

PR #1323 overlooked the zero-value detection for empty slices and maps.

Background & Motivation

Fix the bug and make up the test cases.

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[prestonvasquez]

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

[prestonvasquez]

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
		}

[qingyang-hu]

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.

[matthewdale]

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.

[matthewdale]

Looks good! 👍

[matthewdale]

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.