incrementalPatch does not work as expected for documents with nested fields.

[alex-zywicki]

Given a document structure like

{
    "foo": "bar",
    "data": {
        "field1":  1,
        "field2": 2,
    }
}

a call to incrementalPatch like

doc.incrementalPatch({
    data: {
        field1: 3
    }
})

will result in a new object state like

{
    "foo": "bar",
    "data": {
        "field1":  3,
        // "field2": 2, This field was removed
    }
}

After looking at the implementation https://github.com/pubkey/rxdb/blob/master/src/rx-document.ts#L316

It appears that incrementalPatch simply does a "one level" shallow patch instead of a "deep merge" operation so the output would be

{
    "foo": "bar",
    "data": {
        "field1":  3, // The field was changed, but other fields not interfered with
        "field2": 2,
    }
}

A simple potential solution would be to leverage a package like https://www.npmjs.com/package/object-deep-merge (I cannot personally vouch for this library, it was what I found when searching) or to implement that functionality manually in RxDB.

That would mean updating the current implementation from

    incrementalPatch<RxDocumentType = any>(
        this: RxDocument<RxDocumentType>,
        patch: Partial<RxDocumentType>
    ): Promise<RxDocument<RxDocumentType>> {
        return this.incrementalModify((docData) => {
            Object
                .entries(patch)
                .forEach(([k, v]) => {
                    (docData as any)[k] = v;
                });
            return docData;
        });
    },

to something like

    incrementalPatch<RxDocumentType = any>(
        this: RxDocument<RxDocumentType>,
        patch: Partial<RxDocumentType>
    ): Promise<RxDocument<RxDocumentType>> {
        return this.incrementalModify((docData) => {
            return merge(docData, patch);
        });
    },

I would be happy to provide further context to help prove the existence of the problem, and write a PR with the proposed change if this seems acceptable.

[alex-zywicki]

I appears that patch has the same underlying issue.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed soon. Please update it or it may be closed to keep our repository organized. The best way is to add some more information or make a pull request with a test case. Also you might get help in fixing it at the RxDB Community Chat If you know you will continue working on this, just write any message to the issue (like "ping") to remove the stale tag.

[alex-zywicki]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed soon. Please update it or it may be closed to keep our repository organized. The best way is to add some more information or make a pull request with a test case. Also you might get help in fixing it at the RxDB Community Chat If you know you will continue working on this, just write any message to the issue (like "ping") to remove the stale tag.

There is a PR #7432

[pubkey]

Hi @alex-zywicki I thought a lot about this and I do not think we should deep-merge on patch. If a nested object is replaced with patch, it should not keep the non-defined previous fields, I think that is just confusing. Use incremental modify to do that. I think this should be stated in the docs. Patching an object-property replaces the whole object.

[alex-zywicki]

Hi @pubkey I cannot say that I entirely understand that reasoning. The current implementation does not abide by that thinking in a uniform manor as far as I can tell and is somewhat inconsistent in what to expect from its behavior.

when calling patch/incrementalPatch

// Assume we have a RxDocument with the following state
let doc = {
    foo: "foo",
    bar: "bar",
    nested: {
        another: false,
        baz:"baz"
    }
};


doc = await doc.patch({
    foo: "hello"
});

/* will yield a doc state of
{
    foo: "hello",
    bar: "bar",
    nested: {
        another: false,
        baz:"baz"
    }
}
not a state of 
{
    foo: "hello"
}
*/

// But then doing a patch like

doc = await doc.patch({
    nested: {
        another: true
    }
});

/* will yield a doc state of
{
    foo: "hello",
    bar: "bar",
    nested: {
        another: true,
        // baz went missing
    }
}
instead of

{
    foo: "hello",
    bar: "bar",
    nested: {
        another: true, //only another was updated as that was the only leaf value in the patch
        baz:"baz"
    }
}
*/

How does it makes sense that it will merge at the top level, but not at any other level? How is that a patch operation.

To me a normal patch operation would allow you to provide some Partial<DocumentDataType> object and have the values of that partial object grafted on to the document. Otherwise with the current behavior it forces users with more involved schemas to either implement a workaround doing what my PR suggests, or to call patch with a complete value rather than simply a partial.

To the best of my knowledge the spec for patch/incrementalPatch allows for you to explicitly pass key: undefined to splice out values, so why not just let that be the way things are spliced out?

What is your definition for what a patch should do?

Your suggestion in my opinion leads to a philosophy where RxDB does not provide first class support for complicated document schemas.

If the underlying concern is that you do not want to break peoples usages of patch/incrementalPatch that I can understand. If that is the case it would be fairly simple to add optional flags to the calls to allow the user to enable/disable the deep merge behavior with the existing behavior enabled by default.

I can provide more concrete examples of how I am using RxDB if needed if that will help?

[pubkey]

I think the problem is the naming of the function "patch". I checked and there is no "official" definition if a patch should deep merge or not. Lets have a look at how other NoSQL database perform patches:

(table created with chatGPT but double checked)

So most databases do not deep merge by default. But also they do not have the naming "patch" instead they call it "update" or something.

If you check check HTTP PATCH JSON Merge Patch (RFC 7396), it also does a deep-merge on objects (no on arrays etc.).

So I think we should add an additonal function mergePatch() and incrementalMergePatch() which do a deep-merge. Also for patch we should add infos in the docs that it does not deep-merge by default.

I think its too dangerous to change the current behavior of patch() even in a major version update. People will not notice and their apps will build and work, just not exactly equal anymore.

[alex-zywicki]

Instead of adding an entirely new method would it not make more sense to just add an optional merge flag to patch/incrementalPatch

doc.patch({}, {merge: true})

or

doc.patch({}, true)

[pubkey]

Hi @alex-zywicki The current library does no mix up these modification functions with any flags. Therefore our deepmerge function should also not do this and be a separate function with a different name. This also makes sense because a typescript Partial is no a "deep-partial" and the two function therefore have different typings. See this code which has no valid typings:

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed soon. Please update it or it may be closed to keep our repository organized. The best way is to add some more information or make a pull request with a test case. Also you might get help in fixing it at the RxDB Community Chat If you know you will continue working on this, just write any message to the issue (like "ping") to remove the stale tag.