brandur / json_schema

A JSON Schema V4 and Hyperschema V4 parser and validator.
MIT License
230 stars 45 forks source link

json_schema >= 0.14.0 errors while reference expanding against a schema which has reference to a link description object #79

Closed okitan closed 7 years ago

okitan commented 7 years ago

This is a brief sample code.

json = <<_JSON_
{
    "id": "http://example.com/poc.json",
    "$schema": "http://json-schema.org/draft-04/hyper-schema#",
    "definitions": {
        "getSelfEndpoint": { "$ref": "#/links/0" }
    },
    "properties": {
        "id":   { "type": "string" }
    },
    "links": [
        {
            "rel": "self",
            "method": "GET",
            "href": "/{id}"
        }
    ]
}
_JSON_

schema = JsonSchema::Parser.new.parse!(JSON.parse(json))
schema.expand_references #=> error!

It causes lib/json_schema/reference_expander.rb:156:inresolve_pointer': undefined method reference' for #<JsonSchema::Schema::Link:0x007f932b169390> (NoMethodError)

That is because the schema refers to LDO but JsonSchema::Schema::Link does not support reference and fails.

brandur commented 7 years ago

Ugh, my fault. Thanks for the report!

brandur commented 7 years ago

@okitan I released a fix for the problem in 0.14.2. Sorry about the trouble and let me know if you notice anything else.

brandur commented 7 years ago

Hm, something is still not quite here ... the test suite passed, but there seems to be another problem. I ended up yanking 0.14.2 and would recommend holding off on an upgrade for now.

brandur commented 7 years ago

Alright, 0.14.3 is out instead.

okitan commented 7 years ago

@brandur

I appreciate for your quick fix. Actually it works, but I'm afraid I have some concern about it.

JsonSchema::Schema::Link comes to be a subclass of JsonSchema::Schema. Basically, LDO is not a schema (no refs to "#" in https://github.com/json-schema-org/json-schema-spec/blob/master/archive/draft-04/hyper-schema.json#L116-L156). and subclass of JsonSchema::Schema seems weired to me.

Becoming a subclass, for example, JsonSchema::Schema::Link#additional_properties becomes to return true which is a default value of JsonSchema. It is not per spec, and It may cause conflict when we made our own expansion to LDO.

I think the functionalities about attr_copyable should be as module, and both JsonSchema::Schema and JsonSchema::Schema::Link should include it. There are no need of inheritance.

brandur commented 7 years ago

JsonSchema::Schema::Link comes to be a subclass of JsonSchema::Schema. Basically, LDO is not a schema (no refs to "#" in https://github.com/json-schema-org/json-schema-spec/blob/master/archive/draft-04/hyper-schema.json#L116-L156). and subclass of JsonSchema::Schema seems weired to me.

Yeah, I agree that it's not ideal as well. The only reason I did it is that I got myself a little stuck in the way that reference expansions are handled. They work by copying a target "into" the current reference (also a Schema object, probably incorrectly), and so the source reference must have all the properties of the target in order for the expansion to work.

I may be able to work around that by keeping references as separately objects and have them "morph" into either a schema and link as they're materialized by dynamically inheriting all their properties. I'll think about it some more.

I think the functionalities about attr_copyable should be as module, and both JsonSchema::Schema and JsonSchema::Schema::Link should include it. There are no need of inheritance.

Well, one nice thing that fell out of the refactor is that attr_copyable and friends did move into a separately module that can be mixed in (JsonSchema::Schema::Attributes). As described above though, the inheritance scheme is unfortunately still necessary for the time being.

brandur commented 7 years ago

I spent a few hours on this refactor, but have put it aside for the time being because the complexity was starting to spin out of control. I may still try to do it, but I'm not sure about the timeline.