feat(data-service): add json-patch support by HendrikThePendric · Pull Request #1023 · dhis2/app-runtime

HendrikThePendric · 2021-09-22T07:20:43Z

This introduces JSON Patch support for the useDataMutation hook, via an explicit type. The data property needs to be a valid patch array. It is entirely up to the user to ensure this patch is valid, we do no form of validation in app-runtime.

A JSON Patch mutation object would as follows:

const updateMutation = {
    type: 'jsonPatch',  // The new type
    resource: 'userGroups',
    id: 'xyz123',
    data: [ // patch array
        { op: 'replace', path: '/baz', value: 'boo' },
        { op: 'add', path: '/hello', value: ['world'] },
        { op: 'remove', path: '/foo' },
    ],
}

HendrikThePendric · 2021-10-14T14:13:31Z

FYI: I placed a few comments before that referred to my original implementation and @Mohammer5 replied to one of these comments. By now I've gone with a different type of implementation and the comments did not make much sense anymore. So I removed my comments and the one from Jan-Gerke as well.

Mohammer5

LGTM

mediremi

The data engine's validation helpers need to be updated so that jsonPatch is a valid type:

app-runtime/services/data/src/engine/helpers/validate.ts

Line 5 in 1b3ba1d

const validTypes = ['read', 'create', 'update', 'replace', 'delete']

varl · 2021-11-04T08:02:46Z

Is this intentionally still a draft ?

mediremi · 2021-11-04T16:24:34Z

@varl should be good to rereview now

varl · 2021-11-05T11:51:51Z

 |   Property   |              Type              |              | Description                                                                                                                                                |
 | :----------: | :----------------------------: | ------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------- |
-|   **type**   |            _string_            | **required** | The type of mutation to perform, must be one of `create`, `update`, or `delete`.                                                                           |
+|   **type**   |            _string_            | **required** | The type of mutation to perform, must be one of `create`, `update`, `jsonPatch`\*, or `delete`.                                                            |


These types were intentionally kept abstract here (CRUD vs. POST/PUT/PATCH/DELETE/GET) and jsonPatch goes against that intent, as it goes beyond saying what to do by saying how to do it.

The "update" type could be either a PUT or a PATCH under the hood, or, in this case, a "PATCH + json-patch+json".

So conceptually I'm not sure about the jsonPatch type. It doesn't belong as a type on the same level as the others.

I keep flip-flopping on if I consider json-patch a type of mutation in the same vein as CRUD operations.

Conceptually I want it under the "update" mutation, however the implementation is convincing in its simplicity.

So you would prefer useDataQuery to remain at its current level of abstraction and hide the format of the data being sent to the server? And I guess if/when we support graphql we also wouldn't want a type of graphql so makes sense.

@mediremi yeah, that's correct.

I can see why you wouldn't want jsonPatch as its own type as it doesn't really match with the CRUD types.

But the CRUD types are declarative, which means that the actual implementation is hidden/inaccessible/uncontrollable.
jsonPatch on the other hand is an imperative instruction that tells the server what exactly it should do, so my preference would be to not "hide" it behind a declarative type, I think that it would even violate consistency as we'd be mixing declarative CRUD types with an imperative "handle".

When it comes to translating this to the graphql world, then there's nothing that'd stop us from using jsonPatch as a type. Graphql only knows query and mutation. What kind of mutations exist depends on the schema. Normally there'd be a create/update/delete mutation for every type (e.g. createDataElement, updateDataElement and deleteDataElement; actually there can be a lot more, like linkDataElementWithDataSet for example). There's nothing that prevents us from adding patchDataElement/jsonPatchDataElement.

Right. I think where we see things differently is if the request payload is part of the Data Engine's (our) API or the Server's (their) API.

My perspective is that the payload itself is not part of our API. It's determined by the server in all cases.

So on our side, the developer will declare what they want to do (e.g. update), and pass along a payload. That's declarative as far as our API goes.

If that payload is an "sequential instruction set" we pass that along to the server in the proper way.

If that payload is something else (e.g. GQL), we pass that along in the proper way without the user having to think about the specifics of how a request is sent to the server.

In this case, the trade-off is that a developer has to know that we support json-patch instruction sets when using the update type. This is consistent with what the developer currently needs to know when s/he works with the API through the Data Engine so I am OK with the trade-off.

varl · 2021-11-05T11:54:39Z

    if (type === 'delete' && query.data) {
        errors.push("Mutation type 'delete' does not support property 'data'")
    }
