protobuf-es icon indicating copy to clipboard operation
protobuf-es copied to clipboard

Feature request: mergePartial

Open ivanvanderbyl opened this issue 3 years ago • 8 comments

Hi team, I really love the work you're doing bringing protobufs to the browser!

Feature request:

One use case we have is using a Message<T> as the value of our internal state for some of the UI. This works really nicely because it's typed and can be marshalled/unmarshalled easily. However, we often want to deep merge another PartialMessage<T> into that state. Something like proto3.util.initPartial(), except instead of replacing nested values it merges them left to right.

Other things we have considered

  • Using deepMerge from lodash. Works in v0.2.x so long as you don't have any 64bit ints in your schema. Broken in v0.3.x
  • Attempted porting initPartial to do a mergePartial, but it was fairly complex. I might give it another go and open a PR.

ivanvanderbyl avatar Nov 26 '22 00:11 ivanvanderbyl

Hi @ivanvanderbyl. Sorry for the delayed response. Can you provide me a quick example of what you're trying to do? I tried working up a test locally with lodash's merge and it seemed to work fine, but I just may not be understanding your scenario.

Also, related to lodash, what sort of issues were you seeing with it in v0.3.0?

smaye81 avatar Dec 01 '22 20:12 smaye81

HI @smaye81 — no worries at all. So it seems to work so long as the schema doesn't include any int64s. I'm not sure what it is about that data type but it isn't mergeable.

I've looked over the implementation of initPartial a bit and almost got a mergePartial working. I'd prefer this approach as it doesn't need to do as many property comparison checks as lo-dash merge, which will make it a lot faster.

ivanvanderbyl avatar Dec 02 '22 05:12 ivanvanderbyl

Hey Ivan 👋

I think you're correct - a mergePartial that works on the schema information should be much faster than a generic one, like lo-dash.

Did you know that protobuf-ts has such a function? It was not implemented here, but I think it should be fairly straight-forward to port.

Here's the source: https://github.com/timostamm/protobuf-ts/blob/3.x/packages/runtime/src/reflection-merge-partial.ts And the tests: https://github.com/timostamm/protobuf-ts/blob/3.x/packages/runtime/spec/reflection-merge-partial.spec.ts

timostamm avatar Dec 02 '22 11:12 timostamm

Sorry to dig out a somewhat old topic, but was there any progress on this? I am also looking forward to it!

I checked protobuf-ts and unfortunately it does not perform a deep merge either, at least not for repeated and mapped types.

ygrandgirard avatar May 21 '24 13:05 ygrandgirard

@ygrandgirard, I'm not sure there's a consensus for how a merge should work.

In JavaScript, the very popular lodash.merge merges array elements, while the equally popular deepmerge appends array elements. If you search for "deepmerge" on npmjs.com, there are dozens of alternatives.

Because of it's binary serialization format, Protobuf has it's own semantics for merging, which appends repeated fields. You can try it out by parsing binary data into a message multiple times (see the fromBinary method on class Message). The Go implementation provides a proto.Merge to apply the same merging logic without going through serialization.

Which behavior do you expect? Are you looking for a lodash merge?

timostamm avatar May 23 '24 13:05 timostamm

Hi @timostamm,

My mistake, I didn't check what lodash does first. I didn't think there could be several ways to deep-merge arrays.

I would personally expect it to do the same thing as decoding a concatenation of messages does. Since this merges the messages together -and is, I hope, well specified-, I think it could be great to have access to the same behavior without the serialization process.

ygrandgirard avatar May 23 '24 15:05 ygrandgirard

@ygrandgirard, looks like at lease the two of us have consensus 🙂

I think the the upcoming version 2 can mitigate this problem. Instead of adding a method merge to the class Message, a function merge can do the job. This keeps the door open to also add a function mergeLikeLodash, and neither will increase the bundle size of your web appplication if you don't use it.

The reflection API of v2 makes the implementation quite simple. I gave this a quick stab, see script below. If you put this file in the v2 branch, you can run it with npx tsx packages/protobuf-test/src/merge.ts.

packages/protobuf-test/src/merge.ts
import {clone, create, type DescMessage, mergeFromBinary, type MessageShape, toBinary} from "@bufbuild/protobuf";
import {reflect, type ReflectMessage} from "@bufbuild/protobuf/reflect";
import {UserDesc} from "./gen/ts/extra/example_pb.js";
import assert from "node:assert";


/**
 * Merge message `source` into message `target`.
 */
export function merge<Desc extends DescMessage>(
  messageDesc: Desc,
  target: MessageShape<Desc>,
  source: MessageShape<Desc>,
): void {
  reflectMerge(reflect(messageDesc, target), reflect(messageDesc, source));
}

function reflectMerge(target: ReflectMessage, source: ReflectMessage) {
  for (const f of target.fields) {
    if (!source.isSet(f)) {
      continue;
    }
    switch (f.fieldKind) {
      case "scalar":
      case "enum":
        target.set(f, source.get(f));
        break;
      case "message":
        reflectMerge(target.get(f), source.get(f));
        break;
      case "list":
        const list = target.get(f);
        for (const e of source.get(f)) {
          list.add(e);
        }
        break;
      case "map":
        const map = target.get(f);
        for (const [k, v] of source.get(f)) {
          map.set(k, v);
        }
        break;
    }
  }
}

const a = create(UserDesc, {
  firstName: "Joe",
  active: true,
  locations: ["Paris"],
  projects: {
    "a": "A",
    "b": "B",
  },
});

const b = create(UserDesc, {
  lastName: "Smith",
  locations: ["New York", "Tokyo"],
  projects: {
    "a": "aaa",
    "c": "C",
  },
});

const expectedResult = clone(UserDesc, a);
mergeFromBinary(UserDesc, expectedResult, toBinary(UserDesc, b));
console.log("result from binary merge:", expectedResult);

const merged = clone(UserDesc, a);
merge(UserDesc, merged, b);
console.log("result from merge:", merged);

assert.deepStrictEqual(merged, expectedResult, "result from merge() is not equal to result from binary merge");

/*

Output:

result from binary merge: {
  firstName: 'Joe',
  lastName: 'Smith',
  active: true,
  locations: [ 'Paris', 'New York', 'Tokyo' ],
  projects: { a: 'aaa', b: 'B', c: 'C' },
  '$typeName': 'docs.User'
}
result from merge: {
  firstName: 'Joe',
  lastName: 'Smith',
  active: true,
  locations: [ 'Paris', 'New York', 'Tokyo' ],
  projects: { a: 'aaa', b: 'B', c: 'C' },
  '$typeName': 'docs.User'
}

*/

If you have suggestions for a better API, let us know.

timostamm avatar May 24 '24 14:05 timostamm

FWIW Google's Java implementation provides merge options. I had implemented something similar when trying to add support for FieldMasks in protobuf-ts: https://github.com/timostamm/protobuf-ts/pull/576/files#diff-e04b7f4fd332304149f09e1fb00a2c4d2b7635c142703d536d7e97fa0566c106

jcready avatar May 24 '24 17:05 jcready