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

Optional object references are non-compliant with OpenAPI specification 3.0.3 #925

Open
TimoGuenther opened this issue Jan 27, 2023 · 3 comments

Comments

@TimoGuenther
Copy link

TimoGuenther commented Jan 27, 2023

DRF Spectacular translates optional object references (e.g., child: Child | None in Python) to OpenAPI like this:

child:
  allOf:
  - $ref: '#/components/schemas/Child'
  nullable: true

However, while looking intuitive, this nullable statement is actually a no-op according to the OpenAPI specification version 3.0.3 to which DRF Spectacular adheres -- nullable only does anything when it is true and adjacent to type:

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

Further reading on this:

Indeed, in compliance with the specification, the OpenAPI generator for Python disallows a value of None for child with the above schema.

To fix this, in OpenAPI 3.1.0, we could say:

child:
  oneOf:
  - $ref: '#/components/schemas/Child'
  - type: 'null'

Unfortunately, type: 'null' is not yet supported in OpenAPI 3.0.3. Instead, we have to differentiate the optional type:

OptionalChild:
  type: object
  nullable: true
Child:
  allOf:
   - $ref: '#/components/schemas/OptionalChild'
  type: object  # excludes 'null'

There are a few options to lessen the unwieldiness of this solution:

  • Make generating optional types a configurable opt-in flag.
  • Only generate optional types if they are ever required by other components or endpoints.
  • Only generate both types if both are required by other components or endpoints, otherwise setting the value of nullable on the sole (non-prefixed) generated type as needed.
@tfranzel
Copy link
Owner

Hi @TimoGuenther,

However, while looking intuitive, this nullable statement is actually a no-op according to the OpenAPI specification version 3.0.3 to which DRF Spectacular adheres -- nullable only does anything when it is true and adjacent to type:

I don't fully understand. It is true and indirectly adjacent to a type. That last part is probably the contested point.

This construction is due to nature of references. They replace themselves (and their context) with the content of that $ref. That is why it needs to be wrapped in a allOf/anyOf/oneOf, to not erase the nullable in the process. So step1: substitute the ref content; step2: resolve allOf statement. The parsed&processed end result of that snippet should be:

child:
  type: object    #  the substituted Child ref
  properties: ... #  the substituted Child ref  
  nullable: true

Which looks as it is intended I suppose.

type: object # excludes 'null'

I don't understand how this is supposed to work. From my perspective this is actually a NOP in that example.

Indeed, in compliance with the specification, the OpenAPI generator for Python disallows a value of None for child with the above schema.

That might be an issue with the generator target. openapi-generator is by no means complete or flawless and different targets behave differently. It is not the best indicator for schema validity. Have you checked against typescript-fetch for comparison?

I see your points, however this will introduce another hardcoded prefix, which may produce collisions. Also it will increase parsing complexity and it will require another postprocessing step as you cannot know all the usages beforehand and so cannot decide on whether to use Optional... at all. I'm not yet convinced that this is worth the effort.

@tfranzel
Copy link
Owner

This is a comment of one of the main contributors, which seems to support my interpretation:

OAI/OpenAPI-Specification#1368 (comment)

Can allOf be used to define a non-nullable subtype of nullable base schema?
Yes. The subtype can specify a type without nullable: true, or can specify not: {enum: [null]}.

Also I do interpret the official clarification in my favor. (subtype/subschema refers to what is included in *Of)

Can allOf be used to define a nullable subtype of non-nullable base schema? (See OAI/OpenAPI-Specification#1368.)
No. Subtypes can add constraints, but not relax them.

Given all this, I think your 3rd yaml snippet is even "less correct" and violates the clarification.

I think this aspect of OpenAPI 3.0.x is a hot mess and the fact that there were thousands of words written on this topic show that is not that clear cut. There is still very little clarity, and no bullet-proof definition to depend on.

@TimoGuenther
Copy link
Author

Thanks for the elaborate reply, @tfranzel. I hope my lengthy response does it justice.


I think this aspect of OpenAPI 3.0.x is a hot mess and the fact that there were thousands of words written on this topic show that is not that clear cut. There is still very little clarity, and no bullet-proof definition to depend on.

I agree. Certainly, we both would like to spend our time more meaningfully than to debate something that seems as trivial as nullable references. :) But let us proceed for the sake of correctness.


So step1: substitute the ref content; step2: resolve allOf statement.

We both agree on step 1:

child:
  allOf:
  - type: object
    properties: ...
  nullable: true

However, the question is whether step 2 really simply inlines the keywords from allOf into child as in your snippet, thus activating nullable with the existence of type. Unfortunately, I could not find a definitive answer to that; OpenAPI simply refers to JSON Schema for allOf:

The following properties are taken from the JSON Schema definition but their definitions were adjusted to the OpenAPI Specification.

  • [...]
  • allOf - Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.

But JSON Schema does not concern itself with inlining vs. including for allOf:

An instance validates successfully against this keyword if it validates successfully against all schemas defined by this keyword's value.

To JSON Schema, it's all just additional constraints where each keyword stands alone. Since we are interested in the case of two keywords (nullable and type) suddenly interacting with each other, it looks like all we have to go on is the official clarification to fill this awkward gap between JSON Schema and OpenAPI.


Also I do interpret the official clarification in my favor. (subtype/subschema refers to what is included in *Of)

JSON Schema defines subschema as you do. I could not find a definition for subtype, but in accordance with typical type inheritance like in OOP, I would argue that the directionality of type is the opposite: Subschema actually correlates more closely to supertype (not subtype), because a subtype extends a supertype by adding constraints to an existing subschema.

With that interpretation, those two official clarification statements you quoted support my interpretation of nullable.

That interpretation also explains the inheritance of Child and OptionalChild from my opening post: Child inherits OptionalChild, i.e., it contains all the constraints of the subschema/supertype OptionalChild, but it also adds another constraint for excluding the null value by saying type: object (without nullable).

In any case, the following statement from the official clarification works regardless of direction of type:

nullable: true operates within a single Schema Object. It does not "override" or otherwise compete with supertype or subtype schemas defined with allOf or other applicators. It cannot be directly "inherited" through those applicators, and it cannot be applied to an inherited type constraint.

To me, this is saying pretty explicitly that the type constraint in allOf does not activate the indirect nullable.


This is a comment of one of the main contributors, which seems to support my interpretation:

OAI/OpenAPI-Specification#1368 (comment)

That comment predates OAS 3.0.3 by several years. There has since been a lot of back and forth on that topic. Even if singular allOf attributes with boolean flags are (or used to be) intended, the whole official clarification of nullable favors technicality over intention.


openapi-generator is by no means complete or flawless and different targets behave differently. It is not the best indicator for schema validity. Have you checked against typescript-fetch for comparison?

Good point. Indeed, most targets align with your definition and treat the property in question as nullable; of the ones I tested, only python does not:

target accepts null indicator
python no child does not inherit NoneBase
python-nextgen yes child: Optional[Any] = None in class Parent
rust yes pub child: Option<Option<Box<crate::models::Child>>> instead of one less Option<...> in struct Parent
typescript-fetch yes NullableChild instead of Child

Of course, it is also common to accept non-compliant features like the future type: 'null'.


I'm not yet convinced that this is worth the effort.

I understand; feasibility is, of course, another valuable consideration beside correctness.

Perhaps an easy and only slightly redundant solution is to ensure bulletproof activation of nullable with the existence of an adjacent type keyword by copying its value from the subschema:

child:
  type: object
  allOf:
  - type: object
    properties: ...
  nullable: true

This requires type to be defined on the subschema. I bet that this is the case for all subschemas generated by DRF Spectacular because they all represent custom classes (type: object) or primitives (type: string, etc.).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants