Repository: Redocly/redocly-cli
Pull request #938 (2022-11-16 10:30:28)

fix: nullable in schema object must have a type sibling
## What/Why/How?
We need to let the user know where incorrect use of nullable has happened.

## Reference
#925 

## Testing
Locally.

## Screenshots (optional)
N/a

## Check yourself

- [ ] Code is linted
- [ ] Tested with redoc/reference-docs/workflows
- [ ] All new/updated code is covered with tests

## Security

- [ ] Security impact of change has been considered
- [ ] Code follows company security practices and guidelines


github-actions[bot]: <!-- jest coverage report action for options with hash 3339ae53a38c44382c0fb4491bdb282d -->

## Coverage report 



| <div title="Status of coverage:&#10; 🟢 - ok&#10; 🟡 - slightly more than threshold&#10; 🔴 - under the threshold">St.<sup>:grey_question:</sup></div> | Category   | Percentage | Covered / Total |
| :----------------------------------------------------------------------------------------------------------------------------------------------------: | :--------- | :--------- | :-------------: |
|                                                                           🟡                                                                           | Statements | 73.55%     |    3325/4521    |
|                                                                           🟡                                                                           | Branches   | 64.79%     |    1849/2854    |
|                                                                           🟡                                                                           | Functions  | 66.08%     |     563/852     |
|                                                                           🟡                                                                           | Lines      | 73.55%     |    3090/4201    |


## Test suite run success
513 tests passing in 84 suites.

<p align="right">Report generated by <a href="https://github.com/ArtiomTr/jest-coverage-report-action">🧪jest coverage report action</a> from 4b6027638f8ccb11edf03d00e82243f361ef0a04</p>

AntonKozachuk: @adamaltman with this fix the Rebilly spec we have added to CLI and used in benchmarks is failing: https://github.com/Redocly/redocly-cli/actions/runs/3478505767/jobs/5816135478
Is this expected?

Patch:
diff --git a/packages/core/src/benchmark/benches/rebilly.yaml b/packages/core/src/benchmark/benches/rebilly.yaml
index 5448d76d..61bd6776 100644
--- a/packages/core/src/benchmark/benches/rebilly.yaml
+++ b/packages/core/src/benchmark/benches/rebilly.yaml
@@ -4238,10 +4238,11 @@ components:
               type: integer
               readOnly: true
             dueReminderTime:
-              description: Time past due reminder event will be triggered
+              description: Date and time at which a past due reminder event is triggered.
               nullable: true
-              allOf:
-                - $ref: '#/components/schemas/ServerTimestamp'
+              type: string
+              format: date-time
+              readOnly: true
             dueReminderNumber:
               description: Number of past due reminder events triggered
               type: integer
@@ -9524,10 +9525,11 @@ components:
           allOf:
             - $ref: '#/components/schemas/ServerTimestamp'
         expirationReminderTime:
-          description: Time expiration reminder event will be triggered
+          description: Date and time at which an expiration reminder event is triggered.
           nullable: true
-          allOf:
-            - $ref: '#/components/schemas/ServerTimestamp'
+          type: string
+          format: date-time
+          readOnly: true
         expirationReminderNumber:
           description: Number of expiration reminder events triggered
           type: integer
@@ -10226,9 +10228,10 @@ components:
               readOnly: true
             reviewTime:
               description: Date and time of manual review.
+              type: string
               nullable: true
-              allOf:
-                - $ref: '#/components/schemas/ServerTimestamp'
+              format: date-time
+              readOnly: true
             documentMatches:
               $ref: '#/components/schemas/KycDocumentMatches'
             _links:
@@ -10587,19 +10590,22 @@ components:
         deliveryAddress:
           description: Delivery address
           nullable: true
+          type: object
           allOf:
             - $ref: '#/components/schemas/ContactObject'
         billingAddress:
           description: Billing address
           nullable: true
+          type: object
           allOf:
             - $ref: '#/components/schemas/ContactObject'
         riskMetadata:
           nullable: true
+          type: object
           example: null
-          description: >-
-            Risk metadata. If null, the value would coalesce to the risk
-            metadata captured when creating the payment token.
+          description: |-
+            Risk metadata.
+            If null, the value would coalesce to the risk metadata captured when creating the payment token.
           allOf:
             - $ref: '#/components/schemas/RiskMetadata'
         activationTime:
@@ -10842,18 +10848,16 @@ components:
                   type: string
                   format: date-time
             invoiceTimeShift:
-              description: >
-                You can shift issue time and due time of invoices for this
-                subscription.
+              description: |-
+                You can shift issue time and due time of invoices for this subscription.
 
-                This setting overrides plan settings. To use plan settings, set
-                `null`.
-
-                To use multiple plans in one subscription they all must have the
-                same billing period,
+                This setting overrides plan settings.
+                To use plan settings, set `null`.
 
+                To use multiple plans in one subscription they all must have the same billing period,
                 this property allows to subscribe to different plans.
               nullable: true
+              type: object
               example: null
               allOf:
                 - $ref: '#/components/schemas/InvoiceTimeShift'
@@ -10898,19 +10902,21 @@ components:
               type: integer
               readOnly: true
             renewalReminderTime:
-              description: Time renewal reminder event will be triggered
+              description: Time renewal reminder event will be triggered.
               nullable: true
-              allOf:
-                - $ref: '#/components/schemas/ServerTimestamp'
+              type: string
+              format: date-time
+              readOnly: true
             renewalReminderNumber:
               description: Number of renewal reminder events triggered
               type: integer
               readOnly: true
             trialReminderTime:
-              description: Time renewal reminder event will be triggered
+              description: Time renewal reminder event will be triggered.
               nullable: true
