nullable References are invalid in OpenAPI 3.x

Question

nullable References are invalid in OpenAPI 3.x

commonism opened this issue a year ago · 26 comments

Hi,

I consider the use of attaching the nullable property to a Reference invalid v3.1.0.

object cannot be extended with additional properties and any properties added SHALL be ignored.

It's used often.
Minimal reduced example:

openapi: 3.0.3
info:
  title: ''
  version: 0.0.0
servers:
  - url: http://127.0.0.1/api

security:
  - {}

paths: {}

components:
  schemas:
    Chassis_v1_23_0_Chassis:
      additionalProperties: false
      properties:
        EnvironmentalClass:
          $ref: '#/components/schemas/Chassis_v1_23_0_EnvironmentalClass'
          nullable: true

    Chassis_v1_23_0_EnvironmentalClass:
      enum:
      - A1
      type: string

As this causes trouble working with the description documents, it would be great if you'd update the spec to be valid OpenAPI.
As I remember other uses as well, e.g. with arrays, I propose to compile a list with bad refs first and work from there.

commonism commented 7 months ago

Yes.

Answer 1 · 2024-02-01T14:09:04.000Z

Well that's unfortunate... We didn't notice that detail when doing this originally...

And these definitions are converted from our JSON Schema definitions, which also use the same style of structuring the definition... As far as I know, JSON Schema doesn't have this same rule though...

This may be difficult to fix given our modeling pattern and schema tooling process... But we definitely need to fix this...

Answer 2 · 2024-02-02T07:34:40.000Z

nullable is not valid JSON Schema (2020-12-).

But for property references

      type: object
      properties:
        attr:
          $ref: '#/$defs/item'
          nullable: True

Possibilities to address this without side effects:

oneOf for OpenAPI 3.0

      type: object
      properties:
        attr:
          type: object:
          oneOf:
            - $ref: '#/$defs/item'
            - type: object
              additionalProperties: False
              nullable: True

OpenAPI 3.1 - type lists

      type: object
      properties:
        attr:
          type: [object, "none"]
          $ref: '#/$defs/item'

If I remember correctly, in case of reference nullable arrays "None" is supposed to be valid input for

type:  array
items:
  $ref: #/…
  nullable: True

This is possible via:

type:  array
items:
  $ref: #/…
nullable: True

or for OpenAPI 3.1

type:  [array, "none"]
items:
  $ref: #/…

I'd go with 3.1, being JSON Schema compatible will benefit the tooling.

Answer 3 · 2024-02-02T15:15:15.000Z

nullable isn't, but we also overload the usage of $ref with other schema terms (like our descriptions and other property attributes). We do special handling of nullable when converting from JSON Schema to OpenAPI schema.

Essentially, this definition in our JSON Schema:

                "EnvironmentalClass": {
                    "anyOf": [
                        {
                            "$ref": "#/definitions/EnvironmentalClass"
                        },
                        {
                            "type": "null"
                        }
                    ],
                    "description": "The ASHRAE Environmental Class for this chassis.",
                    "longDescription": "This property shall contain the ASHRAE Environmental Class for this chassis, as defined by ASHRAE Thermal Guidelines for Data Processing Environments.  These classes define respective environmental limits that include temperature, relative humidity, dew point, and maximum allowable elevation.",
                    "readonly": false,
                    "versionAdded": "v1_9_0"
                },

is converted to this definition for OpenAPI:

        EnvironmentalClass:
          $ref: '#/components/schemas/Chassis_v1_25_0_EnvironmentalClass'
          description: The ASHRAE Environmental Class for this chassis.
          nullable: true
          readOnly: false
          x-longDescription: This property shall contain the ASHRAE Environmental
            Class for this chassis, as defined by ASHRAE Thermal Guidelines for Data
            Processing Environments.  These classes define respective environmental
            limits that include temperature, relative humidity, dew point, and maximum
            allowable elevation.
          x-versionAdded: v1_9_0

So, this looks like a wider issue beyond seeing nullable along with a $ref.

Answer 4 · 2024-02-02T18:16:32.000Z

Only nullable makes a difference in the protocol accepted, the remains do not really matter, ignoring them does not change behavior.

Answer 5 · 2024-02-02T18:34:21.000Z

That's a good point; that might allow us to find an easier solution to just address nullable rather than fixing everything at once.

Answer 6 · 2024-02-04T08:21:19.000Z

I think I've already provided the non-breaking-changes options available, oneOf for properties, nullable to array scope in case the array can be null, or items: oneOf: if the items can be null.

Once you have the possibility of breaking changes … I'd look into this use of nullable, I consider it an anti-pattern as it requires checking for "is set and is not null".
Serving arrays empty as in [] or providing a default [] (to allow excluding from the serialization when empty) serves the same purpose and simplifies code on the receiving end. I'd expect serving it empty to be the ecosystem friendly choice, compared to an (complex) default value. There is little value in null as value for array items.
Same for properties/attributes, nullable requires checking for "is set and not null". For most frameworks null is implicit by by not having it required, not providing a default value and not serializing and could be checked by "is set" without having it nullable.

Answer 7 · 2024-02-06T16:46:14.000Z

Coming back to this:

oneOf for OpenAPI 3.0

      type: object
      properties:
        attr:
          type: object:
          oneOf:
            - $ref: '#/$defs/item'
            - type: object
              additionalProperties: False
              nullable: True

I was not really satisfied with having useless container elements for all these nullable attributes.
Skipping the type: object may allow generators to union the type(s) from the oneOf (JSON Schema), instead of creating a model of the union for it - avoiding a container element/redirection.

    type: object
    properties:
       attr:
#         type: object
         oneOf:
           - $ref: '#/$defs/item'
           - type: object
             additionalProperties: False
             nullable: True

Answer 8 · 2024-02-06T19:15:09.000Z

Update - may be better not to drop the type specification due to side effects.

Answer 9 · 2024-02-11T10:28:04.000Z

The side effects I mentioned been improper implementation for applicators and primitive types on my side.
When done properly, the type specified for the container schema has to match the referenced schema. therefore not specifying the type is less maintenance and recommended.

Using an "enum: [null]" instead of the previously proposed object avoids changing the protocol definition by accepting empty objects "{}" in addition to null.

Therefore new proposal:

A:
  oneOf:
   - $ref: …
   - enum: [null]

Answer 10 · 2024-02-11T15:21:19.000Z

I like it; it's minimally invasive. I'd like to do some inspection where we might hit this elsewhere beyond just these enum cases, but I think we'll go forward with this.

Answer 11 · 2024-02-16T14:10:44.000Z

@commonism before I start piecing together tooling changes (and version bumps to have new schemas published), do you see there would be an issue with other terms alongside with $ref. I know things like descriptions are non-functional so it should be safe to leave those as is, but the other term that looks like it might cause problems is readonly. Do you have any opinion about this one in particular?

Answer 12 · 2024-02-16T14:28:52.000Z

I never cared for readOnly, ignoring it completely did not cause any problems for me so far.
Being outside of JSON Schema/Core, I can't say for sure the semantics for readOnly with applicators are even explicitly defined.

That said I'd go with:

A:
  properties:
    a:
      readOnly: True
      oneOf:
      - $ref: …
      - enum: [null]

information is not lost, the usage is opt-in as is.

Answer 13 · 2024-02-16T14:43:09.000Z

Thanks for thinking about that aspect. One last thing; for arrays, would the structure look like this?

A:
  properties:
    a:
      readOnly: True
      type: array
      items:
        oneOf:
        - $ref: …
        - enum: [null]

Answer 14 · 2024-02-16T15:30:31.000Z

what is expected to be nullable, individual items or the array itself?

item nullable

A:
 properties:
   a:
     readOnly: True
     type: array
     items:
       oneOf:
       - $ref: …
       - enum: [null]

valid input

{"a":[null, null]}

array nullable

A:
  properties:
    a:
      readOnly: True
      oneOf:
      - type: array
        items:
          $ref: …
      - enum: [null]

valid input

{"a":null}

I'm quite confident redfish is limited to require only array level nullable.

Answer 15 · 2024-02-16T17:01:44.000Z

The individual items in the array are nullable; arrays themselves are not nullable. This is mainly because of a modeling limitation we inherited from OData.

So... This is not allowed today:

{
    "A": null
}

But this is allowed:

{
    "A": [ "SomeString", null, null, null ]
}

Answer 16 · 2024-02-16T17:42:16.000Z

would the structure look like this?

A:
  properties:
    a:
      readOnly: True
      type: array
      items:
        oneOf:
        - $ref: …
        - enum: [null]

yes

Answer 17 · 2024-02-16T17:46:39.000Z

Excellent; thanks for confirming

Answer 18 · 2024-03-24T19:20:09.000Z

@commonism I'm trying to prep for these changes, and while going through other properties, I notice this might be a wider issue than just enum properties. Take this as an example (this same property is in every resource):

        Description:
          $ref: http://redfish.dmtf.org/schemas/v1/Resource.yaml#/components/schemas/Resource_Description
          nullable: true
          readOnly: true

While the $ref doesn't resolve to an enumeration, I expect the same limitation applies. Should these instances be fixed as well? This would result in the definition changing to...

        Description:
          oneOf:
          - $ref: http://redfish.dmtf.org/schemas/v1/Resource.yaml#/components/schemas/Resource_Description
          - enum: [null]
          readOnly: true

Answer 19 · 2024-03-24T19:53:34.000Z

Yes, all instances where a Reference has additional attribute "nullable" need to be addressed.

c.f. my cure for the current state:

https://github.com/commonism/aiopenapi3_redfish/blob/9aae3854d1c2060d50e496de16c97254ef523184/src/aiopenapi3_redfish/clinic.py#L70-L107

(the array part needs to be updated to be oneOf as well)

Answer 20 · 2024-03-25T01:18:23.000Z

Excellent, thanks for confirming. I'll look to adjust the converter to have the same handling regardless of this being an enum or some other type of property.

Answer 21 · 2024-05-22T17:42:41.000Z

published in 2024.1
https://github.com/DMTF/Redfish-Publications/blob/5b217908b5378b24e4f390c063427d7a707cd308/openapi/ActionInfo.v1_0_8.yaml#L18-L23

Answer 22 · 2024-05-24T18:35:32.000Z

2024.1 specifies

oneOf:
- enum: ["null"]
- …

this is different to our discussion

oneOf:
- enum: [null]
- …

therefore 2024.1 allows "null" - the literal string value "null" instead of being nullable.

And the operation did not address all nullable refs:
e.g.

Answer 23 · 2024-05-28T13:22:13.000Z

Volume is unfortunately owned by SNIA; Swordfish will need a new release for us to pick up those changes.

Answer 24 · 2024-05-28T13:23:18.000Z

I'll need to fix up the "null" usage...

Answer 25 · 2024-05-28T20:13:19.000Z

@commonism just to double check before I start raising this with others, this snippet is correct?

        EnvironmentalClass:
          description: The ASHRAE Environmental Class for this chassis.
          oneOf:
          - $ref: '#/components/schemas/Chassis_v1_25_1_EnvironmentalClass'
          - enum:
            - null
          readOnly: false
          x-longDescription: This property shall contain the ASHRAE Environmental
            Class for this chassis, as defined by ASHRAE Thermal Guidelines for Data
            Processing Environments.  These classes define respective environmental
            limits that include temperature, relative humidity, dew point, and maximum
            allowable elevation.
          x-versionAdded: v1_9_0