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.