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

Add support for referencing subschemas by id. #371

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dfinlay

dfinlay commented Nov 17, 2017

Add support for $ref elements using a subschema's declared id value. This is supported by a an additional store in the RefResolver class that stores all id declared subschemas.

Add support for `$ref` elements using a subschema's declared `id` val…
…ue. This is supported by a an additional store in the `RefResolver` class that stores all `id` declared subschemas.
@Julian

This comment has been minimized.

Show comment
Hide comment
@Julian

Julian Nov 18, 2017

Owner

Hey @dfinlay thanks so much!

Would you mind elaborating a bit on what this is doing? Is this functionality you think is in the spec that's missing from jsonschema, or functionality you wish existed, or a convenience helper for functionality that exists already?

Owner

Julian commented Nov 18, 2017

Hey @dfinlay thanks so much!

Would you mind elaborating a bit on what this is doing? Is this functionality you think is in the spec that's missing from jsonschema, or functionality you wish existed, or a convenience helper for functionality that exists already?

@dfinlay

This comment has been minimized.

Show comment
Hide comment
@dfinlay

dfinlay Nov 19, 2017

Sorry, I should have added some more commentary. This pull request adds support accessing subschemas by their id/$id keyword. This specifically addresses #341 and #313 as well.

A note on the implementation, I added the subschema_store to RefResolver, because I didn't want to be too intrusive in the changes. I think it would be better to make RefResolver.store a dict of dicts, and use # to identify the root schema. Thoughts?

I'd like to add some more tests, but I was a bit lost in the the test structure. Guidance on where / how to add them would be appreciated.

dfinlay commented Nov 19, 2017

Sorry, I should have added some more commentary. This pull request adds support accessing subschemas by their id/$id keyword. This specifically addresses #341 and #313 as well.

A note on the implementation, I added the subschema_store to RefResolver, because I didn't want to be too intrusive in the changes. I think it would be better to make RefResolver.store a dict of dicts, and use # to identify the root schema. Thoughts?

I'd like to add some more tests, but I was a bit lost in the the test structure. Guidance on where / how to add them would be appreciated.

@Julian

This comment has been minimized.

Show comment
Hide comment
@Julian

Julian Nov 19, 2017

Owner

Aha, awesome!

I have to definitely read this carefully -- the one general thing I'd say though is that I definitely want to restrict public changes to the $ref implementation (to RefResolver), because I have strong suspicion that it needs replacing entirely ( #346 ). So definitely if this can be done in a way that doesn't even add to the public interface, we should do that I think (so, e.g., is there actually a need to add the additional subschema_store parameter, or can it just be created by not overridable externally?)

I think it would be better to make RefResolver.store a dict of dicts, and use # to identify the root schema.

This sounds likely to be backwards incompatible with what the current thing asks for. But I probably have to read the changes here carefully to understand exactly.

I'd like to add some more tests, but I was a bit lost in the the test structure.

Yeah, sorry bout that :/. The tests for this seem very likely like they'd go in test_validators, alongside the 2 I see you modified, but I can follow up with a specific recommendation after I read the code.

Owner

Julian commented Nov 19, 2017

Aha, awesome!

I have to definitely read this carefully -- the one general thing I'd say though is that I definitely want to restrict public changes to the $ref implementation (to RefResolver), because I have strong suspicion that it needs replacing entirely ( #346 ). So definitely if this can be done in a way that doesn't even add to the public interface, we should do that I think (so, e.g., is there actually a need to add the additional subschema_store parameter, or can it just be created by not overridable externally?)

I think it would be better to make RefResolver.store a dict of dicts, and use # to identify the root schema.

This sounds likely to be backwards incompatible with what the current thing asks for. But I probably have to read the changes here carefully to understand exactly.

I'd like to add some more tests, but I was a bit lost in the the test structure.

Yeah, sorry bout that :/. The tests for this seem very likely like they'd go in test_validators, alongside the 2 I see you modified, but I can follow up with a specific recommendation after I read the code.

@Julian

This comment has been minimized.

