chbrown / rfc6902

Complete implementation of RFC6902 in TypeScript
https://chbrown.github.io/rfc6902/
316 stars 39 forks source link

Empty pointer when comparing list elements #86

Closed nestorrente closed 2 years ago

nestorrente commented 2 years ago

Hi @chbrown!

I need to create a patch comparing 2 objects, but I need to customize the diff algorithm taking into account some special objects.

I decided to use this library as it seems to be the only one that allows me to customize the diff algorithm (+1 for that! :grinning:). However, I'm facing some issues when comparing elements inside arrays. Let me show it with an example (this is not the real example, but a simplified one):

import { createPatch } from "rfc6902";

const obj1 = {
  numbers: [1, 2, 3],
  users: [
    { id: 1, username: "One", active: false },
    { id: 2, username: "Two", active: true },
    { id: 3, username: "Three", active: true }
  ]
};

const obj2 = {
  numbers: [1, 2, 3],
  users: [
    { id: 2, username: "Two", active: false },
    { id: 1, username: "One", active: true },
    { id: 3, username: "Three", active: true }
  ]
};

const typesByPath = {
  "": "Root",
  "/numbers": "array",
  "/numbers/\\d+": "int",
  "/users": "array",
  "/users/\\d+": "User",
  "/users/\\d+/id": "int",
  "/users/\\d+/username": "string",
  "/users/\\d+/active": "boolean"
};

function resolvePathType(path) {

  const type = Object.entries(typesByPath).find(([regex]) =>
    new RegExp("^" + regex + "$").test(path)
  )?.[1];

  if (type == null) {
    throw new Error("Unknown type for path: " + path);
  }

  return type;

}

const patch = createPatch(obj1, obj2, (input, output, pointer) => {
  const { tokens } = pointer;
  const path = tokens.join("/");

  const type = resolvePathType(path);

  if (type === "User") {
    // Special checks for the User type
    if (input.id !== output.id) {
      return [{ op: "replace", path, value: output }];
    }
  }

  // Other type checks...
});

As you can see, the function resolvePathType(path) resolves the type of the value present in a specific path. However, due to the creation of a new Pointer() at this line, the path information is lost when the users lists are being compared. This causes my code to throw an error with the message "Unknown type for path: /id", when the real path is /users/[index]/id.

I now it's not possible to use a real index in that pointer because the items being compared can have two different indices. However, could it be possible to use some special value for these cases? Something like /users/*/id, for example. That will be more accurate than creating a new Pointer with the path of the root object, and I think it can be achieved by modifying the creation of the Pointer to new Pointer([...token, '*']).

Thank you very much for your time! :slightly_smiling_face:

chbrown commented 2 years ago

Appreciate the simplified example! I see what you mean but you gotta admit, what you're doing is pretty weird 😉 ... I'm surprised but nevertheless pleased that my design can accommodate it.

Any derivatives of the placeholder new Pointer() (on that line you linked to) are immediately discarded because the diff(...)'s returned Operations are discarded (it only checks the .length), so yeah, no problem changing it to be something more expressive, even if it's only used (/useful) in edge cases like yours.

My only concern is to keep it syntactically valid, to not trip up any existing users' diff(..., ptr) calls that expect the given ptr to be valid, albeit not always necessarily pointing to an actual path in the root level object.

So I don't like the '*' idea, but I think '-' or '0' would be fine. However — and maybe I'm not thinking it through entirely — it seems like the most accurate would be to use the path of the input argument in the context of the diff(input, ...) call in question? I.e., replace:

!diff(input[i - 1], output[j - 1], new Pointer()).length

with:

!diff(input[i - 1], output[j - 1], ptr.add(String(i - 1))).length

Which should solve your problem, IIUC?

nestorrente commented 2 years ago

Hi @chbrown!

I really appreciate your quick reply :slightly_smiling_face:.

Yes, I supposed that my case was very edgy, and it sounds weird when out of context :sweat_smile:.

Regarding the solution you mentioned, the only thing I need to know is that I'm visiting an element of the list /users (or a property of one of those elements), but I don't really care about the index of the input and output items, so for me both solutions (the '-' symbol or the index of the input item) are valid :slightly_smiling_face: but maybe the index is more accurate, as you said (and definitely much better than the asterisk :wink:).

Thank you a lot for your time and consideration!

chbrown commented 2 years ago

Ok, fixed in f0de16e, published as v5.0.1

nestorrente commented 2 years ago

These are great news! :grinning: thank you so much @chbrown, your library saved my week :slightly_smiling_face: