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

Deep merge dereferenced keys #163

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

Conversation

pguelpa
Copy link

@pguelpa pguelpa commented Sep 15, 2014

This is a quick and dirty solution to a problem I had when overriding values in our schema. The problem it's trying to solve was this:

Given the links snippet:

      "targetSchema": {
        "properties": {
          "operation": {
            "$ref": "/schemata/operation",
            "properties": {
              "id": {
                "$ref": "/schemata/operation#/definitions/id"
              }
            }
          }
        }
      }

the resulting documentation would not include both the dereferenced object's properties and the override properties. Instead the override properties would replace those from the dereferenced object.

This is a somewhat naive approach and I'd be open to feedback on how to improve it.

ref['$ref'] && ref['$ref'].split('/').last == 'id'
end
ref = idRef || value['anyOf'].first
ref = value['anyOf'].detect {|ref| ref.has_key?('$ref') && ref['$ref'].split('/').last == 'id'} || value['anyOf'].first
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't see how this was affected by my change, but there was a test that was failing for me locally. I also wasn't sure how this was working before, unless somewhere we're creating hash objects that have an empty string as the default value.

@geemus
Copy link
Member

geemus commented Sep 16, 2014

@pguelpa I don't think deep-merge is expected, I think instead it might just be a matter of changing the schema a bit. Given your example, maybe something more like:

      "targetSchema": {
        "properties": {
          "operation": {
            "properties": {
              "$ref": "/schemata/operation#/properties",
              "id": {
                "$ref": "/schemata/operation#/definitions/id"
              }
            }
          }
        }
      }

I think something like that might be in order anyway, I tried to read back through specs and stuff and they are difficult to parse, but don't really indicate anything about more complex or deeper stuff here (I think they are fairly naive and just do overwrites unless I'm totally mistaken)

@brandur I know you have also spent a lot of time looking at this stuff, could you chime in about your understanding of this behavior? Thanks!

@pguelpa
Copy link
Author

pguelpa commented Sep 16, 2014

@geemus - That was what I tried first, but I get a Expected schema; value was: "#/definitions/operation/properties". failure when I try to run prmd verify. I could be doing something else wrong though ... any ideas?

/Users/pguelpa/.rvm/gems/ruby-2.1.2/gems/json_schema-0.1.9/lib/json_schema/parser.rb:38:in `parse!': #/definitions/transaction/links/1/targetSchema/properties/transaction/properties/operations/items: Expected schema; value was: "#/definitions/operation/properties". (RuntimeError)
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/gems/json_schema-0.1.9/lib/json_schema.rb:19:in `parse!'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/gems/prmd-0.6.2/lib/prmd/commands/verify.rb:22:in `verify'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/gems/prmd-0.6.2/bin/prmd:111:in `<top (required)>'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/bin/prmd:23:in `load'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/bin/prmd:23:in `<main>'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/bin/ruby_executable_hooks:15:in `eval'
    from /Users/pguelpa/.rvm/gems/ruby-2.1.2/bin/ruby_executable_hooks:15:in `<main>'

@geemus
Copy link
Member

geemus commented Sep 18, 2014

Hmm. Not off the top of my head. Looks like it might be coming from json_schema instead of prmd itself.

@pguelpa - could you post the schema you are getting the error from? Just want to make sure we are working from the same source before I dig too deep and/or ask for further help. Hopefully from there we can get to the bottom of this. Thanks!

@pguelpa
Copy link
Author

pguelpa commented Sep 22, 2014

@geemus - Sorry for the delay on my response, this is the schema I'm working with is pretty large, but here is a stripped down version which shows what I'm trying to achieve:

https://gist.github.com/pguelpa/53362b0a9e49792ac4f9

Basically because of how our validator currently works (based off the top level object rather than a schema in the link) I'm trying to define the targetSchema to have the id property on the response that won't be validated against on the request. I know that's not standard, but I figured the property overrides would work either way.

@geemus
Copy link
Member

geemus commented Sep 23, 2014

@pguelpa thanks for the details. I would agree that sounds a bit non-standard, but that I think the overrides should still do stuff as you point out.

@brandur could you take a look and see if you have any insights? It isn't jumping out at me.

@bdmac
Copy link
Contributor

bdmac commented Oct 21, 2015

@geemus I'm running into this a bit myself. The problem with your suggestion above is it seems to assume that the schema at "$ref": "/schemata/operation" does not really define anything besides properties. That is not always the case. If there is more defined there then a deep merge of some sort (or supporting allOf) is seemingly required otherwise you have to resort to something like this:

      "targetSchema": {
        "properties": {
          "operation": {
            "$ref": "/schemata/operation",
            "properties": {
              "$ref": "/schemata/operation#/properties",
              "id": {
                "$ref": "/schemata/operation#/definitions/id"
              }
            }
          }
        }
      }

Note the double $ref to operation, one to include the base schema and one to override the id property.

This does not actually seem to work anyways with PRMD because trying to use "$ref" within the context of properties leads to errors on verify such as

docs/schema/api.json: #/definitions/business/definitions/resource/properties/$ref: failed schema #/properties/properties/additionalProperties: "#/definitions/common/definitions/resource/properties" is not an object

In my case we are using json-api and what I want to do is override ONLY the attributes property of the base resource to define my resource's specific properties.

I was not able to use an URL $ref with PRMD so I have a common.js that largely reproduces the json-api schema file:

...
    "resource": {
      "description": "\"Resource objects\" appear in a JSON API document to represent resources.",
      "type": "object",
      "required": [
        "type",
        "id"
      ],
      "properties": {
        "type": {
          "$ref": "/schemata/common#/definitions/type"
        },
        "id": {
          "$ref": "/schemata/common#/definitions/id"
        },
        "attributes": {
          "$ref": "/schemata/common#/definitions/attributes"
        },
        "relationships": {
          "$ref": "/schemata/common#/definitions/relationships"
        }
      },
      "additionalProperties": false
    },
    "attributes": {
      "description": "Members of the attributes object (\"attributes\") represent information about the resource object in which it's defined.",
      "type": "object",
      "patternProperties": {
        "^(?!relationships$|links$)\\w[-\\w_]*$": {
          "description": "Attributes may contain any valid JSON value."
        }
      },
      "additionalProperties": false
    },
...

That definition of attributes works well broadly for the json-api schema but I want my specific resource (e.g. a Business) to basically redefine resource for itself at /schemata/business#/definitions/resource.

The way things stand today I basically have to redefine the entire common resource in the specific resource file just to override resource/properties/attributes.

@geemus
Copy link
Member

geemus commented Dec 1, 2015

@bdmac good point, thanks for elaborating. Do you know if the fix as proposed here would fix the issue for you? Seems like we would need to at least rebase, but would be good to know if this was on the right track.

@nomasprime
Copy link
Contributor

Hi @bdmac, I'm facing the same problem. Did you get any further with this?

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.

4 participants