Skip to content
New issue

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

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

Already on GitHub? # to your account

Generated patch incorrect for Array replacement #1

Closed
tamalsaha opened this issue Mar 2, 2018 · 3 comments
Closed

Generated patch incorrect for Array replacement #1

tamalsaha opened this issue Mar 2, 2018 · 3 comments

Comments

@tamalsaha
Copy link

Here is a case that produces buggy patch:

src = {
  "spec": {
    "loadBalancerSourceRanges": [
      "192.101.0.0/16",
      "192.0.0.0/24"
    ]
  }
}

dst = {
  "spec": {
    "loadBalancerSourceRanges": [
      "192.101.0.0/24"
    ]
  }
}

The generated patch is:

[
  {
    "op": "remove",
    "path": "/spec/loadBalancerSourceRanges/0"
  },  
  {
    "op": "remove",
    "path": "/spec/loadBalancerSourceRanges/1"
  },
  {
    "op": "add",
    "path": "/spec/loadBalancerSourceRanges/0",
    "value": "192.101.0.0/24"
  }
]

This results in an error. The problem seems to be that index 1 should be removed before index 0. If I reverse the order the generated patch works.

I tested patch generation using https://github.com/stefankoegl/python-json-patch library . This produces a valid patch:

[{"path": "/spec/loadBalancerSourceRanges/1", "op": "remove"}, {"path": "/spec/loadBalancerSourceRanges/0", "value": "192.101.0.0/24", "op": "replace"}]

https://gist.github.com/tamalsaha/e8b313aec8f8d8191bc01ba504247e30

I also tested against a node.js library here: https://json8.github.io/patch/demos/apply/ . I get the same issue.

tamalsaha added a commit that referenced this issue Mar 2, 2018
Also uses edit distance to generate optimal seq for simple arrays.

Fixes #1
tamalsaha added a commit that referenced this issue Mar 3, 2018
Also uses edit distance to generate optimal seq for simple arrays.

Fixes #1
tamalsaha added a commit that referenced this issue Mar 3, 2018
Also uses edit distance to generate optimal seq for simple arrays.

Fixes #1
@nylon22
Copy link

nylon22 commented Mar 23, 2018

I was having the same issue, so I used your fork in my project, but it didn't fix it for me.

The issue I had, which seems to be the same as this, was if src was ["A", "B", "C"] and to was ["D"], the patch that was generated causes the library I'm using to apply the patch to try to replace "A" with "D", and then remove index 1 and then index 2. But after index 1 is removed, there is no index 2 anymore, causing an "Index out of bounds exception" trying to apply the patch.

My fix was to sort "remove" operations alphabetically by path and also numerically by path index (in case index >=10) in descending order, so that in the scenario above, index 2 gets removed before index 1.

Here are my changes. Would you be interested in a PR?

@tamalsaha
Copy link
Author

Patch is welcome!

@tamalsaha
Copy link
Author

tamalsaha commented Mar 27, 2018

@nylon22 , I have created #6 to address this issue. Can you take a look?

tamalsaha added a commit that referenced this issue Mar 27, 2018
Also uses edit distance to generate optimal seq for simple arrays.

Fixes #1
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants