1.2.0: Get operation chokes on empty path

[shockey]

Hi everyone,

I help maintain swagger-client, which depends on fast-json-patch 1.x.x. I was opening a PR today against our repository, and started seeing CI failures even though my local was fine.

I was able to isolate the issue to fast-json-patch. 1.1.8 is fine, but 1.2.0 breaks ten tests in our suite that uses apply.

Here's a piece of sample code that recreates the issue:

var fastJsonPatch = require("fast-json-patch")
var doc = {}
var patches = [{"op":"_get","path":""}]

fastJsonPatch.apply(doc, patches) // 1.2.0 throws on this call

console.log(doc) // 1.1.8 gives {}

Here's the trace:

/Users/kyle/swagger/js/node_modules/fast-json-patch/src/json-patch-duplex.js:609
                Object.keys(value_1).forEach(function (key) { return document[key] = value_1[key]; });
                       ^

TypeError: Cannot convert undefined or null to object
    at _loop_1 (/Users/kyle/swagger/js/node_modules/fast-json-patch/src/json-patch-duplex.js:609:24)
    at Object.apply (/Users/kyle/swagger/js/node_modules/fast-json-patch/src/json-patch-duplex.js:617:13)
    at Object.<anonymous>

It looks like passing an empty path in the patch is the problem. It appears that _get is an internal-ish part of the library, so it might be our fault to be leaning on it.

Let me know what y'all think - I'd be happy to make a PR if you can point me in the right direction.

[felixfbecker]

Looks like a bug to me, but FYI there is now getValueByPointer() as a public API for _get. But it uses _get under the hood, so likely affected too.

[alshakero]

Thank you for the elaborate reporting! Will get back to you ASAP.

[alshakero]

@shockey released 1.2.1!

FYI, as @felixfbecker has said, please avoid using _get operation, it is not part of the API and might be removed or changed without being considered as a breaking change. The correct alternative is getValueByPointer. And it's much cleaner.

Thanks a lot :))

[shockey]

@alshakero thanks for the quick turnaround on this! can we get an npm publish of this version?

[alshakero]

@shockey done.

[shockey]

@alshakero, I just pulled down 1.2.1 and I'm seeing the same issue 😕

Here's my example running against the latest version: https://runkit.com/shockey/593194bafad712001247593c

[felixfbecker]

It works for me with getValueByPointer

[alshakero]

@shockey I was a little hesitant to fix an edge case for a deprecated API. But published 1.2.2 🎊

I've just noticed that your example isn't correct BTW. If you want to _get a value, you should use patch.value after you call apply. It worked in 1.8.1 because apply didn't throw an error and doc is already doc. Calling apply was, in fact, extraneous in your example.

Here is an example of all possible ways: https://runkit.com/alshakero/5931aa4204544700129506a3

Please use getValueByPointer, I'd appreciate your effort, even more, testing the actual API in the wild.

[shockey]

@alshakero thanks for the fix! tests are looking good again in our repository.

I'll open a ticket to refactor the _get usage to getValueByPointer on our end, but we've kinda made our bed with this: all our currently-released versions of swagger-client target the current major version of fast-json-patch. I tightened the dependency to 1.1.8 to avoid the deprecation warning for future versions, but our users of existing versions would be in a not-fun situation without this fix being made. We're looking into version locking as a way to avoid this in the future.

Sorry if my example was a bit contrived - I simply observed what was going into the call that broke things and posted it as a test case.

In you're interested in knowing, we construct an object for fast-json-patch here in our codebase. fast-json-patch 1.2.0 and 1.2.1 caused this allOf test to fail, because our allOf plugin was constructing a _get patch with an empty path.

I (along with our OSS team!) appreciate you going the extra mile on this. Cheers! 🍻

[alshakero]

More than welcome! Cheers! 🍻