Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Implement recursive model validation #415

Closed
isadovskiy opened this issue Aug 12, 2016 · 15 comments
Closed

Implement recursive model validation #415

isadovskiy opened this issue Aug 12, 2016 · 15 comments

Comments

@isadovskiy
Copy link

I have an obhect represented the tee of pages. It looks like this:

{
    "title1": "Root page",
    "pages": [
        {
            "title1": "Sub-page 1"
        }
    ]
}

I'm trying to validate it with the following schema using require('swagger-tools').specs.v2.validateModel() method:

{
    "swagger": "2.0",
    "info": {
      "version": "",
      "title": "",
      "license": {
        "name": ""
      }
    },
    "paths": {
    },
    "definitions": {
        "Page": {
            "type": "object",
            "properties": {
                "title": {
                    "type": "string"
                },
                "pages": {
                    "type": "array",
                    "items": {
                        "$ref": "#/definitions/Page"
                    }
                }
            },
            "required": [
                "title"
            ]
        }
    }
}

It validates just root node for me and does not dive in depth (display error just about missed required "title" field at the top level; missing required "title" field for sub-item is not reported). Is there any chance to fix this?

@whitlockjc
Copy link
Member

I'm not sure what version of swagger-tools you're using but this should be working. Here is the code that tells the validator to not stop after the first error: https://github.com/apigee-127/swagger-tools/blob/master/lib/helpers.js#L61

@isadovskiy
Copy link
Author

isadovskiy commented Aug 12, 2016

i'm using "0.10.1"(looks like the latest one). And of course I tested it in different ways with valid and invalid property names both on first and second level. If you fix title field on first level, it still never report you error on the second level. Basicaly, library reports this JSON is valid (while it's not true).

{
    "title": "Root page",
    "pages": [
        {
            "title1": "Sub-page 1"
        }
    ]
}

@whitlockjc
Copy link
Member

Oh, I know what is up. Your model is circular and so the circular reference gets replaced with {} and anything validates against that. json-refs is currently being updated to make circular references configurable in their handling but as for right now, this is working as designed because you have a circular reference.

@isadovskiy
Copy link
Author

But swagger specs do not deny circular references. So is it something, which will be improved and allowed in the near future?

@isadovskiy
Copy link
Author

In additional, schema above with the circular reference was successfully validated. So I want to understand where this restiction come from...

@whitlockjc
Copy link
Member

Circular references are allowed, which is why your Swagger document does not get flagged as invalid. But when you want to validate a value against a schema defined in your Swagger document, circular references break any JSON Schema validator out there. swagger-tools uses z-schema and it will fail if you have a schema that is circular. In fact, we use to use jjv and it had the same limitation.

So while Swagger and swagger-tools will work with circular references, validation against models that are circular are currently impacted by json-refs just turning the circular reference into a {}. Ideally, json-refs would be configurable to allow you to dictate how to handle circulars. This would allow you to dictate a depth of how many levels to circularly recurse but at some point, you have to break the circular chain to avoid breaking JSON Schema validators. The json-refs enhancement is tracked here: whitlockjc/json-refs#74

@isadovskiy
Copy link
Author

I'm wondering if json-ref could be replaced with something more tolerant to circular references? Like https://github.com/BigstickCarpet/json-schema-ref-parser

@whitlockjc
Copy link
Member

No because the problem isn't tolerance, json-refs can gladly give you circular references. The issue is that the JSON Schema validator libraries fail using them so they have to be scrubbed somewhere and right now, json-refs is the place that happens...it's just too aggressive.

@isadovskiy
Copy link
Author

Sadly to hear this :( Thanks for the explanations!

@whitlockjc
Copy link
Member

whitlockjc commented Aug 12, 2016

Once json-refs is updated to better handle this, swagger-tools and sway will be updated. While it isn't to your liking, I hope the reasoning behind the issue makes sense. To reiterate, this isn't an issue with json-refs or Swagger or swagger-tools supporting circular references. The issue is that JSON Schema validation libraries do not handle circular references and so somewhere, they have to be scrubbed. The real fix is to make this configurable in json-refs, something that is already planned.

As for json-schema-ref-parser, it just perpetuates the same problem json-refs shields you from. If you look at the docs, it clearly tells you that if you plan on serializing the dereferenced document with circular references, you have to remove the circulars. (The reason this matters is because JSON Schema validators typically walk documents recursively, like JSON serialization does, and so the same problem arises there as well.) I do think that json-schema-ref-parser has some lightweight configurable way to do this but I'm not switching libraries just for that especially when json-refs already is aware and working on the issue.

@isadovskiy
Copy link
Author

Yeah, completely makes sense. So be waiting for updates.

@whitlockjc
Copy link
Member

I plan on working on json-refs over the weekend. Hopefully I can get this sorted and get swagger-tools/sway updated as a result.

@isadovskiy
Copy link
Author

would be amazing!

@dave-irvine
Copy link

@whitlockjc Has there been any progress on this?

@whitlockjc
Copy link
Member

Resolver is done, rewriting tests. Once json-refs is released, swagger-tools and sway will be updated to use it, and to have bugs addressed.

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

No branches or pull requests

3 participants