+    if (type === 'jsonPatch' && !Array.isArray(query.data)) {


Are there other introspective checks we can do on the data to make sure that we are dealing with a json-patch request body if the type is 'update' ?

I guess we know the shape of a patch array, so we could make sure that the children are sane.

yep we could check if query.data is an array, whose values are objects with an op key containing one of the following values: "add", "remove", "replace", "move", "copy", "test" (see https://datatracker.ietf.org/doc/html/rfc6902/#section-4)

Nice, that seems like a solid validation to do.

amcgee · 2021-11-10T13:01:08Z

@HendrikThePendric thanks for putting this together!

Our mutation types are all CRUD verbs (create, read, update, delete) so I'm hesitant to add jsonPatch. @varl and I were thinking that we should prefer consistency, if possible. Could we maybe send a json-patch HTTP request when passing an array to data for a update mutation? We could perhaps do a shallow validation of the array to make sure all are valid json-patch operations. The risk, of course, is that there might be hidden somewhere DEEP in our API a PUT request somewhere which expects a top-level array body, though I don't know of any and would probably consider it a bug if I found one.

HendrikThePendric · 2021-11-16T09:19:34Z

@HendrikThePendric thanks for putting this together!

Our mutation types are all CRUD verbs (create, read, update, delete) so I'm hesitant to add jsonPatch. @varl and I were thinking that we should prefer consistency, if possible. Could we maybe send a json-patch HTTP request when passing an array to data for a update mutation? We could perhaps do a shallow validation of the array to make sure all are valid json-patch operations. The risk, of course, is that there might be hidden somewhere DEEP in our API a PUT request somewhere which expects a top-level array body, though I don't know of any and would probably consider it a bug if I found one.

I thought this had been discussed already and that we preferred an explicit approach. Perhaps I have misremembered.

I would be on board with this alternative approach too, but I do see some potential downside with determining the request-type based on the shape of the data property. Personally I imagined that in the future we could add some tooling to the data-service that would actually produce patch arrays for the app. For example (this is a very rough idea which I haven't really thought through) we could retain the original object and the app could send us an updated object, and we could produce the patch array based on the diff.

The type of thing I outline above would not be possible if we expect the data to already be a patch array when it is sent to the data-service. But I guess we could always expose other hooks/helpers to aid the creation of patch arrays....

amcgee · 2021-11-16T09:25:24Z

The type of thing I outline above would not be possible if we expect the data to already be a patch array when it is sent to the data-service. But I guess we could always expose other hooks/helpers to aid the creation of patch arrays....

Why would it not be possible? If we see an update with an object data body, we could tehn generate the necessary patch array right?

HendrikThePendric · 2021-11-23T16:10:15Z

Why would it not be possible? If we see an update with an object data body, we could then generate the necessary patch array right?

Just so it doesn't look like I simply ignored Austin's question.... Austin and I had a chat about this on Slack and there was a slight misunderstanding on my side: If we assume all supported DHIS2-core versions have working JSON Patch support then automatically converting the data of an update is possible, and actually a great idea. As a byproduct of my misunderstanding Austin also realised that we shouldn't start doing this automatic conversion before all supported DHIS2 Core versions have full support for JSON patch 😊 .

amcgee · 2021-11-23T16:17:07Z

I'll take a stab at this while @mediremi is off - if anyone has opinions on my comments above let me know

amcgee · 2021-12-04T23:55:21Z

Possible alternative name... instead of a mutation with type: 'jsonPatch' and instead of inspecting the data type let's keep it simple but also explicit with a new verb type: 'patch' - patch is also a verb which makes just as much sense as update or delete (update still has its place when you want to replace an entire object... maybe replace could be added as an alias for update...)

varl · 2021-12-06T13:06:58Z

I thought the API design choice for using "CRUD" (create, read, update, delete) is that it is very easy for the consumer to understand what they do, and therefor when they should be used. Adding "patch" into that makes it less so.

"Patch" is basically "Edit", which is covered under "Update", so it pushes the complexity to the user for the sake of less complexity in the implementation.

If less complexity in the implementation is the design goal, then "CRUD" isn't the optimal convention; instead we can pass through the HTTP verbs and deprecate the CRUD pattern.

amcgee · 2021-12-06T13:30:35Z

I actually think it makes *less* complexity for the user. Right now we have CRUD (create, read, update, delete) but `update` has two modes of operation - complete (replace, default) or partial (patch, currently with `partial: true` which we want to get rid of). We're considering replacing that with a subtle behavior change based on whether the user passes an object (replace) or an array (patch). I think it's actually much cleaner to have separate verbs for replacing versus patching (modifying, editing, partially updating)?

…

On Mon, Dec 6, 2021 at 1:07 PM Viktor Varland ***@***.***> wrote: I thought the API design choice for using "CRUD" (create, read, update, delete) is that it is very easy for the consumer to understand what they do, and therefor when they should be used. Adding "patch" into that makes it less so. "Patch" is basically "Edit", which is covered under "Update", so it pushes the complexity to the user for the sake of less complexity in the implementation. If less complexity in the implementation is the design goal, then "CRUD" isn't the optimal convention; instead we can pass through the HTTP verbs and deprecate the CRUD pattern. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1023 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHHNMCJGV4NZ5SKVB7S673UPSYPZANCNFSM5EQTNW3Q> .

varl · 2021-12-07T09:32:34Z

The problem is that "update" as a term covers both "full updates" and "partial updates" so having "update" as a verb and having "patch" doesn't respect the abstraction we have chosen.

That level of abstraction is "create - read - update - delete", and "patch" is a lower level of abstraction because "patch" is a kind of update.

I don't see how they can live on the same abstraction level conceptually.

If we go with a new type for json-patch, I think the original proposal of using jsonPatch is a good choice with the least surprises from a consumer perspective.

The partial concept is not bad to me either. Since updates can be done in many ways, having a specifier for how to update is warranted. A boolean for partial isn't ideal though.

Some examples below for how it can fit into our abstraction level, and how it can look if we decide to lower the abstraction level.

With our current level of abstraction:

Default (replace):

{
  type: 'update',
  partial: false, // PUT + json
  data: {},
}

Standard PATCH + json (edit):

{
  type: 'update',
  partial: true, // PATCH + json
  data: {},
}

JSON Patch + json (edit):

{
  type: 'update',
  partial: 'json-patch', // PATCH + json-patch+json
  data: [{}, {}, {}],
}

Lower level of abstraction

Default:

{
  type: 'put',
  data: {},
}

Standard PATCH + json:

{
  type: 'patch',
  data: {},
}

JSON Patch + json:

{
  type: 'json-patch',
  data: [{}, {}, {}],
}

amcgee · 2021-12-08T09:56:12Z

What about deprecating `update` in favor or `replace` and `patch`?

…

On Tue, Dec 7, 2021 at 9:32 AM Viktor Varland ***@***.***> wrote: The problem is that "update" as a term covers both "full updates" and "partial updates" so having "update" as a verb *and* having "patch" doesn't respect the abstraction we have chosen. That level of abstraction is "create - read - update - delete", and "patch" is a lower level of abstraction because "patch" is a kind of update. I don't see how they can live on the same abstraction level conceptually. If we go with a new type for json-patch, I think the original proposal of using jsonPatch is a good choice with the least surprises from a consumer perspective. The partial concept is not bad to me either. Since updates can be done in many ways, having a specifier for how to update is warranted. A boolean for partial isn't ideal though. Some examples below for how it can fit into our abstraction level, and how it can look if we decide to lower the abstraction level. With our current level of abstraction: Default (replace): { type: 'update', partial: false, // PUT + json data: {},} Standard PATCH + json (edit): { type: 'update', partial: true, // PATCH + json data: {},} JSON Patch + json (edit): { type: 'update', partial: 'json-patch', // PATCH + json-patch+json data: [{}, {}, {}],} Lower level of abstraction Default: { type: 'put', data: {},} Standard PATCH + json: { type: 'patch', data: {},} JSON Patch + json: { type: 'json-patch', data: [{}, {}, {}],} — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1023 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHHNMADUJQ32D2Y6C7TJ43UPXID5ANCNFSM5EQTNW3Q> .

varl · 2022-03-14T13:15:39Z

Related to dhis2/notes#330

# [3.4.0](v3.3.0...v3.4.0) (2022-03-15) ### Features * **data-service:** add json-patch support ([#1023](#1023)) ([cdcdf24](cdcdf24))

dhis2-bot · 2022-03-15T09:05:11Z

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

HendrikThePendric requested a review from amcgee September 22, 2021 07:20

feat(data-service): add json-patch support

aeda8dd

HendrikThePendric force-pushed the feat/add-json-patch-support branch from 573dfa9 to aeda8dd Compare October 14, 2021 12:03

HendrikThePendric added 4 commits October 14, 2021 14:26

fix(data-service): implement json patch request via explicit flag

0fb6504

fix(data-service): throw error on unknown type

9ab9690

test(data-service): add test for fetch type jsonPatch

2f4f7e0

test(data-service): add test for json patch request

8a7fd01

HendrikThePendric commented Oct 14, 2021

View reviewed changes

Comment thread services/data/src/links/RestAPILink/queryToRequestOptions.test.ts Outdated

fix(data-service): stringify request body for json-patch requests

9e04739

HendrikThePendric requested a review from mediremi October 14, 2021 12:59

mediremi approved these changes Oct 14, 2021

View reviewed changes

HendrikThePendric requested a review from varl October 14, 2021 13:04

HendrikThePendric added 2 commits October 14, 2021 15:12

chore: add mutation type jsonPatch

9d0345e

docs(data-service): add info about json patch

abacd56

dhis2 deleted a comment from Mohammer5 Oct 14, 2021

varl suggested changes Nov 1, 2021

View reviewed changes

Comment thread services/data/src/links/RestAPILink/queryToRequestOptions.ts Outdated

Mohammer5 approved these changes Nov 2, 2021

View reviewed changes

mediremi suggested changes Nov 3, 2021

View reviewed changes

mediremi self-assigned this Nov 3, 2021

mediremi marked this pull request as draft November 3, 2021 16:46

fix(engine): add jsonPatch to list of valid mutation types

7ae6bc2

varl self-requested a review November 4, 2021 08:02

test(engine): ensure jsonPatch mutations have data prop of type array

6932b2e

mediremi marked this pull request as ready for review November 4, 2021 16:22

chore: merge branch 'master' into feat/add-json-patch-support

7e5f5dd

mediremi approved these changes Nov 4, 2021

View reviewed changes

varl suggested changes Nov 5, 2021

View reviewed changes

amcgee self-assigned this Nov 23, 2021

mediremi mentioned this pull request Jan 11, 2022

feat(user-group): bulk group member manager dhis2/user-app#854

Merged

refactor(data-service): rename jsonPatch mutation type to json-patch

f5015d2

mediremi requested a review from varl March 14, 2022 10:53

chore: merge branch 'master' into feat/add-json-patch-support

474a6cf

varl approved these changes Mar 15, 2022

View reviewed changes

amcgee approved these changes Mar 15, 2022

View reviewed changes

mediremi merged commit cdcdf24 into master Mar 15, 2022

mediremi deleted the feat/add-json-patch-support branch March 15, 2022 08:54

dhis2-bot added a commit that referenced this pull request Mar 15, 2022

chore(release): cut 3.4.0 [skip ci]

58c9096

# [3.4.0](v3.3.0...v3.4.0) (2022-03-15) ### Features * **data-service:** add json-patch support ([#1023](#1023)) ([cdcdf24](cdcdf24))

dhis2-bot added the released label Mar 15, 2022

Conversation

HendrikThePendric commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

HendrikThePendric commented Oct 14, 2021

Uh oh!

Uh oh!

Mohammer5 left a comment

Choose a reason for hiding this comment

Uh oh!

mediremi left a comment

Choose a reason for hiding this comment

Uh oh!

varl commented Nov 4, 2021

Uh oh!

mediremi commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varl Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varl Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcgee commented Nov 10, 2021

Uh oh!

HendrikThePendric commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcgee commented Nov 16, 2021

Uh oh!

HendrikThePendric commented Nov 23, 2021

Uh oh!

amcgee commented Nov 23, 2021

Uh oh!

amcgee commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varl commented Dec 6, 2021

Uh oh!

amcgee commented Dec 6, 2021 via email

Uh oh!

varl commented Dec 7, 2021

With our current level of abstraction:

Lower level of abstraction

Uh oh!

amcgee commented Dec 8, 2021 via email

Uh oh!

varl commented Mar 14, 2022

Uh oh!

dhis2-bot commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HendrikThePendric commented Sep 22, 2021 •

edited

Loading

varl Nov 12, 2021 •

edited

Loading

varl Nov 5, 2021 •

edited

Loading

HendrikThePendric commented Nov 16, 2021 •

edited

Loading

amcgee commented Dec 4, 2021 •

edited

Loading