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

WIP: resolve refs in schemas in one extrapolated hash #222

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

Conversation

RST-J
Copy link
Contributor

@RST-J RST-J commented Jan 24, 2015

This is a first shot at generating a fully resolved schema hash as requested in #32.
I commented very verbosely so you can directly follow my ideas. Any comments, improvements et al?

Notes:

  • So far this only works with $ref but can be easily extended for extends too
  • in the test test_linked_list_ref using the file path instead of the URL does not work (see comment)
  • pretty printing a cyclic hash or using it for validation kills kittens

Questions:

  1. Has someone an idea why using the file path in the test does not work?
  2. Is there anything missing to be draft4 compliant?

@@ -137,6 +137,10 @@ def load_ref_schema(parent_schema, ref)
build_schemas(schema)
end

def base_schema
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's normal to use attr_accessor for this kind of thing.

@inglesp
Copy link

inglesp commented Feb 6, 2015

I've had a look at this because it's a feature that I'd like to see in the gem. I think the approach is sensible, but I don't know the library very well so there might be some edge cases I'm missing.

I've made a handful of stylistic comments which I hope are helpful!

@RST-J
Copy link
Contributor Author

RST-J commented Feb 7, 2015

Thanks for your feedback. I'll see what I can do about the names and particularly list is really not self describing.

@BGarretson
Copy link

I tested this branch using the following test:

# test
    context 'testing schema resolver code' do
        it 'does not throw an error' do
            schema = JSON::Validator.get_schema_body('./schemas/iodocs/v2/expanded/iodocs_schema_v2.0.json')
            resolved = JSON::Validator.fully_resolve(schema)
            json = resolved.to_json(max_nesting: 10000)
            json
        end
    end

It generated a NestingError regardless of the depth I allowed in the call above - this is definitely a serialization issue where the object being serialized has infinite depth due to circular references in the way the hashes are related to each other.

JSON::NestingError: nesting of 10000 is too deep
./spec/lib/cli_spec.rb:156:in `to_json'
./spec/lib/cli_spec.rb:156:in `block (3 levels) in <top (required)>'
/Users/bgarrets/.rvm/gems/ruby-2.0.0-p451/gems/ruby-debug-ide-0.4.24/lib/ruby-debug-ide.rb:86:in `debug_load'
/Users/bgarrets/.rvm/gems/ruby-2.0.0-p451/gems/ruby-debug-ide-0.4.24/lib/ruby-debug-ide.rb:86:in `debug_program'
-e:1:in `load'
-e:1:in `<main>'

It's not necessarily enough to create a hash in memory and reuse existing hash object references if they can be reused during buildout. In any sufficiently complex schema where objects are being reused in multiple places in the schema hierarchy you'll end up with circular references that can't then be serialized out.

To get around this, you would need to basically collapse every schema you encounter into the 'new schema file's definitions block, possibly generating new definition entries for schemas that were the root schema in a file, and updating $ref statements to point to the new schema location within the single file schema instead of reaching out to other files.

@BGarretson
Copy link

The suggestion above is basically the process I had to go through to manually create a single-file schema from the multi-file schema set as a manual workaround for my colleague.

@BGarretson
Copy link

Alternately, you could monitor for circular references and only allow the method to resolve a schema if it was a directed acyclical graph

@RST-J
Copy link
Contributor Author

RST-J commented May 18, 2015

I like your first idea better and will have a look at how to do this when I find time.

@Jubke
Copy link

Jubke commented May 2, 2016

this has been quiet for a while... any updates or chances it will be finished and merged?

@ubhide
Copy link

ubhide commented Jul 6, 2020

Has this been abandoned?

@DeltaRazero
Copy link

Any progress on this? This is very trivial feature for a web application that I am working on right now.

@bastelfreak
Copy link
Member

@DeltaRazero we need to rebase this to merge it. If you're interested you can checkout the branch, rebase against our master and submit it as new PR.

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.

8 participants