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

fix: recursively resolve and dereference extended refs #45

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

skyqrose
Copy link

(tests don't pass, not ready to merge, see description)

Motivation and Context

Resolves #44

This is blocking my usage of this library, and I don't have any other workaround besides writing my own custom resolving or dereferencing.

Description

  • When crawling over the schema to resolve or bundle its refs, remove an else so that the crawl recurses into the children of an object even if it has a $ref.
  • New unit tests.

Unfortunately there's also a bug in dereferencing, and I've tried to fix it, but the dereferencing recursion is more complicated and I wasn't able to figure it out on my own. This would need attention from a maintainer to fix it.

Specifically the new test fails with

MissingPointerError: at "#/local", token "$defs" in "#/$defs/local" does not exist
  1) Schema with a $ref nested inside an extended $ref
       should dereference successfully:
     MissingPointerError: at "#/local", token "$defs" in "#/$defs/local" does not exist
      at Pointer.resolve (lib/pointer.js:100:13)
      at $Ref.resolve (lib/ref.js:126:28)
      at $Refs._resolve (lib/refs.js:165:15)
      at dereference$Ref (lib/dereference.js:115:23)
      at crawl (lib/dereference.js:62:26)
      at dereference$Ref (lib/dereference.js:157:24)
      at crawl (lib/dereference.js:49:22)
      at dereference (lib/dereference.js:21:22)
      at $RefParser.dereference (lib/index.js:270:5)
      at async Context.<anonymous> (test/specs/nested-extended/nested-extended.spec.js:36:20)

And in lib/dereference.js:dereference$Ref the path is wrong. The log statement (currently commented out) incorrectly says

Dereferencing $ref pointer "#/$defs/local" at /Users/srose/git/json-schema-ref-parser/test/specs/nested-extended/referenced-root.yaml#/local`

(note that referenced-root.yaml#/local doesn't exist)

If you remove the top level $ref: "referenced-root.yaml" from test/specs/nested-extended/nested-extended.yaml (so it's not an extended ref anymore), that same log says (correctly)

Dereferencing $ref pointer "#/$defs/local" at /Users/srose/git/json-schema-ref-parser/test/specs/nested-extended/nested-extended.yaml#/local

I tried to fix this, but I didn't understand enough about how paths are made and what they mean. Someone who's more familiar with the code would need to fix dereference.js so that the new test passes.

How Has This Been Tested?

  • New unit tests for parsing and resolving extended refs.
  • New unit test for dereferencing, that doesn't pass and I can't fix on my own.
  • New unit test for bundling, which also fails, and I don't know if it's because of the dereferencing issue or because I misunderstood how bundling is supposed to work.
  • Other unit tests (mostly) pass locally. (I'm pretty sure it's just flaky CI tests that fail.)
  • I've tried to run on my own project, to check that it works in a real-world use case, but got stuck on the dereferencing issue, so can't confirm this yet.

Screenshot(s)/recordings(s)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

This is a behavior change, so could potentially be considered breaking, but I think it still falls under "bug fix".

Checklist

@brendarearden
Copy link

I just merged in #47 to address the MissingPointerError for external refs - it might resolve the MissingPointerError you are experiencing with this PR! 🤞

@skyqrose
Copy link
Author

skyqrose commented Oct 3, 2023

I've merged in those changes from upstream. Unfortunately, I'm still getting the same MissingPointerError. Perhaps the same fixes from #47 or this PR needs to be applied to dereference.js in addition to resolve-external.js?

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

Successfully merging this pull request may close these issues.

doesn't resolve $refs if a parent had a $ref
2 participants