-              allOf:
-                - $ref: '#/components/schemas/ServerTimestamp'
+              type: string
+              format: date-time
+              readOnly: true
             trialReminderNumber:
               description: Number of renewal reminder events triggered
               type: integer
@@ -11807,10 +11813,12 @@ components:
         paymentInstrument:
           $ref: '#/components/schemas/PaymentInstrument'
         billingAddress:
-          description: >-
-            Billing Address. If not supplied, we use the billing address
-            associated with the payment instrument, and then customer.
+          description: |-
+            Billing address.
+            If not supplied, the billing address associated
+            with the payment instrument is used, and then customer.
           nullable: true
+          type: object
           allOf:
             - $ref: '#/components/schemas/ContactObject'
         requestId:
diff --git a/packages/core/src/rules/common/spec.ts b/packages/core/src/rules/common/spec.ts
index d2ec1baf..8c7644ff 100644
--- a/packages/core/src/rules/common/spec.ts
+++ b/packages/core/src/rules/common/spec.ts
@@ -158,6 +158,13 @@ export const OasSpec: Oas3Rule | Oas2Rule = () => {
             });
           }
         }
+
+        if (propName === 'nullable' && !node.type) {
+          report({
+            message: 'The `type` field must be defined when the `nullable` field is used.',
+            location: location.child([propName]),
+          });
+        }
       }
     },
   };


Test patch:
diff --git a/__tests__/lint/no-invalid-media-type-examples-invalid-schema/snapshot.js b/__tests__/lint/no-invalid-media-type-examples-invalid-schema/snapshot.js
index 47c998f8..0640a6cb 100644
--- a/__tests__/lint/no-invalid-media-type-examples-invalid-schema/snapshot.js
+++ b/__tests__/lint/no-invalid-media-type-examples-invalid-schema/snapshot.js
@@ -3,7 +3,21 @@
 exports[`E2E lint no-invalid-media-type-examples-invalid-schema 1`] = `
 
 validating /openapi.yaml...
-[1] openapi.yaml:28:17 at #/paths/~1test/get/responses/202/content/application~1json/schema
+[1] openapi.yaml:16:17 at #/components/schemas/Test/nullable
+
+The \`type\` field must be defined when the \`nullable\` field is used.
+
+14 |   schemas:
+15 |     Test:
+16 |       nullable: true
+   |                 ^^^^
+17 | paths:
+18 |   "/test":
+
+Error was generated by the spec rule.
+
+
+[2] openapi.yaml:28:17 at #/paths/~1test/get/responses/202/content/application~1json/schema
 
 Example validation errored: "nullable" cannot be used without "type".
 
@@ -21,8 +35,8 @@ Warning was generated by the no-invalid-media-type-examples rule.
 
 /openapi.yaml: validated in <test>ms
 
-Woohoo! Your OpenAPI definition is valid. 🎉
-You have 1 warning.
+❌ Validation failed with 1 error and 1 warning.
+run \`openapi lint --generate-ignore-file\` to add all problems to the ignore file.
 
 
 `;
diff --git a/packages/core/src/rules/common/__tests__/spec.test.ts b/packages/core/src/rules/common/__tests__/spec.test.ts
index 1d45dfb4..122fe301 100644
--- a/packages/core/src/rules/common/__tests__/spec.test.ts
+++ b/packages/core/src/rules/common/__tests__/spec.test.ts
@@ -138,6 +138,176 @@ describe('Oas3 spec', () => {
       ]
     `);
   });
+
+  it('should report on nullable without type', async () => {
+    const document = parseYamlToDocument(
+      outdent`
+      openapi: 3.0.0
+      components:
+        requestBodies:
+          TestRequestBody:
+            content:
+              application/json:
+                schema:
+                  nullable: true
+        `,
+      'foobar.yaml'
+    );
+
+    const results = await lintDocument({
+      externalRefResolver: new BaseResolver(),
+      document,
+      config: await makeConfig({ spec: 'error' }),
+    });
+
+    expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The field \`paths\` must be present on this level.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The field \`info\` must be present on this level.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "location": Array [
+            Object {
+              "pointer": "#/components/requestBodies/TestRequestBody/content/application~1json/schema/nullable",
+              "reportOnKey": false,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The \`type\` field must be defined when the \`nullable\` field is used.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+      ]
+    `);
+  });
+
+  it('should report on nullable with type defined in allOf', async () => {
+    const document = parseYamlToDocument(
+      outdent`
+      openapi: 3.0.0
+      components:
+        requestBodies:
+          TestRequestBody:
+            content:
+              application/json:
+                schema:
+                  nullable: true
+                  allOf:
+                    - $ref: "#/components/requestBodies/TestSchema"
+          schemas:
+            TestSchema:
+              title: TestSchema
+              type: object
+        `,
+      'foobar.yaml'
+    );
+
+    const results = await lintDocument({
+      externalRefResolver: new BaseResolver(),
+      document,
+      config: await makeConfig({ spec: 'error' }),
+    });
+
+    expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The field \`paths\` must be present on this level.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The field \`info\` must be present on this level.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "location": Array [
+            Object {
+              "pointer": "#/components/requestBodies/TestRequestBody/content/application~1json/schema/nullable",
+              "reportOnKey": false,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The \`type\` field must be defined when the \`nullable\` field is used.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/components/requestBodies/schemas",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "The field \`content\` must be present on this level.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+        Object {
+          "from": undefined,
+          "location": Array [
+            Object {
+              "pointer": "#/components/requestBodies/schemas/TestSchema",
+              "reportOnKey": true,
+              "source": "foobar.yaml",
+            },
+          ],
+          "message": "Property \`TestSchema\` is not expected here.",
+          "ruleId": "spec",
+          "severity": "error",
+          "suggest": Array [],
+        },
+      ]
+    `);
+  });
 });
 
 describe('Oas3.1 spec', () => {