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

Switch to APIDevTools/json-schema-ref-parser #1054

Open
philsturgeon opened this issue Apr 2, 2020 · 20 comments
Open

Switch to APIDevTools/json-schema-ref-parser #1054

philsturgeon opened this issue Apr 2, 2020 · 20 comments
Labels
breaking Pull requests that introduce a breaking change tech-debt triaged

Comments

@philsturgeon
Copy link
Contributor

Currently we use our home grown json-ref-resolver, but we've switched most of our ecosystem over to APIDevTools/json-schema-ref-parser for it's superior bundling strategy (https://github.com/stoplightio/studio/issues/275) amongst other things.

We've got some outstanding pull requests going on, but so to avoid extra work let's wait for APIDevTools/json-schema-ref-parser#153 to be merged, then switch Spectral over.

@philsturgeon
Copy link
Contributor Author

Work was started on this a while back but not completed. Feel free to use this as a base: https://github.com/stoplightio/spectral/tree/feat/schema-ref-parser

@lehphyro
Copy link

Are you guys planning to finish the switch soon? The linked issues forced us to disable linting in our projects due to random errors produced by failure to resolve references properly.

@philsturgeon
Copy link
Contributor Author

@lehphyro most of the examples in the linked issue (#870) have workarounds, and some of them even seem to indicate user error, so we've focused on frying bigger fish for now. Is there no workaround for your infinitely circular dependency scenario?

@lehphyro
Copy link

@philsturgeon It's not about only circular refs, I've created a repo that you can use to reproduce the issue: https://github.com/lehphyro/spectral-ref-issue

I usually use maven to run node tools, so you can reproduce it by cloning that repo and then running mvn clean install from the root directory, running spectral binary directly yields the same result.

This is the error you should see:

$ spectral lint src/main/resources/openapi.yaml --ruleset openapi-rules.yaml
OpenAPI 3.x detected

/repo/spectral-ref-issue/src/main/resources/examples/response.json
 1:1  error  examples-responses-are-valid  can't resolve reference schemas/statement.yaml from id #

✖ 1 problem (1 error, 0 warnings, 0 infos, 0 hints)

@philsturgeon
Copy link
Contributor Author

Interesting. There may well be some sort of issue from having a model load up the main openapi file again, which is in turn trying to load up a bunch of refs and then some of them call back to the main file, and on it goes.

I'd switch this:

  documents:
    description: List of documents provided by the bank
    items:
      $ref: '../openapi.yaml#/components/schemas/AccountStatementIdentification'
    type: array

to this:

  documents:
    description: List of documents provided by the bank
    items:
      $ref: schemas/statement-id.yaml
    type: array

See if that sorts you out.

@lehphyro
Copy link

I cannot unfortunately, I need to have all $refs point to the components section in order to have https://openapi-generator.tech/ generate code with good class names.

@lehphyro
Copy link

lehphyro commented Jun 4, 2020

Should I assume that this won't be done any time soon? Since I'll have to switch to another linter entirely, it'd be nice to know it before making the effort.

@philsturgeon philsturgeon added the breaking Pull requests that introduce a breaking change label Jul 9, 2020
@philsturgeon
Copy link
Contributor Author

philsturgeon commented Jul 10, 2020

@lehphyro Lots of work is being done on json-schema-ref-parser and several releases have been made since these discussions started, so I'm sure whatever concerns you have with it will be solved soon enough, if not already.

The plan is to wrap up the resolution logic in an adapter pattern as part of v6, and provide an adapter for both resolvers - or allow people to pass in their own entirely - therefore separating the resolution and linting logic entirely. You can keep using the old one until json-schema-ref-parser has solved whatever problems are keeping you off it.

Consolidation of efforts is the goal here, meaning two teams are working on improving one tool instead of us both playing whackamole with various edge cases. Neither are good or bad, they've just both got different problems, but json-schema-ref-parser generally has fewer of them, and is getting a lot of PRs from us to remove the rest. Soon comes the new $ref logic for $id and OAS3.1/JSON Schema 2019-09 logic (APIDevTools/json-schema-ref-parser#145), so... no point us all doing that twice.

@lehphyro
Copy link

@philsturgeon thank you for the update, I'll give it a try when a working solution becomes available.

@sebas2day
Copy link

If you (accidentally) have a reference inside the spec to the spec itself and run spectral lint ./self-ref-spec.yaml it will run into an infinite loop and eat up your memory. Not sure if this is a known issue but it might seem related.

Any idea if switching to this new json-schema-ref-parser package would fix this as well?

@philsturgeon
Copy link
Contributor Author

philsturgeon commented Jun 16, 2021 via email

@jarrodparkes
Copy link

@philsturgeon any updates on this? is there anyway to bypass circular references, use the new parser, etc in the meantime?

@philsturgeon
Copy link
Contributor Author

Ping @mnaumanali94

@magicmatatjahu
Copy link
Contributor

magicmatatjahu commented Sep 2, 2022

@philsturgeon @mnaumanali94 Hi! Any update? From what I can see in the Stoplight Studio sources a new resolver based on json-schema-ref-parser is already in use (I could be wrong, I'm looking at the minified code). I don't have any problems with the current resolver, however, what hurts me the most is that it doesn't keep the same reference (in JS, after dereferencing) between $refs, example:

type: object
properties:
  someProperty:
    type: object
    properties:
      deepProperty:
        $ref: '#/properties/anotherProperty'
  anotherProperty:
    type: string

and comparison:

$.properties.someProperty.properties.deepProperty === $.properties.anotherProperty

equals false 😢 I can "fix" that using documentInventory but it's a big hack to reassign references again.

@magicmatatjahu
Copy link
Contributor

magicmatatjahu commented Sep 2, 2022

Sorry everyone! This is an error in my code. Sorry for the problem but question Any update? is still active 😄

@smoya
Copy link
Collaborator

smoya commented Apr 26, 2023

Any update on the effort to do the migration? Especially considering the fact https://github.com/stoplightio/json-ref-resolver is marked as deprecated.

Thanks 🙏

@ankorstore-haddowg
Copy link

any update on this, having a number of issues with non-deterministic results which seems related to our usage of $refs

@afmhenry
Copy link

Also running into issues with this when running spectral on multiple files inside the same cli invocation.

@derberg
Copy link

derberg commented Jul 3, 2023

@mnaumanali94 hey any ideas on when will that be addressed?

@derberg
Copy link

derberg commented Jul 15, 2024

@mnaumanali94 hey, does triaged mean it will be taken into account. Usage of this deprecated package causes lots of issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Pull requests that introduce a breaking change tech-debt triaged
Projects
None yet
Development

No branches or pull requests

10 participants