fix: no error for inline array schema when items field is a ref (#257)

Purpose: - generator flattens reference to an array schema back into an inline schema Changes: - Add a check to see if the schema is an array with an items field that uses a ref - Add a check to see if the array schema has primitive type items - Add reusable isPrimitiveType function Tests: - Add tests to ensure the validator does not flag different variations of this array schema - Update expected warnings in tests that use specs with this array schema - Add test to ensure arrays with primitive-type items are not flagged - Add tests for isPrimitiveType function
IBM · Mar 8, 2021 · 4dd3708 · 4dd3708
1 parent 4b7ae88
commit 4dd3708
Show file tree

Hide file tree

Showing 7 changed files with 218 additions and 22 deletions.
diff --git a/src/plugins/utils/isPrimitiveType.js b/src/plugins/utils/isPrimitiveType.js
@@ -0,0 +1,7 @@
+// takes a schema object that has a 'type' field
+module.exports = function(schema) {
+  return (
+    schema.type &&
+    ['boolean', 'integer', 'number', 'string'].includes(schema.type)
+  );
+};
diff --git a/src/plugins/validation/2and3/semantic-validators/responses.js b/src/plugins/validation/2and3/semantic-validators/responses.js
@@ -1,10 +1,20 @@
 const each = require('lodash/each');
 const { walk, isResponseObject } = require('../../../utils');
 const MessageCarrier = require('../../../utils/messageCarrier');
+const isPrimitiveType = require('../../../utils/isPrimitiveType');
 
 const INLINE_SCHEMA_MESSAGE =
   'Response schemas should be defined with a named ref.';
 
+function arrayItemsAreRefOrPrimitive(schema) {
+  return (
+    schema &&
+    schema.type === 'array' &&
+    schema.items &&
+    (schema.items.$ref || isPrimitiveType(schema.items))
+  );
+}
+
 module.exports.validate = function({ jsSpec, isOAS3 }, config) {
   const messages = new MessageCarrier();
 
@@ -36,9 +46,12 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
                       i < mediaType.schema[schemaType].length;
                       i++
                     ) {
-                      const hasInlineSchema = !mediaType.schema[schemaType][i]
-                        .$ref;
-                      if (hasInlineSchema) {
+                      const currentSchema = mediaType.schema[schemaType][i];
+                      const hasInlineSchema = !currentSchema.$ref;
+                      if (
+                        hasInlineSchema &&
+                        !arrayItemsAreRefOrPrimitive(currentSchema)
+                      ) {
                         messages.addMessage(
                           [
                             ...path,
@@ -57,7 +70,10 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
                     }
                   }
                 });
-              } else if (!mediaType.schema.$ref) {
+              } else if (
+                !mediaType.schema.$ref &&
+                !arrayItemsAreRefOrPrimitive(mediaType.schema)
+              ) {
                 messages.addMessage(
                   [...path, responseKey, 'content', mediaTypeKey, 'schema'],
                   INLINE_SCHEMA_MESSAGE,
@@ -72,7 +88,10 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
           if (responseKey.startsWith('x-')) return;
 
           const hasInlineSchema = response.schema && !response.schema.$ref;
-          if (hasInlineSchema) {
+          if (
+            hasInlineSchema &&
+            !arrayItemsAreRefOrPrimitive(response.schema)
+          ) {
             messages.addMessage(
               [...path, responseKey, 'schema'],
               INLINE_SCHEMA_MESSAGE,

diff --git a/test/cli-validator/tests/expected-output.test.js b/test/cli-validator/tests/expected-output.test.js
@@ -145,7 +145,7 @@ describe('cli tool - test expected output - Swagger 2', function() {
     const validationResults = await inCodeValidator(oas2Object, defaultMode);
 
     expect(validationResults.errors.length).toBe(5);
-    expect(validationResults.warnings.length).toBe(9);
+    expect(validationResults.warnings.length).toBe(8);
     expect(validationResults.infos).not.toBeDefined();
     expect(validationResults.hints).not.toBeDefined();
 
@@ -181,10 +181,9 @@ describe('cli tool - test expected output - Swagger 2', function() {
     expect(capturedText[33].match(/\S+/g)[2]).toEqual('15');
     expect(capturedText[37].match(/\S+/g)[2]).toEqual('15');
     expect(capturedText[41].match(/\S+/g)[2]).toEqual('197');
-    expect(capturedText[45].match(/\S+/g)[2]).toEqual('108');
-    expect(capturedText[49].match(/\S+/g)[2]).toEqual('131');
-    expect(capturedText[53].match(/\S+/g)[2]).toEqual('134');
-    expect(capturedText[57].match(/\S+/g)[2]).toEqual('126');
+    expect(capturedText[45].match(/\S+/g)[2]).toEqual('131');
+    expect(capturedText[49].match(/\S+/g)[2]).toEqual('134');
+    expect(capturedText[53].match(/\S+/g)[2]).toEqual('126');
   });
 
   it('should return exit code of 0 if there are only warnings', async function() {
@@ -364,7 +363,7 @@ describe('test expected output - OpenAPI 3', function() {
     const validationResults = await inCodeValidator(oas3Object, defaultMode);
 
     expect(validationResults.errors.length).toBe(4);
-    expect(validationResults.warnings.length).toBe(11);
+    expect(validationResults.warnings.length).toBe(10);
     expect(validationResults.infos).not.toBeDefined();
     expect(validationResults.hints).not.toBeDefined();
 

diff --git a/test/cli-validator/tests/info-and-hint.test.js b/test/cli-validator/tests/info-and-hint.test.js
@@ -39,7 +39,7 @@ describe('test info and hint rules - OAS3', function() {
     expect(jsonOutput['errors']['schema-ibm'].length).toBe(1);
 
     // Verify warnings
-    expect(jsonOutput['warnings']['responses'].length).toBe(3);
+    expect(jsonOutput['warnings']['responses'].length).toBe(2);
     expect(jsonOutput['warnings']['operations-shared'].length).toBe(2);
     expect(jsonOutput['warnings']['refs'].length).toBe(1);
     expect(jsonOutput['warnings']['schema-ibm'].length).toBe(1);

diff --git a/test/cli-validator/tests/option-handling.test.js b/test/cli-validator/tests/option-handling.test.js
@@ -121,7 +121,7 @@ describe('cli tool - test option handling', function() {
 
     // totals
     expect(capturedText[statsSection + 1].match(/\S+/g)[5]).toEqual('5');
-    expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('9');
+    expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('8');
 
     // errors
     expect(capturedText[statsSection + 5].match(/\S+/g)[0]).toEqual('1');
@@ -138,25 +138,22 @@ describe('cli tool - test option handling', function() {
 
     // warnings
     expect(capturedText[statsSection + 11].match(/\S+/g)[0]).toEqual('2');
-    expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(22%)');
+    expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(25%)');
 
     expect(capturedText[statsSection + 12].match(/\S+/g)[0]).toEqual('2');
-    expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(22%)');
+    expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(25%)');
 
     expect(capturedText[statsSection + 13].match(/\S+/g)[0]).toEqual('1');
-    expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(11%)');
+    expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(13%)');
 
     expect(capturedText[statsSection + 14].match(/\S+/g)[0]).toEqual('1');
-    expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(11%)');
+    expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(13%)');
 
     expect(capturedText[statsSection + 15].match(/\S+/g)[0]).toEqual('1');
-    expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(11%)');
+    expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(13%)');
 
     expect(capturedText[statsSection + 16].match(/\S+/g)[0]).toEqual('1');
-    expect(capturedText[statsSection + 16].match(/\S+/g)[1]).toEqual('(11%)');
-
-    expect(capturedText[statsSection + 17].match(/\S+/g)[0]).toEqual('1');
-    expect(capturedText[statsSection + 17].match(/\S+/g)[1]).toEqual('(11%)');
+    expect(capturedText[statsSection + 16].match(/\S+/g)[1]).toEqual('(13%)');
   });
 
   it('should not print statistics report by default', async function() {

diff --git a/test/plugins/utils/isPrimitiveType.test.js b/test/plugins/utils/isPrimitiveType.test.js
@@ -0,0 +1,42 @@
+const expect = require('expect');
+const isPrimitiveType = require('../../../src/plugins/utils/isPrimitiveType');
+
+describe('isPrimitiveType - util', () => {
+  it('should return true when the schema uses a primitive type', () => {
+    const exampleObject = {
+      schema: {
+        type: 'string'
+      }
+    };
+
+    expect(isPrimitiveType(exampleObject.schema)).toBe(true);
+  });
+
+  it('should return true when given items with a primitive type', () => {
+    const exampleObject = {
+      schema: {
+        type: 'array',
+        items: {
+          type: 'boolean'
+        }
+      }
+    };
+
+    expect(isPrimitiveType(exampleObject.schema.items)).toBe(true);
+  });
+
+  it('should return false when the schema uses a non-primitive type', () => {
+    const exampleObject = {
+      schema: {
+        type: 'object',
+        properties: {
+          exampleProp: {
+            type: 'string'
+          }
+        }
+      }
+    };
+
+    expect(isPrimitiveType(exampleObject.schema)).toBe(false);
+  });
+});
diff --git a/test/plugins/validation/2and3/responses.test.js b/test/plugins/validation/2and3/responses.test.js
@@ -76,6 +76,64 @@ describe('validation plugin - semantic - responses', function() {
         expect(res.errors.length).toEqual(0);
       });
 
+      it('should not complain about inline array schema with items defined as ref', function() {
+        const spec = {
+          paths: {
+            '/stuff': {
+              get: {
+                summary: 'list stuff',
+                operationId: 'listStuff',
+                produces: ['application/json'],
+                responses: {
+                  200: {
+                    description: 'successful operation',
+                    schema: {
+                      type: 'array',
+                      items: {
+                        $ref: '#/components/schemas/ListStuffResponseModel'
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        };
+
+        const res = validate({ jsSpec: spec }, config);
+        expect(res.warnings.length).toEqual(0);
+        expect(res.errors.length).toEqual(0);
+      });
+
+      it('should not complain about inline array schema with primitive type items', function() {
+        const spec = {
+          paths: {
+            '/stuff': {
+              get: {
+                summary: 'list stuff',
+                operationId: 'listStuff',
+                produces: ['application/json'],
+                responses: {
+                  200: {
+                    description: 'successful operation',
+                    schema: {
+                      type: 'array',
+                      items: {
+                        type: 'string'
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        };
+
+        const res = validate({ jsSpec: spec }, config);
+        expect(res.warnings.length).toEqual(0);
+        expect(res.errors.length).toEqual(0);
+      });
+
       it('should not complain for a response with no schema', function() {
         const spec = {
           paths: {
@@ -271,6 +329,80 @@ describe('validation plugin - semantic - responses', function() {
         expect(res.warnings.length).toEqual(0);
         expect(res.errors.length).toEqual(0);
       });
+
+      it('should not complain for a valid combined schema where one schema is an array with items defined as ref', function() {
+        const spec = {
+          paths: {
+            '/stuff': {
+              get: {
+                summary: 'list stuff',
+                operationId: 'listStuff',
+                responses: {
+                  200: {
+                    description: 'successful operation',
+                    content: {
+                      'application/json': {
+                        schema: {
+                          anyOf: [
+                            {
+                              $ref:
+                                '#/components/schemas/ListStuffResponseModel'
+                            },
+                            {
+                              type: 'array',
+                              items: {
+                                $ref:
+                                  '#/components/schemas/ListStuffSecondModel'
+                              }
+                            }
+                          ]
+                        }
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        };
+
+        const res = validate({ jsSpec: spec, isOAS3: true }, config);
+        expect(res.warnings.length).toEqual(0);
+        expect(res.errors.length).toEqual(0);
+      });
+
+      it('should not complain for a valid combined schema where one schema is an array with items defined as ref', function() {
+        const spec = {
+          paths: {
+            '/stuff': {
+              get: {
+                summary: 'list stuff',
+                operationId: 'listStuff',
+                responses: {
+                  200: {
+                    description: 'successful operation',
+                    content: {
+                      'application/json': {
+                        schema: {
+                          type: 'array',
+                          items: {
+                            $ref: '#/components/schemas/ListStuffResponseModel'
+                          }
+                        }
+                      }
+                    }
+                  }
+                }
+              }
+            }
+          }
+        };
+
+        const res = validate({ jsSpec: spec, isOAS3: true }, config);
+        expect(res.warnings.length).toEqual(0);
+        expect(res.errors.length).toEqual(0);
+      });
+
       it('should complain about an inline schema', function() {
         const spec = {
           paths: {