OAI / learn.openapis.org

OpenAPI - Getting started, and the specification explained
https://learn.openapis.org/
Creative Commons Attribution 4.0 International
112 stars 56 forks source link

Schema issues in documentation examples #97

Open zx80 opened 4 months ago

zx80 commented 4 months ago

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

handrews commented 4 months ago

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

zx80 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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: