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

Only support full RFC6902 in OIP042 Edits #18

Closed
OstlerDev opened this issue Sep 3, 2019 · 4 comments
Closed

Only support full RFC6902 in OIP042 Edits #18

OstlerDev opened this issue Sep 3, 2019 · 4 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@OstlerDev
Copy link
Collaborator

As MLG has been using the OIP042 Edit spec for the past month or two, we have occasionally ran into an issue with how Edits use a Squashed version of the RFC6902 Patches.

While squashing the patch works in most instances, there are a couple specific semi-common cases that the squashed patches cannot handle. The main issue occurs when multiple items are being added to an array, as if there are multiple add operations for an Array, and they are “appending” the data, the squashed patch format will only pick up on one of the adds and ignore the second. The additional issues stem from how RFC6902 patches are Sequential, and how the Squashed Patch format removes any ordering.

The main bug logic can be demonstrated as follows using js-oip. Notice how we lose the “add” for “three”.

let rfc6902Patch = edit.createRFC6902Patch({ field: [ 'one', 'two' ] }, { field: [ 'two', 'three', 'four' ] })
// rfc6902Patch = [
//    {"op": "remove", "path": "/field/0"}, 
//    {"op": "add", "path": "/field/-", "value": "three"}, 
//    {"op": "add", "path": "/field/-", "value": "four"}
// ]
let squashedPatch = edit.squashRFC6902Patch(rfc6902Patch)
// squashedPatch = { 
//    "add": { "/field/-": "four" }, 
//    "remove": ["/field/0"] 
// }

We propose that we retroactively remove support for the Squashed Patch format from js-oip and OIPd and only support patches containing a full RFC6902 formatted JSON Patch. MLG would then publish "replacement" edits for records that we have already edited. We at MLG think this way forward would be simplest for the long term support of OIPd, in order to prevent having to maintain the squashed patch format forever.

Currently MLG is the only user of OIP042 Edits, so this should luckily not affect anybody else.

We propose that the format of an OIP042 Edit be defined as the following:

{
  "oip042": {
    "edit": {
      "artifact": {
        "artifactID": "string",
        "timestamp": integer,
        "patch": // RFC6902 JSON Patch https://tools.ietf.org/html/rfc6902
      },
      "signature": "string"
    }
  }
}

An example edit would look like:

{
  "oip042": {
    "edit": {
      "artifact": {
        "artifactID": "a66b77ec997a72b649fd8e2e256f39f698b914c8c35ea87dc6b0ffb7c2fd8c29",
        "timestamp": 1567526939603,
        "patch":[
          {"op": "remove", "path": "/field/0"}, 
          {"op": "add", "path": "/field/-", "value": "three"}, 
          {"op": "add", "path": "/field/-", "value": "four"}
        ]
      },
      "signature": "IPra1n/c8Zd76cTwNMgM6I4SVc/r6MBo+KQdVoWoOxi8JZvGMmjYO/gazJl0llZoiAQ+BBNYrDGYfHnAJYUywMA="
    }
  }
}
@OstlerDev OstlerDev added bug Something isn't working enhancement New feature or request labels Sep 3, 2019
@OstlerDev OstlerDev self-assigned this Sep 3, 2019
@bitspill
Copy link
Member

bitspill commented Sep 3, 2019

I thought we maintained an array of each operation in the squashed form so it should still support multiple Add.

https://oip.wiki/Squash_Edit
The JS on the wiki seems to show an array being created for each operation

Due to arrays maintaining order, the order of operations would remain within an op type however as mentioned order between types would indeed be lost and lead to potential edge cases resulting in invalid final output such as the index removed/replaced being incorrect if an insert were processed first

Overall, it's probably safest to fall back to full spec RFC6902 if MLG doesn't mind invalidating and rebroadcasting all current oip042 edits.

@OstlerDev
Copy link
Collaborator Author

We had initially thought about going down the route of using the arrays for each operation, but decided that for the full ordering between op types, it was worth falling back to the RFC6902 spec.

MLG is ok with invalidating and rebroadcasting all of our current OIP042 edits :)

I am going to work on developing code to perform this work and will create a PR referencing this issue once I have tested it and believe it to be feature complete.

@cchrysostom
Copy link
Member

Yes, MLG will invalidate the squashed edits. I support making this adjustment.

@OstlerDev
Copy link
Collaborator Author

I have opened this PR that will fix this issue once it is merged :)

#19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants