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

Schema issues in documentation examples #3704

Open
zx80 opened this issue Apr 12, 2024 · 6 comments
Open

Schema issues in documentation examples #3704

zx80 opened this issue Apr 12, 2024 · 6 comments
Labels
bug examples requests for more or better examples in the specification

Comments

@zx80
Copy link

zx80 commented Apr 12, 2024

Here are 3 possible issues in schemas used in the documentation examples, that could be fixed:

  • in upsto example, the two uriref formats at the end should be uri-ref to conform to JSON Schema v5?
  • in webhook the Pet schema should have a "type": "object" added, and maybe an "additionalProperties": false.
  • in callback the 201 response schema should also have a "type": "object" added, and maybe an "additionalProperties": false.
@handrews
Copy link
Contributor

The whole "JSON Schema v5" thing was a bit of a misunderstanding- that draft is mostly just draft-04 tidied up. the uriref/uri-ref format was something that slipped in but was never implemented because there's no way to detect "draft-05" as distinct from draft-04. In draft-06 (we gave up and skipped 5 because of the confusion) we added it properly as uri-reference.

On the plus side (sort-of), unrecognized format values are ignored so it's mostly harmless. But it should match the spec.


I'd advise against "additionalProperties": false

@handrews handrews added bug examples requests for more or better examples in the specification labels Apr 12, 2024
@zx80
Copy link
Author

zx80 commented Apr 12, 2024

But it should match the spec.

I know that v5 was a kind-of a misunderstanding, but anyway spec examples should really match the spec that they were created to illustrate…

I'd advise against "additionalProperties": false

This one is just a suggestion. However, why not? ISTM that most of the time people should use that instead of the default, so it would not be that bad if a few examples in the documentation do that.

@handrews
Copy link
Contributor

However, why not?

Mostly, because it's something of a footgun and makes your schemas difficult if not impossible to re-use. Which, particularly when using a pet example (implicitly referencing the inheritance in the petstore example) is best avoided.

The re-use problems were mostly solved in OAS 3.1 / draft 2020-12 with unevaluatedProperties, and I really don't want to generate an endless stream of complaints about additionalProperties here from people who copy the example and then try to extend it.

ISTM that most of the time people should use that instead of the default

This impression is common among users of strictly typed languages and uncommon among users of dynamically typed languages.

@zx80
Copy link
Author

zx80 commented Apr 13, 2024

The re-use problems were mostly solved in OAS 3.1 / draft 2020-12 with unevaluatedProperties, and I really don't want to generate an endless stream of complaints about additionalProperties here from people who copy the example and then try to extend it.

Then would it be possible to add unevaluatedProperty in some example, if it does (more or less) solve the re-use (inheritance) issue?

ISTM that most of the time people should use that instead of the default

This impression is common among users of strictly typed languages and uncommon among users of dynamically typed languages.

I do not buy this usual argument in full: if a dynamic person actually bothers to declare a schema, i.e. a type, I do not understand why they would stop midstream: they get all of the pain with only part of the benefit.

@Bellangelo
Copy link
Contributor

@handrews @zx80 About the type: object, do we really need it? I used a variation of our test files to check it and it seems that it passes. Also, I checked what JSON our YAML files produce and it is exactly the same as our own.

@handrews
Copy link
Contributor

@Bellangelo if you want the schema to only consider object valid, then somewhere there needs to be a type: object. But if you are composing multiple schemas, they don't all need to repeat that type: object. So whether a given schema needs it depends on what the goal is and whether there are other schemas applied to the same instance location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug examples requests for more or better examples in the specification
Projects
Status: No status
Development

No branches or pull requests

3 participants