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 #97

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

Schema issues in documentation examples #97

zx80 opened this issue Apr 12, 2024 · 8 comments
Labels
bug Something isn't working

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
Member

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 the bug Something isn't working label 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
Member

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
Member

@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
Member

@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.

@zx80
Copy link
Author

zx80 commented May 24, 2024

@Bellangelo if you want the schema to only consider object valid, then somewhere there needs to be a type: object.

Indeed, otherwise 3.141592653589793 would be a valid Pet, which is probably not what is intended by the API designer.

Also, ISTM that people tend to learn by looking at examples rather than reading documentations in depth, so having good examples helps.

Moreover, AI generators which are expected to help programmers do only learn by example, if you feed them bad stuff they will produce bad stuff.

@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.

Maybe you could add a test expected to fail on an invalid input type (400 Bad Request), so as to get a difference?

About tests, so as to illustrate my point about additional*, a typo in an optional property (tags instead of tag below) would go undetected if the object is not closed somehow:

{ "id":  1234, "name": "Hobbes", "tags": "fierce tiger" }

@handrews
Copy link
Member

We are moving these examples to the learn.openapis.org site, so I am going to transfer this issue so it's easier to see what issues are for the separate examples like these and what are for the examples embedded in the spec. See this issue for the decision:

@handrews handrews transferred this issue from OAI/OpenAPI-Specification May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: No status
Development

No branches or pull requests

3 participants