chbrown / rfc6902

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

Can you add a "test" operation before a "remove" or "replace". #16

Closed nathanrobinson closed 7 years ago

nathanrobinson commented 7 years ago

For an array of simple values the test would be straightforward. Diffing

var a = { "itemCodes": [ "123", "456", "789" ] };

var b = { "itemCodes": [ "123", "789" ] };

var patch = rfc6902.createPatch(a, b);

results in

patch === [
    {op: 'test', path: '/itemCodes/1', value: "456"}, 
    {op: 'remove', path: '/itemCodes/1'}
] 

For arrays of POJOs the tests would be more complicated...

var a = { "items": [ {
        "code": "123", 
        "description": "item # 123",
        "componentCodes": [ "456", "789" ]
    }, {
        "code": "456", 
        "description": "item # 456",
        "componentCodes": [ "789" ]
    }, {
        "code": "789", 
        "description": "item # 789",
        "componentCodes": [ ]
    } ] };

var b = { "items": [ {
        "code": "123", 
        "description": "item # 123",
        "componentCodes": [ "456", "789" ]
    }, {
        "code": "789", 
        "description": "item # 789",
        "componentCodes": [ ]
    } ] };

var patch = rfc6902.createPatch(a, b);

might result in

patch === [
    {op: 'test', path: '/items/1/code', value: "456"}, 
    {op: 'test', path: '/items/1/description', value: "item # 456"}, 
    {op: 'test', path: '/items/1/componentCodes/0', value: "789"}, 
    {op: 'remove', path: '/items/1'}
]
chbrown commented 7 years ago

You can add those tests, sure, and then rollback to the original (perhaps a copy) if any of the results from rfc6902.applyPatch(...) are not null.

But what I think you want is to automatically assert that things you're about to change are the things you think they are?

I'd accept a PR for a transformation function, something like:

/** returns a patch of (equality) test Operations based on the keys
    indicated by 'patch' and the corresponding current values of 'input' */
function createTests(input: any, patch: Operation[]): Operation[] { ... }

And then you could use something like this in your consuming library:

const tests_patch = createTests(object, repl_rem_patch);
if (applyPatch(object, tests_patch).every(e => e === null)) {
    applyPatch(object, repl_rem_patch);
}

But createPatch itself is not the proper place to generate those tests. (If I'm wrong about that, please point me to appropriate part of the RFC6902 spec.)

nathanrobinson commented 7 years ago

But what I think you want is to automatically assert that things you're about to change are the things you think they are?

You are correct. You are also correct that RFC6902 deals with the structure and application of JSON patches and not how to generate them. I like your solution, but I have one question: which files should I modify in a pull request? Would it be a new function in index.ts with the logic placed in diff.ts? I would probably need to add a test file as well. Thanks.

chbrown commented 7 years ago

Admittedly, patch-creation functionality is entirely separate from the spec, but if there were something in the spec about testing values before removing/replacing them, when applying a patch, I'd want to know ;)

createTests would go in index.ts, but I don't imagine it'd be too complex:

  1. filter out non-destructive, non-manipulative operations
  2. for each remaining thing with a key: a. use pointer.tsto get the current value b. create a test operation

I.e., all the logic can probably stay in createTests.

And, yeah, a basic test or two would be great. Don't you dare reduce my 96% coverage! :)

nathanrobinson commented 7 years ago

ok, one last question... I'm assuming I missed something setting up my environment, but when I run npm test I'm getting make × ERR not found: make-$(TYPESCRIPT:%.ts=%.js). I'm using git bash on windows. Do I need to add an environment variable?

chbrown commented 7 years ago

I don't know what git bash is, and I got off Windows back in 2006. I know they (Microsoft) are doing good things these days for open source (TypeScript being one such thing), but I'm not real interested in re-exploring the pain and suffering that the Microsoft ecosystem has laid out for developers that eschew Visual Studio. :(

I just noticed TypeScript broke backwards compatibility on a minor version (2.0 compiles, 2.1 does not), so I just pushed e66bd8c, but as of that commit, the typical git clone + npm test dance works for me:

[chbrown@air ~/Desktop]$ git clone https://github.com/chbrown/rfc6902
Cloning into 'rfc6902'...
remote: Counting objects: 617, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 617 (delta 0), reused 0 (delta 0), pack-reused 614
Receiving objects: 100% (617/617), 905.07 KiB | 0 bytes/s, done.
Resolving deltas: 100% (328/328), done.
[chbrown@air ~/Desktop]$ cd rfc6902/
[chbrown@air ~/Desktop/rfc6902]$ npm test

> rfc6902@1.2.2 test /Users/chbrown/Desktop/rfc6902
> make test

npm install
npm WARN deprecated minimatch@2.0.10: Please update ...
rfc6902@1.2.2 /Users/chbrown/Desktop/rfc6902
├─┬ babel-core@5.8.38
│ ...zomg so many deps...
└── typescript@2.0.10

node_modules/.bin/tsc
node_modules/.bin/istanbul cover node_modules/.bin/_mocha -- tests/ --compilers js:babel-core/register -R spec

  array diff+patch identity
    ["A","Z","Z"] → ["A"]
      ✓ should apply produced patch to arrive at output
  ... more tests ...
    Array vs. Object
      ✓ should produce diff equal to spec patch

  105 passing (294ms)

=============================================================================
Writing coverage object [/Users/chbrown/Desktop/rfc6902/coverage/coverage.json]
Writing coverage reports at [/Users/chbrown/Desktop/rfc6902/coverage]
=============================================================================

=============================== Coverage summary ===============================
Statements   : 95.34% ( 348/365 ), 45 ignored
Branches     : 93.24% ( 138/148 ), 35 ignored
Functions    : 95.08% ( 58/61 ), 1 ignored
Lines        : 95.93% ( 259/270 )
================================================================================
cat coverage/lcov.info | node_modules/.bin/coveralls || true
nathanrobinson commented 7 years ago

Thanks for the tip. It worked fine on debian. Although I do get an intermittent tsc error compiling diff.tc.

chbrown commented 7 years ago

Yep, diff.ts:63 was the bit that confused tsc 2.1, but was fine on tsc 2.0.

I haven't been keeping up with TypeScript changes lately, but it just needs to know that the T in objects: T[] is going to be a normal object with keys that are strings.

I did not think you could use a for-in loop to iterate over anything except the string keys of an object, but tsc 2.1 apparently disagrees.

nathanrobinson commented 7 years ago

What's really odd is that typescript only complains about the key in (key_counts[key] || 0) + 1 all the other times [key] is used are fine... I submitted another PR with a fix by using (key_counts[<string>key] || 0) + 1 to reassure typescript that key is in fact a string.