nullable in schema object must have a type sibling

[adamaltman]

Describe the bug

From 3.0 about nullable:

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.

To Reproduce

This schema should not be valid because foo definition has nullable and that does not have an explicit sibling of type.

schema:
  type: object
  properties:
    foo:
       nullable: true

People also seem to think this is valid, but it isn't:

schema:
  type: object
  properties:
    foo:
       nullable: true
       allOf:
          $ref: # some reference

Expected behavior

Let the user know where incorrect use of nullable has happened.

Logs

N/A

OpenAPI definition

OpenAPI 3.0.

Redocly Version(s)

1.0.0-beta.112

Node.js Version(s)

16

Additional context

n/a

[adamaltman]

@AntonKozachuk you didn't close this issue. You should have put in the reference of the description "Closes #925" or "Fixes #925"

[jroyal]

I want to preface this by saying that i'm not sure what the correct answer is, but I think the recent change in #938 broke validation for a specific use case. My understanding of the spec is that the following should be ok because its not a ref, but the new validator says that type needs to be specified.

api-response-single:
      type: object
      allOf:
        - $ref: "#/components/schemas/api-response-common"
        - properties:
            result:
              nullable: true
              anyOf:
                - type: object
                - type: string

Curious to hear your thoughts on it?

[marqu3z]

It seems like https://github.com/Redocly/redocly-cli/pull/938 is a breaking change.

Schemas like

 veryBigNumber:
      allOf:
        - $ref: '#/components/schemas/BigInt'
        - nullable: true

Are not valid anymore even though the ref schema has a type field declared.

@adamaltman is this the intended behavior?

[alsoba13]

Hi @adamaltman, Why is this definition a wrong use of nullable?

    ResultDetail:
      type: object
      properties:
        declineReason:
          nullable: true
          allOf:
          - $ref: '#/components/schemas/ResultDetailDeclineReason'
    ResultDetailDeclineReason:
      type: string
      enum:
      - CANCELLED_BY_USER

What type should declineReason have? Can you provide an example of valid definition of this?

[alexwiese]

I don't think this is actually correct.

The spec doesn't say anything about it being invalid, just that it doesn't add null as a possible value for that type.

From the clarifying proposal:

Does an untyped schema (without a type keyword) allow null values by default? What effect, if any, does nullable: true have on an untyped schema? Yes, an untyped schema allows null values, in addition to all other types. nullable: true has no effect, because null values are already allowed. And nullable: false has no effect because it just leaves the type constraint unmodified.

I interpret that as saying it's perfectly valid to have nullable: true without type specified (it just has no effect). I cannot find anything in the specification that states that type MUST BE specified when nullable: true is.

I think #938 should be reverted.

[mgirouard]

Thanks for the proposal ref, @alexwiese. I came here for exactly this. I hope this clarifies things for this issue as well.

We're pretty motivated to fix this as it has broken a few integrations for us internally. Working around it is possible with an ignore file, but this doesn't scale when multiple teams across multiple repos are affected. We're now having to adjust our common schemas in order to work around things en masse.

[RomanHotsiy]

I'll reopen this for further investigation.

[IGx89]

Still an issue FYI. Getting The 'type' field must be defined when the 'nullable' field is used. on the following OpenAPI 3.0.1 doc section:

"product": {
    "allOf": [
        {
            "$ref": "#/components/schemas/Product"
        }
    ],
    "nullable": true
},

This was generated by Swashbuckle.AspNetCore.

I looked at the rule's code and it's pretty basic -- has no awareness of allOf:

        if (propName === 'nullable' && !node.type) {
          report({
            message: 'The `type` field must be defined when the `nullable` field is used.',
            location: location.child([propName]),
          });
        }

Disabling the spec rule is the workaround, but obviously not ideal...

[magnusja]

Disabling the spec rule is the workaround, but obviously not ideal...

Is there any better solution? I feel that spec also includes some other important checks? Would be better if I disable a rule one specific check would be affected.

[tatomyr]

I think the behaviour needs to be revised. @lornajane, could you take a look at this?

@magnusja, @IGx89, disabling the spec rule is not a good workaround as it checks the most essential aspects of API descriptions. Until it's fixed (if it will be), I'd recommend adding that specific error to the ignore file.

[lornajane]

I support reverting #938, it's too limited to be a good solution. However there are some situations where we're not warning about a potential problem. Let's add a separate rule to check it so we can add a bit more advanced logic and let users opt in to using it (or at least disable that more easily). Note also that this behaviour changed between 3.0 and 3.1

[RomanHotsiy]

The current check is valid:

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.

allOf is not considered by the spec.

On the other side, there is nothing in the spec that says you can't use nullable without a type. it's just that the nullable doesn't have any effect if it's not defined next by the explicit type.

I agree with @lornajane we should extract it from hte spec rule and move it into a separate rule which can be added as a warning to the minimal ruleset.

[RomanHotsiy]

@IGx89 btw your specific case is explicitly mentioned in the issue:

[tatomyr]

Related: https://github.com/Redocly/redocly-cli/issues/386