GODRIVER-3470 Correct BSON unmarshaling logic for null values

[prestonvasquez]

GODRIVER-3470

Summary

Ensure UnmarshalBSONValue is bypassed and the Go pointer is set to nil ONLY when the Go type is a pointer and the BSON value is null.

Background & Motivation

PR #1903 introduced logic where UnmarshalBSONValue() is not called for bson.TypeNull, which breaks applications that rely on this behavior for initializing fields.

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[matthewdale]

Should we also test this behavior for UnmarshalBSON?

[prestonvasquez]

Good idea!

[qingyang-hu]

LGTM!

[matthewdale]

Overall, looks good! 👍

Open question about using the same set of conditions in ValueUnmarshalerDecodeValue and UnmarshalerDecodeValue so it's easier to understand in the future.

[matthewdale]

There is a similar block in UnmarshalerDecodeValue after the value is read into a []byte:

if val.Kind() == reflect.Ptr && len(src) == 0 {
	val.Set(reflect.Zero(val.Type()))
	return nil
}

Can we use the same condition in both methods? Or are they distinct scenarios?

[prestonvasquez]

The len(src) check was implemented here: https://github.com/mongodb/mongo-go-driver/pull/833/files

Since the bytes represent BSON null are copied, the type is converted from null to invalid:

	_, src, err := bsonrw.Copier{}.CopyValueToBytes(vr)
	if err != nil {
		return err
	}

Which means that this check doesn’t work:

	if val.Kind() == reflect.Ptr && vr.Type() == bsontype.Null {
		val.Set(reflect.Zero(val.Type()))
		return nil
	}

Checking after copying is weaker since validity should be checked on the first block:

	if !val.IsValid() || (!val.Type().Implements(tValueUnmarshaler) && !reflect.PtrTo(val.Type()).Implements(tValueUnmarshaler)) {
		return ValueDecoderError{Name: "ValueUnmarshalerDecodeValue", Types: []reflect.Type{tValueUnmarshaler}, Received: val}
	}

So I think BSON null is the only case where you would get an invalid type after copying. I suggest we mirror ValueUnmarshalerDecodeValue.

[matthewdale]

Removing this will result in UnmarshalBSON being called with an empty byte slice for BSON minkey and maxkey value. Because the byte slice passed to UnmarshalBSON has the BSON type byte stripped off it, zero-length BSON values will be indistinguishable from each other (null, minkey, maxkey). Because of that limitation, we should keep the existing behavior.

Here are two test cases that can be added to unmarshalingTestCases that demonstrate the behavior:

{
	name:  "nil pointer and non-pointer type with BSON minkey",
	sType: reflect.TypeOf(unmarshalBehaviorTestCase{}),
	want: &unmarshalBehaviorTestCase{
		BSONValueTracker: unmarshalBSONValueCallTracker{
			called: true,
		},
		BSONValuePtrTracker: &unmarshalBSONValueCallTracker{
			called: true,
		},
		BSONTracker: unmarshalBSONCallTracker{
			called: true,
		},
		BSONPtrTracker: nil,
	},
	data: docToBytes(D{
		{Key: "bv_tracker", Value: primitive.MinKey{}},
		{Key: "bv_ptr_tracker", Value: primitive.MinKey{}},
		{Key: "b_tracker", Value: primitive.MinKey{}},
		{Key: "b_ptr_tracker", Value: primitive.MinKey{}},
	}),
},
{
	name:  "nil pointer and non-pointer type with BSON maxkey",
	sType: reflect.TypeOf(unmarshalBehaviorTestCase{}),
	want: &unmarshalBehaviorTestCase{
		BSONValueTracker: unmarshalBSONValueCallTracker{
			called: true,
		},
		BSONValuePtrTracker: &unmarshalBSONValueCallTracker{
			called: true,
		},
		BSONTracker: unmarshalBSONCallTracker{
			called: true,
		},
		BSONPtrTracker: nil,
	},
	data: docToBytes(D{
		{Key: "bv_tracker", Value: primitive.MaxKey{}},
		{Key: "bv_ptr_tracker", Value: primitive.MaxKey{}},
		{Key: "b_tracker", Value: primitive.MaxKey{}},
		{Key: "b_ptr_tracker", Value: primitive.MaxKey{}},
	}),
},

[prestonvasquez]

Nice catch, updated!

[matthewdale]

Looks good! 👍

[matthewdale]

drivers-pr-bot please backport to release/1.17

[mongodb-drivers-pr-bot]

Sorry, unable to cherry-pick to release/1.17, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout -b cherry-pick-release/1.17-659342c5d8d8436dcfe7863026808d14505c225d release/1.17

git fetch origin 659342c5d8d8436dcfe7863026808d14505c225d

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 659342c5d8d8436dcfe7863026808d14505c225d

3. You will likely have some merge/cherry-pick conflicts here, fix them and commit:

git commit -am {message}

4. Push to a named branch:

git push origin cherry-pick-release/1.17-659342c5d8d8436dcfe7863026808d14505c225d

5. Create a PR against branch release/1.17. I would have named this PR:

"GODRIVER-3470 Correct BSON unmarshaling logic for null values (#1924) [release/1.17]"