Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: mergePartial #315

Open
ivanvanderbyl opened this issue Nov 26, 2022 · 8 comments
Open

Feature request: mergePartial #315

ivanvanderbyl opened this issue Nov 26, 2022 · 8 comments

Comments

@ivanvanderbyl
Copy link

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.
@smaye81
Copy link
Member

smaye81 commented Dec 1, 2022

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?

@ivanvanderbyl
Copy link
Author

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.

@timostamm
Copy link
Member

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:/timostamm/protobuf-ts/blob/3.x/packages/runtime/src/reflection-merge-partial.ts
And the tests: https:/timostamm/protobuf-ts/blob/3.x/packages/runtime/spec/reflection-merge-partial.spec.ts

@ygrandgirard
Copy link

ygrandgirard commented May 21, 2024

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.

@timostamm
Copy link
Member

@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?

@ygrandgirard
Copy link

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.

@timostamm
Copy link
Member

@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.

@jcready
Copy link

jcready commented May 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants