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

Fails to resolve correct path/filename for extended $ref #200

Closed
daniellubovich opened this issue Dec 11, 2020 · 19 comments
Closed

Fails to resolve correct path/filename for extended $ref #200

daniellubovich opened this issue Dec 11, 2020 · 19 comments

Comments

@daniellubovich
Copy link

I recently ran into an issue testing some of my more complex schema documents where combination schema $refs cannot be resolved properly if they are part of an "extended" $ref. It is a little hard to explain the case, but I think I have found a simple enough schema that causes the problem to occur.

// base.json
{
  "type": "object",
  "properties": {
    "some_value": { "$ref": "defs.json#/definitions/astring" }
  }
}

//

{
  "definitions": {
    "astring": {
      "description": "astring",
      "$ref": "defs2.json#/definitions/bstring"
    }
  }
}


// defs2.json
{
  "definitions": {
    "bstring": {
      "oneOf": [
        {
          "$ref": "#/definitions/cstring"
        },
        {
          "$ref": "#/definitions/dstring"
        }
      ]
    },
    "cstring": {
      "type": "string"
    },
    "dstring": {
      "type": "number"
    }
  }
}

The error that I see when trying to resolve this looks like:
Error while processing route: dynamic-form Token "cstring" does not exist. MissingPointerError: Token "cstring" does not exist.

This only seems to happen when the intermediate schema object is an "extended" ref. For example, if we remove the description from astring then we are able to dereference without issue. From doing some debugging it looks to me like our issue is that when resolving cstring, the path we resolve is defs.json#/definitions/cstring instead of looking in defs2.json. I think this is because when we resolve extended $refs, we explicitly do not update the pointer's path.

For reference:

if ($Ref.isExtended$Ref(pointer.value)) {
// This JSON reference "extends" the resolved value, rather than simply pointing to it.
// So the resolved path does NOT change. Just the value does.
pointer.value = $Ref.dereference(pointer.value, resolved.value);
return false;
}

I would like to propose a fix, but feel ill-equipped to do so without some guidance. It does seem like updating the path in the above code fixes my specific issue, but clearly that decision was made for a reason so I'd rather not step on any toes without understanding the intent. Any thoughts would be greatly appreciated!

@daniellubovich
Copy link
Author

I should also mention the issue described here: #168.

Based on the draft 07 spec, it seems like the behavior of objects with $ref and additional properties appears to be undefined. I think the decision on how to proceed with this behavior going forward may inform the decision made for this issue as well.

@karlvr
Copy link

karlvr commented Mar 11, 2021

This feels like the same problem that I've had, but I'm not sure what an extended ref is!

The problem I've had is that when it comes time to resolve (using your example):

    "bstring": {
      "oneOf": [
        {
          "$ref": "#/definitions/cstring"
        },
        {
          "$ref": "#/definitions/dstring"
        }
      ]
    },

... I've lost the context that these $refs are in defs2.json and it's just looking for #/definitions/cstring in my main document.

So I've patched resolve-external.js to "correct" refs as we crawl the external document so that they reference the external document.

I'm still testing this myself, but it seems promising. Here's the patch if it sounds like it might address this issue for you.

diff --git a/node_modules/@apidevtools/json-schema-ref-parser/lib/resolve-external.js b/node_modules/@apidevtools/json-schema-ref-parser/lib/resolve-external.js
index c7238fb..d1f1824 100644
--- a/node_modules/@apidevtools/json-schema-ref-parser/lib/resolve-external.js
+++ b/node_modules/@apidevtools/json-schema-ref-parser/lib/resolve-external.js
@@ -51,7 +51,7 @@ function resolveExternal (parser, options) {
  * If any of the JSON references point to files that contain additional JSON references,
  * then the corresponding promise will internally reference an array of promises.
  */
-function crawl (obj, path, $refs, options) {
+function crawl (obj, path, $refs, options, external) {
   let promises = [];
 
   if (obj && typeof obj === "object" && !ArrayBuffer.isView(obj)) {
@@ -59,16 +59,17 @@ function crawl (obj, path, $refs, options) {
       promises.push(resolve$Ref(obj, path, $refs, options));
     }
     else {
+      if (external && $Ref.is$Ref(obj)) {
+        /* Correct the reference in the external document so we can resolve it */
+        let withoutHash = url.stripHash(path);
+        obj.$ref = withoutHash + obj.$ref;
+      }
+
       for (let key of Object.keys(obj)) {
         let keyPath = Pointer.join(path, key);
         let value = obj[key];
         
-        if ($Ref.isExternal$Ref(value)) {
-          promises.push(resolve$Ref(value, keyPath, $refs, options));
-        }
-        else {
-          promises = promises.concat(crawl(value, keyPath, $refs, options));
-        }
+        promises = promises.concat(crawl(value, keyPath, $refs, options, external));
       }
     }
   }
@@ -107,7 +108,7 @@ async function resolve$Ref ($ref, path, $refs, options) {
 
     // Crawl the parsed value
     // console.log('Resolving $ref pointers in %s', withoutHash);
-    let promises = crawl(result, withoutHash + "#", $refs, options);
+    let promises = crawl(result, withoutHash + "#", $refs, options, true);
 
     return Promise.all(promises);
   }

karlvr added a commit to karlvr/json-schema-ref-parser that referenced this issue Mar 11, 2021
Possibly corrects APIDevTools#200.

This however causes tests to fail as they’re not expecting the references to be resolved. I’m not sure yet how to fix the tests. I thought I’d wait to see if the fix is valid!
@ajcool2k
Copy link

@daniellubovich Thanks for reporting this. I almost gave up until I saw the issue you created.

I believe I run into the same issue here. Went with a workaround nesting the $ref inside an oneOf to keep the description. Not sure if there's a better way but it fixes the resolving issues I run into.

{
    "parameters": {
        "description": "My parameter description ...",
        "oneOf": [
            {
                "$ref": "./external.schema.json#/definitions/ParameterDefinition"
            }
        ]
    }
}

@btruhand
Copy link

@daniellubovich if I get the problem correctly, it seems to be a case of nested references to external files? That is:

main file --- references ---> external file 1 --- references ---> external file 2

At least this seems to be the issue that I'm getting and guessing that this is the underlying problem.

I'm in a similar position where I'd be happy to help but would appreciate some guidance on it. From my perspective it seems there is at least a need to be able to propagate referencing context such that at the time of dereferencing the correct value can be obtained from the correct reference location

@karlvr
Copy link

karlvr commented Mar 31, 2021

@btruhand if you get a moment, your issue sounds like the issue that I've resolved. I've published a version of json-schema-ref-parser as @openapi-generator-plus/json-schema-ref-parser if you'd like to test it to see if it resolves your problem?

@btruhand
Copy link

btruhand commented Apr 7, 2021

@karlvr just got back to replying on this. I actually realized my problem is elsewhere (at least for the case that I was testing). Though I'll try to give your solution a whirl and see how it goes and get back with results for future reference

@benjamin-mogensen
Copy link

@karlvr where can I find your published package? I am facing an issue where json-schema-ref-parser creates references that are not supported by AJV and AJV can therefore not resolve them (see issue reported here). This is a huge problem for me because the schemas are really larger and I don't want to have to "hardcode" some replacement paths to have it AJV compliant (although at the moment that is my only choice).

@karlvr
Copy link

karlvr commented Apr 11, 2021

@benjamin-mogensen Right, hopefully it resolves the issue. I published it https://www.npmjs.com/package/@openapi-generator-plus/json-schema-ref-parser for interoperability with my https://www.npmjs.com/package/openapi-generator-plus. The source code is https://github.com/karlvr/json-schema-ref-parser. The master branch contains what's published to npm. I've also recently fixed the bundle task as well as I'm using that to convert schemas referencing external files into a single self-contained schema for use with tools that don't seem to support external references.

(I've fixed the reference to where I'd published it above; I was publishing a few things at the time!)

@benjamin-mogensen
Copy link

Thanks a lot for getting back on this. I tried using your package, bundled the schemas and then checked if the $ref -> $ref problem had been fixed. Unfortunately not so AJV still cannot compile the bundled schemas and I am forced to hard code a fix that replaces these "broken" paths in the $ref so that AJV can compile without errors.

@karlvr
Copy link

karlvr commented Apr 13, 2021

@benjamin-mogensen interesting; can you post an example file that it doesn't fix? My use case is resolved but I'm happy to look if this is related to what I've fixed.

@benjamin-mogensen
Copy link

@karlvr thanks for your reply - The schemas I bundle are very big and takes many $ref which in turn has many other $ref so it is a bit difficult to make a simple example. I will see if I can one of the coming days though. The result I get (in a super stripped down version) is like the link I posted above. But I guess the end result will not help us much finding out if we can fix this in json-schema-ref-parser :)

@y0n3r
Copy link

y0n3r commented Jun 14, 2022

I am having this same issue. It's disappointing that it's still causing problems. If I have a $ref pointer that shows up after a "extended" $ref, as OP calls it, then the file can't be found.

CampaignResponse:
  allOf:
    - $ref: './Campaign.yml' # works just fine, no problem
      required:
        - id
        - state
        - candidates
      properties:
        id:
          type: string
        state:
          type: string
        revision:
          type: number
        publishedBy:
          $ref: './ChangeLogEntry.yml' # ERROR! Can't find file! (even though it DEFINITELY exists in the specified location)
        candidates:
          type: array
          items:
            $ref: './Candidate.yml'

As I'm sure you're all aware, this is incredibly frustrating because it's forcing me to have to duplicate data structures in multiple places just to get it to work properly. It shouldn't have to be this way. All explicit references should simply work. Any suggestions/comments/hacks would be warmly welcomed and greatly appreciated 🙏🏾

@philsturgeon
Copy link
Member

@y0n3r this isn't a simple thing, and lets not just stamp our feet and talk about being disappointed. All time spent on this is entirely voluntary, and I already run a reforestation charity with about 40 hours of my free time each week, so sometimes issues dont get seen to as fast as we'd all like.

@karlvr thank you for submitting a pull request, I'll talk to you over there about getting it moving.

Everyone else, if you're seeing this issue, please give https://www.npmjs.com/package/@openapi-generator-plus/json-schema-ref-parser a try and see if it fixes that issue. We'll get the changes merged upstream ASAP.

karlvr added a commit to karlvr/json-schema-ref-parser that referenced this issue Jul 5, 2022
Possibly corrects APIDevTools#200.

This however causes tests to fail as they’re not expecting the references to be resolved. I’m not sure yet how to fix the tests. I thought I’d wait to see if the fix is valid!
@Borduhh
Copy link

Borduhh commented Sep 20, 2022

I am having a similar issue with both packages.

The folder structure looks like:

jsonSchemaParser
|__schemas
    |__src
        |__ common
            |__ enums
                |__ us-states.json
            |__ physical-address.json
    |__ location-created.json

location-created.json

{
  "$schema": "http:https://json-schema.org/draft-07/schema",
  "title": "LocationCreated",
  "type": "object",
  "properties": {
    "data": {
      "type": "object",
      "properties": {
        "id": {
          "type": "string"
        },
        "name": {
          "type": "string"
        },
        "physicalAddress": {
          "$ref": "schemas/src/common/physical-address.json"
        }
      },
      "required": ["id", "name", "physicalAddress"]
    }
  },
  "required": ["data"]
}

location-created.json

{
  "$schema": "http:https://json-schema.org/draft-07/schema",
  "title": "PhysicalAddress",
  "type": "object",
  "properties": {
    "streetNumber": {
      "type": "string"
    },
    "streetName": {
      "type": "string"
    },
    "streetDetails": {
      "type": "string"
    },
    "city": {
      "type": "string"
    },
    "state": {
      "$ref": "enums/us-states.json"
    },
    "countryCode": {
      "type": "string",
      "enum": ["US"]
    },
    "postalCode": {
      "type": "string"
    }
  },
  "required": ["streetNumber", "streetName", "city", "state", "countryCode", "postalCode"]
}

us-states.json

{
  "$schema": "http:https://json-schema.org/draft-07/schema",
  "title": "USStates",
  "type": "string",
  "enum": [
    "AL",
    "AK",
    "AZ",
    "AR",
    "CA",
    "CO",
    "CT",
    "DE",
    "FL",
    "GA",
    "HI",
    "ID",
    "IL",
    "IN",
    "IA",
    "KS",
    "KY",
    "LA",
    "ME",
    "MD",
    "MA",
    "MI",
    "MN",
    "MS",
    "MO",
    "MT",
    "NE",
    "NV",
    "NH",
    "NJ",
    "NM",
    "NY",
    "NC",
    "ND",
    "OH",
    "OK",
    "OR",
    "PA",
    "RI",
    "SC",
    "SD",
    "TN",
    "TX",
    "UT",
    "VT",
    "VA",
    "WA",
    "WV",
    "WI",
    "WY",
    "DC",
    "AS",
    "GU",
    "MP",
    "PR",
    "UM",
    "VI"
  ]
}

In location-created.json I have tried the following:

path from base dir

{
    "physicalAddress": {
      "$ref": "schemas/src/common/physical-address.json"
    }
}

Which seems to duplicate the root directory and gives the error:

Error: Error resolving $ref pointer "/jsonSchemaParser/jsonSchemaParser/schemas/src/common/physical-address.json#". 
"/jsonSchemaParser/jsonSchemaParser/schemas/src/common/physical-address.json" not found.

path from dir where location-created.json resides

{
    "physicalAddress": {
        "$ref": "common/physical-address.json"
    }
}

Which seems to then work from the base directory, but then correctly errors out:

ResolverError: Error opening file "/jsonSchemaParser/common/physical-address.json" 
ENOENT: no such file or directory, open '/jsonSchemaParser/common/physical-address.json'

@ahamid
Copy link

ahamid commented Oct 4, 2022

I'll add that I think I'm seeing the same issue when parsing an external doc that references its own definitions relatively via #/path/def. For example, this doc, and peer external reference to jsf schema:

https://raw.githubusercontent.com/CycloneDX/specification/master/schema/bom-1.4.schema.json
references types in:
https://raw.githubusercontent.com/CycloneDX/specification/master/schema/jsf-0.82.schema.json

fails with MissingPointerError: Token "signer" does not exist.\n.

My workaround is to fully qualify the internal references in jsf-0.8.2.schema.json from #/definitions/signature to jsf-0.82.schema.json#/definitions/signature.

@karlvr FWIW with @openapi-generator-plus/json-schema-ref-parser I get a different error related to resolving the external schema file itself:

Error: Error resolving $ref pointer "/Users/ahamid/ws/web/export/schema/schema/jsf-0.82.schema.json#/definitions/signature"
^^^ doubled .../schema/schema/... parent dir.

Update: I take this back, I believe the my problem is due to an incompatibility with another change in the downstream json-schema-to-typescript module which needs to be reconciled with this change which I think would otherwise wfm. Thanks for addressing this!

@mikethecalamity
Copy link

@JamesMessinger @P0lip has anyone looked into this? Maybe PR #220 is good? Seems like a big issue affecting a lot of people.

@danielfcollier
Copy link
Contributor

Hi there,

can someone help to write tests for this feature?

I've tested with npm run coverage:node @karlvr branch and the tests are not passing, is that right? Please, correct with I'm wrong.

There is a starting point for the tests here: e121717

(I've added a branch with changes from this PR on top of some changes done for ISSUE-283 for tests on Windows.)

@wobo-ttemple
Copy link

I had this issue and was able to implement a quick fix by adding the @ApiExtraModels() decorator inside my controller endpoint and adding the missing ref that way so it would show up in the yaml file

@bmoyroud
Copy link

bmoyroud commented Jan 14, 2023

Yup wanted to thank you @philsturgeon for your work.
The solution above works great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests