If field is repeated and field kind is "message" then reflectionMergePartial() only overwrites part of array

[jcready]

I can't tell if this is intentional or perhaps I'm just not reading the code correctly, but there doesn't appear to be a test case for it so I figured I'd ask.

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

Lines 60 to 64 in 80de3ee

	case "message":
	let T = field.T();
	if (field.repeat)
	for (let i = 0; i < (fieldValue as any[]).length; i++)
	(output[name] as any[])[i] = T.create((fieldValue as any[])[i]);

With the above logic it seems like if the target/output array was longer than the source/input array then we would end up with with only a partially overwritten target/output array.

const source = { foo: [{ bar: 1 }] };
const target = { foo: [{ bar: 2 }, { bar: 3 }] };

const result = reflectionMergePartial(SomeMessageType, source, target);
expect(result).toEqual({ foo: [{ bar: 1 }, { bar: 3 }] }); // I think? But is this actually expected?

The logic for handling repeated scalar fields just replaces the target array which is why I suspect that this behavior isn't expected.

[fnatte]

I believe golang just appends: https://github.com/protocolbuffers/protobuf-go/blob/de9682ad1656cd1ee6808865f7a42f5a5d79eb73/proto/merge.go#L104

[timostamm]

Good catch, @jcready. I missed this discussion initially, sorry.

The partial merge is a logic specific to protobuf-ts. It is definitely different from the protobuf merge semantics, where repeated fields (including maps) are appended, which I think is pretty unexpected for JS developers.

The JSDoc states:

Message fields are recursively merged (by calling mergePartial()
of the responsible message handler). Map and repeated fields
are simply overwritten, not appended or merged.

Looking at the code, that doesn't seem to be the case for repeated message fields and neither for maps. The values of the input array or map replace the corresponding values in the output array or map. But the map or array itself is not replaced, and entries may remain in the output.

I find it difficult to reason about the semantics - there are always arguments in favor and against a specific behavior. But the difference between documentation and implementation is clearly a bug.

[jcready]

I guess I was more concerned that the behavior was different between repeated scalar fields and repeated message fields. Repeated scalar fields are replaced (even if the source array is shorter than the target array) where as that is not the case for repeated message fields.

const source = { foo: [{ bar: 1 }],             baz: [1] };
const target = { foo: [{ bar: 2 }, { bar: 3 }], baz: [2, 3] };

const result = reflectionMergePartial(SomeMessageType, source, target);
//    result = { foo: [{ bar: 1 }, { bar: 3 }], baz: [1] }

[timostamm]

Gotcha. I've been going through the code again, and am updating the documentation in #361.

[fnatte]

@timostamm What's your take on #335? Have you had time to consider merging it?

[timostamm]

Just did :)