Closed tomkralidis closed 1 year ago
Setting all the GeoJSON schemas to nullable looks a bit weird and is probably not what we want.
The oneOf could be fixed as follows, I think
oneOf:
- nullable: true
enum:
- null
- $ref: geometryGeoJSON.yaml
But maybe Features has a better solution? I assume they also faced this in their schemas?
Additions:
@m-mohr
Setting all the GeoJSON schemas to nullable looks a bit weird and is probably not what we want.
The change by @tomkralidis is correct. Yours is not a valid OpenAPI schema.
But maybe Features has a better solution? I assume they also faced this in their schemas?
I had a look and noticed that we have only added nullable
to properties, not to geometry
. I will create a PR in Features.
The change by @tomkralidis is correct. Yours is not a valid OpenAPI schema.
Sorry, but I disagree.
Why is this invalid OpenAPI? No type set -> all types allowed -> nullable adds null to the allowed types -> enum restricts to a single value (null).
The issue I see with nullable in the subschemas is that if you want to reuse a geometry schema in another part in the future and don't want to allow null, you need to duplicate the schema just to remove nullable.
Also (and most importantly), the geometryGeoJSON is a "oneOf", but for null all schemas apply which makes the validation fail for null!
Why is this invalid OpenAPI? No type set -> all types allowed -> nullable adds null to the allowed types -> enum restricts to a single value (null).
nullable
does not change the semantics in this case:
nullable: A
true
value adds"null"
to the allowed type specified by thetype
keyword, only iftype
is explicitly defined within the same Schema Object.
The other issue is that even if it would conform to OpenAPI 3.0.3, it would allow any value (e.g., geometry: 42
).
For more discussion, see the following proposal that was adopted for v3.0.3: https://github.com/OAI/OpenAPI-Specification/blob/main/proposals/2019-10-31-Clarify-Nullable.md.
On the other hand, you state that ...
The current approach doesn't work for null. Validating geometryGeoJSON.yaml against a null value will fail!
Why would that be the case. Because a null
value will violate the required
constraints? The page linked above clarifies that these do not apply in the null
case.
nullable
was really a bad design and it is good that they got rid of it again.
nullable
does not change the semantic
Yes, I wasn't sure whether nullable is actually required, but it also doesn't hurt. So then the enum is enough.
The other issue is that even if it would conform to OpenAPI 3.0.3, it would allow any value (e.g.,
geometry: 42
).
No! The enum restricts to only null. How can 42 be valid for an enum that just lists null as valid value?
The current approach doesn't work for null. Validating geometryGeoJSON.yaml against a null value will fail!
Why would that be the case. Because a
null
value will violate therequired
constraints? The page linked above clarifies that these do not apply in thenull
case.
Look at the geometryGeoJSON file. There's a oneOf that lists all geometry types, which are all nullable. That means all schemas are valid for the value null, which means more than one sub-schema is valid and thus the whole schema is returning that null is invalid! The null constraint must and should be defined in the Feature/Record, not in the individual geometries. The only way I'm aware of to enforce a "only null" contraint, is via enum in OpenAPI.
That's the corresponding issue visualized with JSON Schema (due to the lack of similar OpenAPI tooling):
nullable
was really a bad design and it is good that they got rid of it again.
This one I agree with 100%!
Yes, you are correct:
an empty object allows all values, including null.
So, it must be
oneOf:
- enum:
- null
- $ref: geometryGeoJSON.yaml
Wow, let's hope that tools understand this edge case :wink:...
FYI geometry null handling updated accordingly.
Addresses #263
cc @pvretano @kalxas @m-mohr