OAI / OpenAPI-Specification

The OpenAPI Specification Repository
https://openapis.org
Apache License 2.0
28.87k stars 9.07k forks source link

Extra JSON Reference properties ignored - reduces reusability of references #556

Closed JimJafar closed 4 years ago

JimJafar commented 8 years ago

Hi everyone.

I feel that the ignoring of extra properties when using JSON References goes against the goal of reusability.

Take this example from a fictional Swagger contract:

{
    addresses: [
        "homeAddress": {
            "description": "The premises at which electricity is provided.",
            "$ref": "#/definitions/Address"
        },
        "invoiceAddress": {
            "description": "The billing address - must be where the customer's credit card is registered.",
            "$ref": "#/definitions/Address"
        }
    ]
}

The "description" properties help to distinguish between the two uses of the "Address" Reference. Of course you could argue that better naming of the "homeAddress" and "invoiceAddress" properties could achieve this, but we are not always in control of property names in our data models and sometimes you need a more verbose description.

The Swagger editor (http://editor.swagger.io/#/) is an example of an application that could be improved by removing this limitation.

Is this something that has been raised previously? Please see here for a discussion on the Swagger Google Group: https://groups.google.com/forum/#!topic/swagger-swaggersocket/ewgimdO2cOI

webron commented 8 years ago

Currently, we use JSON References as-is. While there's a huge advantage to that tooling-wise, it does hinder reusability in some cases, and the issue has been raised (unofficially) a few times. Overriding the description is one case, but there are also cases of wanting to override whether fields are required and so on. Not sure if we should change it, given the cost, but we should consider it.

qcho commented 8 years ago

I think this is a very good Proposal. In my example I want to set:

      parameters:
        - $ref: '#/parameters/fields'
          default: 'local_default'
whitlockjc commented 8 years ago

I would be willing to update json-refs to have a flag that allowed you to handle this situation. Based on OpenAPI feature requests, I can see a few different things we'd need:

The JSON Reference spec clearly says these should be ignored. But...there's nothing stopping json-refs from allowing extra functionality on top in an opt-in manner.

whitlockjc commented 8 years ago

Realize, this has nothing specific to do with OpenAPI other than a number of the Node.js tools around OpenAPI use json-refs. A decision on either side does not have to reflect or impact the ability/decision of the other. I hope that is clear.

fehguy commented 8 years ago

I would love this feature, but I strongly advise we don't do it. If we did, tooling support would be inconsistent because the spec says it's a no-no.

There is a construct in JSON schema draft 5 but I don't think we're considering updating to that (@webron?)

whitlockjc commented 8 years ago

Good point. I was just pointing out that if the decision was to do this, I have no objection for my tooling. As for JSON Schema Draft 5, unless it alters the JSON References specification I don't see us being able to support this if we do things the JSON References way. I wonder if we should reach out to the JSON References authors?

drewish commented 8 years ago

I'd also love to be able to override items from a $ref. I'd actually been using them before but now that the swagger editor is flagging them as warnings I'd been reconsidering it.

whitlockjc commented 8 years ago

The warnings are accurate so don't attempt to merge/override properties in a referenced object as it won't work. I would love to see this addressed as well.

webron commented 8 years ago

Parent meta issue: #579

webron commented 8 years ago

I haven't checked draft 5, so I don't know if it has effects on JSON References as well (possibly with a new draft to that). I don't think we'll be moving to draft 5 as it's not official (yeah, an official draft).

Breaking away from specs is a challenge. Unlike some expansions to JSON Schema which are actually allowed by JSON Schema itself, we're suggesting here something that goes against the spec of JSON References.

That said, within the spec, the inability to add/override JSON References makes them unusable in some cases and forces people to not DRY or make incomplete documentation.

The real challenge here is not to say we allow it, but try to think of the edge cases that could help users abuse this feature in a way, and causing more problems for the tooling. I'd like to see this change happening, but we need to be careful here.

fehguy commented 8 years ago

There is a merge construct which addresses this issue. But I think moving to draft 5 would be a big mistake right now.

jmatcuk commented 8 years ago

I have a team trying to use the swagger editor hosted on swaggers site to edit our API's but with the recent enhancement to the editor to use this update, our descriptions were showing as warnings. We have a very similar issue as @JimSangwine which I can post our actual example but for now we've just cloned an older tag of the swagger editor and are running it locally.

So the reason that I posted is to support this change but to ask for clarification, is @fehguy suggesting that we do not support this because the draft version of the JSON Schema does not support it and the swagger editor was upgraded to use the new draft version?

If i'm following correctly, it's a draft and a reason why this argument should be made to change the specification.

IvanGoncharov commented 8 years ago

Actually it supported both in Swagger and Draft-4, you just need to use allOf keyword. So instead of:

description: 'The premises at which electricity is provided.'
$ref": '#/definitions/Address'

You should write:

description: 'The premises at which electricity is provided.'
allOf:
  - $ref: '#/definitions/Address'

It standard usage of allOf nothing fancy here, and I think it fully solve this problem.

tadhgpearson commented 8 years ago

Thanks @JimSangwine - I also face this issue, indeed I opened this as a question on the JSON Schema Group https://groups.google.com/forum/#!topic/json-schema/YS1dbWXk_4s

@IvanGoncharov - I really like this suggestion. It seems syntactically valid, but functionally unclear. For example - if both elements happen to have a description, which one wins? Furthermore, fields which include this type of reference are dropped from the output of the code generator, so for me, it's not an acceptable 'interim fix' :(

ralfhandl commented 8 years ago

Found out accidentally today that Swagger UI supports and visualizes description within $ref object as in the initial example, whereas Swagger Editor issues a warning

webron commented 8 years ago

Tackling PR: #741

ghost commented 8 years ago

@IvanGoncharov Regarding the suggestion to use allOf for the purpose, I am not sure if this is the intended use of allOf as has been suggested. allOf intended use does not seem to suggest that it can be used for overriding property values or providing additional "description" property.

tadhgpearson commented 7 years ago

Maybe this can be best resolved by the proposed new $use extension of JSON Schema - see https://github.com/json-schema-org/json-schema-spec/issues/98

advance512 commented 7 years ago

allOf is pretty much unsupported in most tools. e.g. https://github.com/swagger-api/swagger-editor/issues/476

nickreevesuk commented 7 years ago

The overarching goal of OpenAPI is to meet the needs around meta data definition of APIs. If a technical choice (e.g. to model references as JSON References) conflicts with those needs it is not the original need that it is at fault but the technical choice, because it is not fit for purpose.

There are many genuine uses for wanting to add meta data at the point of reference. If JSON References cannot meet that need, then Open API should select a different syntactic construct as an alternative to meet that need. This might be the new $use extension of JSON schema as above, or an alternative specific to Open API. For backwards compatibility this would probably be an alternative option rather than a replacement.

Failure to address reasonable community needs will simply cause the community to move elsewhere to projects that can meet their needs.

darrelmiller commented 7 years ago

@nickreevesuk If you take the time to read the history of this issue, you will see that we all would like this feature to exist. $ref works the way it does in OpenAPI because that's the way it is defined in JSON Schema. Despite this shortcoming of the $ref feature, community consensus is that people want OpenAPI to use JSON Schema more consistently with its spec, not less.

There have been efforts to address this issue in JSON-Schema in the past, but those efforts stalled. Now that the folks working on that spec have got some momentum again, it is possible that we may see this issue resolved in the near future.

As I noticed you are new here on GitHub, perhaps you are not aware this is a volunteer run project and the best way to make things happen is to contribute. Those who are involved tend not to react positively to threats suggesting that if you don't implement this feature, all your users are going to leave.

Now, I am going to go back to trying to analyze the differences between the draft-4 JSON Schema spec and the latest draft so that we can adopt the new JSON-Schema spec in OpenAPI V3 without introducing breaking changes for our users.

nickreevesuk commented 7 years ago

@darrelmiller no threat intended, that's just simply the way mindshare of projects works. I was just trying to move the discussion back to the requirements.

emil-prager commented 7 years ago

@darrelmiller Thank you for your patience and for the intention to (re)check whether the last draft of the JSON spec might add more flexibility in this regard! However, regarding your remark that:

the best way to make things happen is to contribute

I'd say that actually there is at least such an initiative from the community --- see this comment, for example.

handrews commented 7 years ago

@darrelmiller do note that Draft 06 is almost ready, with most of the remaining PRs as minor bugfix sorts of things: https://github.com/json-schema-org/json-schema-spec/milestone/2

I strongly recommend looking at this rather than Draft 05 as there are significant improvements.

webron commented 7 years ago

We're not going to include changes from draft 05 or 06 at this stage. The reason we're considering moving to 05 is that it doesn't change the functionality of the things that are already supported by the OAS. It looks like 06 adds functionality, and it's too late for us to evaluate whether we can support it or not. To add to that, since tooling takes time to catch up, I don't think it's wise for us to play the catch-up game in this case.

There is one major issue with draft 05 that has to be resolved before we can switch to it, and that's the drop of the integer type. If that's added back to draft 05, we're likely to move to it. If not, for now, we'll stick with draft 04 (even if expired).

handrews commented 7 years ago

@webron it was apparently an unintentional deletion on the draft editor's part and has been added back in Draft 06. You can't actually change a draft once published so the actual draft 05 document won't reflect this, but it's safe to treat draft 05 as if integer was still supported. There is actually one part in draft 05 that mentions 7 types as if integer was still present, so it wasn't even entirely removed.

The only other thing I'd note about going from draft 05 to draft 06 is that draft 05 added a "uriref" format which became "uri-ref" in draft 06 (because "date-time" and making all compound formats use hyphens).

Otherwise, draft 06 validation is compatible with draft 05 so you'll still be on a smooth path. The big changes (in both) were in hyper-schema which does not impact you.

webron commented 7 years ago

Yeah, I think it was @Relequestual who mentioned that it was an unintentional drop. However, it don't know if it's safe to treat it that way. If you can't change it, then I would expect you to release draft 06 with the fix and push the rest of the changes as draft 07. I realize this may not work with your plan.

To avoid confusion, it might be safer for us to stay with draft 04 for now (assuming you can't make the suggested change) and look back on updating the support in the next version of our spec.

nickreevesuk commented 7 years ago

Having looked at the root cause of this I can suggest one other possible approach in addition to those I mentioned above.

The underlying reason why JSON schema specification rejects siblings of $ref seems to be because of the concern about these potentially overriding validation criteria in different references to the same underlying definition in conflicting ways. It is safe for purely JSON schema validation purposes to ignore siblings of $ref which don't affect validation as others have already observed.

This opens the possibility of defining the OpenAPI as a superset of the JSON schema where all additional tags must begin with a whitelisted set of prefixes which get stripped out to reduce the contents to a standard JSON schema document. This allows the OpenAPI file to be used both as is by tools which understand the additions or will ignore them. For tools with a strict interpretation of JSON schema we require an additional tool in the toolchain which will strip out this well defined set of extensions to to produce a new set of JSON schema specification compliant input files.

Although this does require an extra tool in the chain it does break the log jam by decoupling the two specifications, leaving OpenAPI much more free to evolve as it needs. This might help with a much wider set of issues.

Following the principle that things should be closed for modification but open for extension it would have been more helpful if the JSON schema specification forbad only tags it understood as siblings of $ref, but that is beyond the scope of this group.

handrews commented 7 years ago

@nickreevesuk json-schema-org/json-schema-spec#98 , the "$use" proposal mentioned earlier which is under consideration for Draft 07 in large part because of support from folks from OAI. Options for implementing it would include either a whitelist (all annotation keywords are on the whitelist and can appear in the "with" clause) or blacklist approach (all validation keywords are on the blacklist and cannot appear in the "with" clause). Either approach could be extensible,a nd both could be used.

It is easy enough for OAI to implement "$use" as an extension which could then fold into JSON Schema proper, just keep us posted on any implementation choices you make.

timburks commented 7 years ago

I ran into this issue recently and add the following as a practical note:

Scanning the descriptions in the apis.guru OpenAPI directory, I found two descriptions that put "description" fields alongside "$ref".

I expect that this will be a fairly common mistake.

We might guard against this by enforcing schema validation early. For example, public archives like apis.guru might only publish API models that pass validation.

But hand-written source files are what they are.

For better or worse, I was able to accept these descriptions in my schema-driven reader by generating my reader from a modified schema that adds a "description" property to the "jsonReference" definition.

    "jsonReference": {
      "type": "object",
      "required": [
        "$ref"
      ],
      "additionalProperties": false,
      "properties": {
        "$ref": {
          "type": "string"
        },
        "description": {
          "type": "string"
        }
      }
    }
webron commented 7 years ago

Pointing to specific lines in those would help, because I can't find a problem. That said, there's nothing to catch in the validation. It's not wrong to have additional properties, they're just ignored. There's nothing invalid about that.

MikeRalphson commented 7 years ago

For example, public archives like apis.guru might only publish API models that pass validation.

We do. :) What @webron said.

ePaul commented 7 years ago

I think there is a real use case for having description near references – a description for a type is not suitable as description for all properties of this type. That is why people put a description there – even knowing than any tools will ignore it, it is useful for human readers.

Relequestual commented 7 years ago

We had a similar discussion over at JSON Schema as @ePaul explains. The description is ignored when with a $ref, but actually it's still useful for human readers of the raw documents.

advance512 commented 7 years ago

Wouldn't it be nice having this valuable information as part of visual OpenAPI clients, like Swagger UI, and not just for a human incidentally reading the raw JSON schema?

Relequestual commented 7 years ago

As stated earlier, we are looking into adding a new key word to JSON Schema core which will allow this sort of thing. JSON reference was kind of rolled into JSON Schema.

Regardless I think this is probably more of an upstream issue from Swagger / OAI, as they are not defining the behaviour.

tadhgpearson commented 7 years ago

We had a similar discussion over at JSON Schema as @ePaul explains. The description is ignored when with a $ref, but actually it's still useful for human readers of the raw documents.

Absolutely - see the initial discussion of this subject on the JSON Schema group The proposal to adapt to this in a generic way is JSON Schema #98, but I would tend to agree that the description seems to be by-far the most common use case.

If apis.guru provides strong support to indicate this issue applies almost exclusively to the description, and it the solution could be much simpler if only support for the description were required, I'd encourage you to go for it!

MikeRalphson commented 7 years ago

If apis.guru provides strong support to indicate this issue applies almost exclusively to the description...

I'm not sure if you meant APIs.guru the collection or APIs.guru the maintainer (hi!), but anyway here is some analysis.

Of 5,182 instances of $ref having other properties: 4,356 are description only 283 are x-ms-client-flatten 138 are title only 131 are description and readOnly 107 are description and title 102 are description and x-ms-client-flatten 20 are properties and required 16 are example 7 are type 6 are description and type 6 are readOnly 5 are description and x-ms-client-name 3 are description and externalDocs 2 are required

The results may be skewed somewhat by the Google specs being particularly bad offenders of $ref + description.

If anyone's interested I could run the same analysis over our entire test corpus of nearly 50,000 definitions.

handrews commented 7 years ago

If anyone's interested I could run the same analysis over our entire test corpus of nearly 50,000 definitions.

@MikeRalphson I would love to see this information, and if you could update json-schema-org/json-schema-spec#98 with it that would be great. The main reason that proposal hasn't gone in is that a few people are refusing to support it on the grounds that they want to be able to use it with all validation keywords. My argument has been that annotation keywords (description, title, default, and examples from the validation spec, readOnly from the hyper-schema spec) are their own distinct use case that has substantially broader support for overrides than validation. Your data so far would seem to support that.

I'm curious about the type usage, though- any patterns there?

Relequestual commented 7 years ago

For refernece, myself and @handrews are maintainers of JSON Schema.

MikeRalphson commented 7 years ago

json-schema-org/json-schema-spec#98 updated.

I'm curious about the type usage, though- any patterns there?

They seem mainly to be type: object where the type property hasn't been defined in the referenced definition. I'm guessing they may be remnants of cut-and-pasting where a definition has been extracted for re-use? The larger number across the 50K definitions might not bear that out though.

slhenty commented 7 years ago

Having read all these comments, and being faced with a real-world desire to share definitions but make the description meaningful at the point of reference, I find myself aligning with @nickreevesuk's observation that treating OpenAPI as a superset of JSONSchema solves the immediate needs of the OpenAPI community without limiting existing tooling, nor future adherence to JSONSchema should it finally acquire a recognized method of mixing in / overriding shared references.

I appreciate the respective OpenAPI / JSONSchema teams wanting to 'get this right', but providing a stepping stone would help the client community now. At the end of the day, I just want to use Swagger as a single source of truth for API definition and documentation - as it stands, the current $ref is failing that charge. The community (myself included) appears to gravitate naturally to adding a description field to the referencing object, whether as override or mixin... I say treat that as the signal it is.

fehguy commented 7 years ago

I just think it's a tough one. If we go against JSON schema the expect that all JSON schema tooling will not work, and therefore we'd be better off building our own syntax, since the benefit of tooling reuse is zero.

I personally think the $ref stand-alone constraint in JSON schema is garbage, at least for our purposes. Makes for ugly definitions and causes too many tears.

handrews commented 7 years ago

Here are all of the proposed solutions to date, including just allowing "definitions" and "title" alongside "$ref": https://github.com/json-schema-org/json-schema-spec/labels/additionalProperties (not all of these are strictly about additionalProperties, that's just the biggest complaint so I used it for the label)

Most people involve recognize the problem, the deadlock is on choosing the solution. I am pushing hard to resolve this in the next draft, as I view it as the "elephant in the room" of JSON Schema and one of the biggest barriers to further adoption.

handrews commented 7 years ago

Anyway, please feel free to weigh in on the various proposals. Community support matters for these things.

Relequestual commented 7 years ago

@handrews I would support putting this issue as 1st priority for draft-7. It's a clear issue, and something people want. I confess I have a LOT of catching up and other JSON Schema related work to do though... and I'll be at less than 100% strength for a while! ;)

But seirously, if this isn't on the draft-7 milestone, it should be. I'll avoid further thoughts as this is OAI issue and not in JScheam repo.

pierslawson commented 7 years ago

I'm afraid to say I'm new to OAI having found my way here due to a difficulty I'm having with swagger. However, could I also suggest that you add "xml" to the list of items possibly supported alongside a $ref. The current approach means that XML overrides can only be applied to basic properties. As far as I can see, if a property is defined using a $ref, it is impossible for me to override its XML name. e.g. the example below would rename id to Id but not content to Content in the XML world.

"Document": {
  "type": "object",
  "properties": {
    "id": {
      "type": "string",
      "xml": {
        "name": "Id"
      }
    },
    "content": {
      "$ref": "#/definitions/Content",
      "xml": {
        "name": "Content",
        "attribute": false
      }
    }
  }
 }

There is a similar issue with naming the elements within an array... if the "items" contains a $ref I can't change the name of the XML element that wraps each item in the array.

tadhgpearson commented 7 years ago

Anyway, please feel free to weigh in on the various proposals. Community support matters for these things.

This is a tough one. On one hand, I have only ever had a need to overwrite the description column to date. Based on the feedback from @MikeRalphson, it looks like this is the most common use case. However, I can see why a schema creator may also want to overwrite example, required or readOnly... and once schema creators become used to the idea of overwriting descriptions of imported $refs, they will probably want that functionality for all the elements.

The key issue for me is ease-of-use of the two solutions, which is putting the overwrite value beside the ref (the fictional example from the top) vs using a specific overwrite object such as with.

Ultimately, I believe we should consider

  1. Ease of use for schema creators
  2. Ease of use for tool creators

For schema creators, the fictional contract design seems easier because it's intuitive, so long as all element can be overwritten. For example, if description can be overwritten but example cannot, that would seem counter-intuitive. On the other hand, adding an extra with element doesn't seem terribly burdensome to for schema creators, and the fact that it's more explicit may make schemas easier to maintain in the long run.

For tool creators, with seems much easier. It's an explicit overwrite element in the parsing tree, with a defined functionality. Simply overwriting in-line, like our fictional contract, requires a set of rules and exceptions to be programmed into every schema reader.

Given the pros and cons of both options, I would be OK with either, but would lean towards using the with element, or some equivalent. Hope that helps.

pierslawson commented 7 years ago

@tadhgpearson do they have to be considered as overrides? A description alongside a $ref could be seen as an override of the referenced type's description or it could be viewed as a description of the property itself.

In @JimSangwine original example, I think the descriptions he provides are better considered as a description of the property rather than a replacement for the address type's description (i.e. in addition to the address's own description). For example, if an address has a description "5 lines of free form text with a UK postcode", that is still valid, alongside the additional "The premises at which electricity is provided" that is describing this particular usage of the type.

In my case of having a problem with xml, the information held within xml is definitely referring to the property, not the type of the property (i.e. not the $ref)... in which case it certainly isn't meant to be an override of any xml information within the referenced type.

Does this enhancing the property (rather than overriding the type) effect whether you think introducing with is best or not?

Another thought...

It seems to me that the "path of least surprise" would push more towards the description having a common location regardless of whether a property happens to have a type or a $ref. By which I mean, given this example:

{
    addresses: [
        "homeAddress": {
            "type": "string",
            "description": "The premises at which electricity is provided."
        }
    ]
}

If I wanted to change it from type based to $ref based, I would probably assume that I would only have to change one line:

{
    addresses: [
        "homeAddress": {
            "$ref": "#/definitions/Address",
            "description": "The premises at which electricity is provided."
        }
    ]
}

Finding I have to introduce a whole new node (with) and move values into it would be a surprise to me at least ;-)

tadhgpearson commented 7 years ago

As someone who wants a clear, consistent behavior for my specification, I think that having a rule that appends 2 descriptions, like you described, very weird. In this case, a set of rules would be determining how to separately defined fields, potentially far from each other in the specification file, are merged. I don't think that's an obvious behavior, especially in the context of JSON Schema & Swagger, where each field tends to be explicitly defined.

Regarding the location of the fields, I agree with you @pierslawson - with nothing specified today, as we can see from @MikeRalphson's example, specification creators do put the description in the same node as the $ref. There are so many examples of it that I think we can consider it to be intuitive or unsurprising behavior.

The question I would ask from here is, if the spec was changed to define a with sub-node that allows specification creators to overwrite properties from the $referred object, would that still feel intuitive? Or would they constantly be looking at the manual, trying to figure out why it's not working? To me, I think the with specification is actually quite good: I don't find it burdensome to remember, it forces me to bundle the overwriting elements together, and gives me a keyword to find where overwrites occur. However, that's just my own little opinion!