Show comment
Hide comment
@Julian

Julian Dec 10, 2017

Owner

Hey, haven't forgotten this, just still having some trouble figuring out how much of this is a bug fix and how much is new :/.

It's likely my own fuzzy knowledge of $ref that I never properly sat down to think about.

Still definitely hoping to find some time to think about this PR though. Sorry for the wait.

Owner

Julian commented Dec 10, 2017

Hey, haven't forgotten this, just still having some trouble figuring out how much of this is a bug fix and how much is new :/.

It's likely my own fuzzy knowledge of $ref that I never properly sat down to think about.

Still definitely hoping to find some time to think about this PR though. Sorry for the wait.

@elipapa

This comment has been minimized.

Show comment
Hide comment
@elipapa

elipapa Feb 15, 2018

any news on this?

elipapa commented Feb 15, 2018

any news on this?

@dfinlay

This comment has been minimized.

Show comment
Hide comment
@dfinlay

dfinlay Feb 17, 2018

I should probably have been more proactive with this. I needed to add more tests! Also, looking back at this without rose-tinted glasses, there is as a problem with the implementation. You cannot pass an object for validation against a defined subschema. Instead, you can only validate subschema elements if they are children of root schema instance. This should be resolved before this change is merged.

Ideally, one should be able to pass a string id to the validator with an instance and have it validated against the schema w/ that ID, regardless of whether it is a subschema and root.

Here is an example of how you have to validate instances against subschemas now (Ignore the versions here, just an example).

schema = {
  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "awesome.json",
  "title": "Awesome",
  "type": "object",
  "properties": {
    "name": {
      "title": "Awesome Name",
      "type": "string",
      "maxLength": 256,
      "default": ""
    },
    "sauce": {
      "$ref": "#awesome-sauce"
    }
  },
  "required": [
    "name",
    "sauce"
  ],
  "definitions": {
    "awesome-sauce": {
      "$id": "#awesome-sauce",
      "title": "Awesome Sauce",
      "description": "Sauce for your food that is quite awesome.",
      "enum": [
        "ranch",
        "french-onion",
        "au-jus",
        "misc",
        "recycled"
      ]
    }
  }
}

validator = jsonschema.Draft4Validator(schema)

data = dict(name="Dave's Homemade Ranch", sauce="ranch")

sauce = dict(sauce="ranch")

validator.validate(data)
> True

validator.validate(sauce)
> False

validator.validate(sauce, schema['definitions']['awesome-sauce'])
> True

dfinlay commented Feb 17, 2018

I should probably have been more proactive with this. I needed to add more tests! Also, looking back at this without rose-tinted glasses, there is as a problem with the implementation. You cannot pass an object for validation against a defined subschema. Instead, you can only validate subschema elements if they are children of root schema instance. This should be resolved before this change is merged.

Ideally, one should be able to pass a string id to the validator with an instance and have it validated against the schema w/ that ID, regardless of whether it is a subschema and root.

Here is an example of how you have to validate instances against subschemas now (Ignore the versions here, just an example).

schema = {
  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "awesome.json",
  "title": "Awesome",
  "type": "object",
  "properties": {
    "name": {
      "title": "Awesome Name",
      "type": "string",
      "maxLength": 256,
      "default": ""
    },
    "sauce": {
      "$ref": "#awesome-sauce"
    }
  },
  "required": [
    "name",
    "sauce"
  ],
  "definitions": {
    "awesome-sauce": {
      "$id": "#awesome-sauce",
      "title": "Awesome Sauce",
      "description": "Sauce for your food that is quite awesome.",
      "enum": [
        "ranch",
        "french-onion",
        "au-jus",
        "misc",
        "recycled"
      ]
    }
  }
}

validator = jsonschema.Draft4Validator(schema)

data = dict(name="Dave's Homemade Ranch", sauce="ranch")

sauce = dict(sauce="ranch")

validator.validate(data)
> True

validator.validate(sauce)
> False

validator.validate(sauce, schema['definitions']['awesome-sauce'])
> True
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment