reflectionMergePartial() will overwrite target oneof field even if source has none set

[jcready]

protobuf-ts/packages/runtime/src/reflection-merge-partial.ts

Lines 39 to 49 in 3eea598

	const group = input[field.oneof] as UnknownOneofGroup | undefined; // this is the oneof`s group in the source
	if (group == undefined) { // the user is free to omit
	continue; // we skip this field, and all other members too
	}
	fieldValue = group[name]; // our value comes from the the oneof group of the source
	output = (target as UnknownMessage)[field.oneof] as UnknownOneofGroup; // and our output is the oneof group of the target
	output.oneofKind = group.oneofKind; // always update discriminator
	if (fieldValue == undefined) {
	delete output[name]; // remove any existing value
	continue; // skip further work on field
	}

message Foo {
  oneof bar {
    optional int32 baz = 1;
    optional int32 qux = 2;
  }
}

const src = Foo.create();
const dst = Foo.fromJson({ baz: 1 });
Foo.mergePartial(dst, src);
Foo.toJson(dst); // {}

I don't know for sure if this behavior is correct or incorrect, but it feels incorrect to me considering how reflectionMergePartial() treats other fields where the source field is unset:

protobuf-ts/packages/runtime/src/reflection-merge-partial.ts

Lines 51 to 55 in 3eea598

	fieldValue = input[name]; // we are using the source directly
	output = target as UnknownMessage; // we want our field value to go directly into the target
	if (fieldValue == undefined) {
	continue; // skip further work on field, existing value is used as is
	}

I would not expect an unset oneof field in the source to overwrite/unset a set oneof field in the target. I believe the fix would be to update line 40 to:

- if (group == undefined) { // the user is free to omit
+ if (group?.oneofKind == undefined) { // the user is free to omit

[timostamm]

I would not expect an unset oneof field in the source to overwrite/unset a set oneof field in the target.

I agree with that. Good shout.