File read edge-case bug

[iamjochem]

hi there,

I stumbled upon a small bug which occurs when all the following are true (I am sure there are other variations of loading multiple schemas that trigger the problem).

Explaination of the problem

1. you use bundle() (I have not tried with other methods but I assume they suffer in the same way)
2. you use a custom file resolver that returns Objects and caches/reuses it's responses (node's own require in my case)
3. you load/dereference 2 files in the same process
4. those 2 files both have a reference to a 3rd schema file
5. the 3rd schema file contains 2 references to a 4th schema.

Script & Schemas to reproduce

Here is a gist with a small repro-case (note that the pass.js uses lodash/deepClone to circumvent the issue):

https://gist.github.com/iamjochem/8fcb056301c4c846773f62022b9831cd

If I run fail.js then the error that I get is this:

{
  "name": "SyntaxError",
  "message": "Error resolving $ref pointer \"/work/demos/bug/b.json#/oneOf/1/properties/x\".
Token \"oneOf\" does not exist.",
}

The built-in file resolver does not have this problem because it uses fs.readFile (so you are getting a string that is JSON.parse()d everytime the reference is resolved.)

Proposed Solution

I believe the thing to would be to deep clone anything that is already an object in the JSON parser on the following line:

https://github.com/BigstickCarpet/json-schema-ref-parser/blob/master/lib/parsers/json.js#L56

e.g.

resolve(clonedeep(data));

where clonedeep is var clonedeep = require('lodash.clonedeep'); ... I use lodash in this example but you may wish to avoid another depedency and write your own version.

Info over lodash.clonedeep v4.5.0 size:

> du -sckh node_modules/lodash.clonedeep/*
4.0K	node_modules/lodash.clonedeep/LICENSE
4.0K	node_modules/lodash.clonedeep/README.md
 44K	node_modules/lodash.clonedeep/index.js
4.0K	node_modules/lodash.clonedeep/package.json
 56K	total

[JamesMessinger]

Hi @iamjochem. Thanks for opening this issue. It's always cool to see the many different ways that people are using this library.

It seems like your custom file resolver should clone the objects before returning them, rather than returning the same object instance each time. What do you think?

[iamjochem]

hi @BigstickCarpet - in my code I have aleady wrapped my resolver's require statement with the aforementioned clonedeep ... this mitigates the problem. Therefore I am not strictly in need of this fix.

BUT ... :-) ... the reason I created this issue is because I believe it is much more robust if you consequently handle the clone internally; for a number of reasons e.g.:

1. 100% less likely for you to receive ["bug"] reports related to this (I suspect that a number of issues in your list are actually related to this)
2. users won't be confronted by this issue, which given the rather abstract (but technically correct) error message may lead them on long and winding bug-hunts (it took me a hour or more to figure out what was going on - most of which was spent trying to determine if my schemas contains something invalid and pouring over the JSON-Schema spec to determine if my use of "oneOf" was invalid)
3. a consumer of input [Objects] should never mutate it's input (a principle of immutability).

I think this is a win for you and the users of your module (which I commend you for btw!) - that at the cost of one extra dependency ... I think it is worth the "cost".

You could choose to document the requirement, which is a valid alternative, but I would caution against it because.

1. people often don't read (all of) the documentation.
2. people may not understand the ramifications of the "clone" requirement.
3. people may still **** up their custom resolver implementation.

end result is that there is still a distinct possibilty of users running into this, which results in frustration (and people thinking things like "this module sucks", which is unfair) and the chance that you are stuck triages more issues (which I guess you'd rather not do!)

I'd also argue that changing the code is less work that writing documentation :-)

Obviously the choice is yours, either way I was able to work around the problem with a simple change in my own code, i.e. without having to fork and hack, so I'm personally not beholden.

Thanks for taking the time to read & respond!

[hipstersmoothie]

I think I am seeing this issue with my project as well, but the clone is not helping. I seem to be ending up with invalid references in my schema.

I am getting the error:

$ref '/properties/views/items/oneOf/5/allOf/1/properties/gatewayActions/properties/affirmativeAction/oneOf/1/properties/metaData' in 'http://localhost:8000/merged-schema.json' can not be resolved.

this points to

    "gatewayActions": {
        "type": "object",
        "properties": {
            "binding": {
                "type": "string"
            },
            "affirmativeAction": {
                "$ref": "#/properties/views/items/oneOf/5/allOf/1/properties/gatewayActions/properties/prevAction"
            },
            "negativeAction": {
                "$ref": "#/properties/views/items/oneOf/5/allOf/1/properties/gatewayActions/properties/prevAction"
            },
            "prevAction": {
                "title": "Action Choice",
                "oneOf": [
                    {
                        "title": "Action Asset",
                        "type": "object",
                        "required": [
                            "asset"
                        ],
                        "properties": {
                            "asset": {
                                "$ref": "#/properties/views/items/oneOf/1/allOf/1/properties/fields/definitions/asset/oneOf/8"
                            }
                        }
                    },
                    {
                        "title": "Action Object",
                        "description": "(DEPRECATED): Use an action asset instead.",
                        "type": "object",
                        "not": {
                            "required": [
                                "asset"
                            ]
                        },
                        "properties": {
                            "id": {
                                "type": "string"
                            },
                            "label": {
                                "$ref": "#/properties/views/items/oneOf/5/allOf/1/properties/coveredForms/properties/label"
                            },
                            "metaData": {
                                "id": "metaData",
                                "type": "object",
                                "additionalProperties": true,
                                "properties": {
                                    "role": {
                                        "type": "string",
                                        "description": "A role assigned that gives it greater semantic meaning."
                                    }
                                }
                            },
                            "value": {
                                "type": "string"
                            },
                            "applicability": {
                                "type": "string"
                            },
                            "accessibility": {
                                "type": "string"
                            }
                        }
                    }
                ]
            },
            "actions": {
                "maximum": 0
            }
        }
    }

Instead of a object, I get reference to a sibling. I believe that all references to:

/properties/views/items/oneOf/5/allOf/1/properties/gatewayActions/properties/affirmativeAction/oneOf/1/properties/metaData

should be

/properties/views/items/oneOf/5/allOf/1/properties/gatewayActions/properties/prevAction/oneOf/1/properties/metaData

Full Schema, Code, and Error: https://paste.ee/p/weaNo