Repository: BRIKEV/express-jsdoc-swagger
Pull request #148 (2021-04-17 17:37:23)

feat: make optional properties nullable by default
### What kind of change does this PR introduce? (check at least one)
 - [ ] Bugfix
 - [X] Feature
 - [X] Test
 - [X] Docs
 - [X] Refactor
 - [ ] Build-related changes
 - [ ] Other, please describe:

### Description:
This PR introduces a new option to the library's configuration object, `notRequiredAsNullable`. This will allow users to **configure whether non-required props shall be interpreted as `nullable` or not**. To maintain the current behaviour, this setting will be disabled by default. 

### Background:

Currently, a required property is just expected not to be `undefined` in our request or response payload. Thus, we're considering `null` as an actual value for a property. For this, we're working under the assumption that **the meaning of `undefined` and `null` is different**:

- `null` represents an explicit empty value.
- `undefined` means that we have no idea of the real value of that property. It could either be empty, or not. 

Let's better illustrate the difference with an example:

```
/**
 * @typedef {object} Song
 * @property {string} title.required - The title
 * @property {string} artist - The artist
 * @property {string|null} album - The album to which the song belongs (if any)
 */
```

In this case, the `album` property is optional, as well as `nullable`. If `album` was set to `null`, then we would understand that the song doesn't belong to any album at all. If, however, it didn't come in our payload (it was `undefined`), then this could either mean it's not part of an album or that we just do not know the name of the album to which the song belongs. 

Depending on our data source, though, **we don't always have the means to differentiate between these two scenarios**. There is a chance the user has data coming from a source (such as a SQL database) that can return `null` values for properties that are not required. In those cases, `null` can represent both an explicit empty value or the lack of information. 

**The new option `notRequiredAsNullable` can be useful in APIs using a data source that can potentially return `null` values for all optional fields**, preventing the user to explicitly add `null` as a data type when defining optional properties in their schemas. Instead, the library will assume all non-required properties to be `nullable` by default. In the example above, this would translate to both `artist` and `album` being able to receive `null` values. 

This change is closely related to [this issue](https://github.com/BRIKEV/express-oas-validator/issues/10) in the [`
express-oas-validator`](https://github.com/BRIKEV/express-oas-validator) library, where `null` values can trigger validation errors when the `nullable` flag has not been enabled for optional properties.

### Task list:

:cyclone: **Feature**
- [x] Update `Options` interface with new option.
- [x] When parsing properties, add `nullable` flag to all non-required properties if `notRequiredAsNullable` is enabled.

:heavy_check_mark: **Test**
- [x] Create new test to check that `nullable` is set to `true` for all optional fields when `notRequiredAsNullable` is enabled.

:arrows_counterclockwise: **Refactor**
- [x] Create new file with default settings that can be later overriden by user's config.

:memo: **Documentation**
- [x] Update config object example in `README.md` with the new option.
- [x] Create example of `notRequiredAsNullable` usage in `nullableFields` folder.


bri06: LGTM 

Patch:
diff --git a/README.md b/README.md
index bb3d3de..21fa083 100644
--- a/README.md
+++ b/README.md
@@ -45,6 +45,7 @@ const options = {
   exposeSwaggerUI: true, // Expose OpenAPI UI. Default true
   exposeApiDocs: false, // Expose Open API JSON Docs documentation in `apiDocsPath` path. Default false.
   apiDocsPath: '/v3/api-docs', // Open API JSON Docs endpoint. Default value '/v3/api-docs'.
+  notRequiredAsNullable: false, // Set non-required fields as nullable by default
 };
 
 const app = express();
diff --git a/config/default.js b/config/default.js
new file mode 100644
index 0000000..9432c45
--- /dev/null
+++ b/config/default.js
@@ -0,0 +1,18 @@
+const expressJSDocSwaggerOptions = {
+  // Absolute path of the application
+  baseDir: __dirname,
+  // Glob pattern to find your jsdoc files (multiple patterns can be added in an array)
+  filesPattern: './**/*.js',
+  // URL where SwaggerUI will be rendered
+  swaggerUIPath: '/api-docs',
+  // Expose OpenAPI UI
+  exposeSwaggerUI: true,
+  // Expose Open API JSON Docs documentation in `apiDocsPath` path.
+  exposeApiDocs: false,
+  // Open API JSON Docs endpoint.
+  apiDocsPath: '/v3/api-docs',
+  // Set non-required fields as nullable by default
+  notRequiredAsNullable: false,
+};
+
+module.exports = expressJSDocSwaggerOptions;
diff --git a/examples/nullableFields/index.js b/examples/nullableFields/explicitNullable.js
similarity index 94%
rename from examples/nullableFields/index.js
rename to examples/nullableFields/explicitNullable.js
index ba925ca..0023169 100644
--- a/examples/nullableFields/index.js
+++ b/examples/nullableFields/explicitNullable.js
@@ -7,6 +7,7 @@ const options = {
   info: {
     version: '1.0.0',
     title: 'Albums store',
+    description: 'Example where we define which specific properties are nullable',
     license: {
       name: 'MIT',
     },
diff --git a/examples/nullableFields/nullableByDefault.js b/examples/nullableFields/nullableByDefault.js
new file mode 100644
index 0000000..759a739
--- /dev/null
+++ b/examples/nullableFields/nullableByDefault.js
@@ -0,0 +1,41 @@
+const express = require('express');
+
+const logger = require('../utils/logger');
+const expressJSDocSwagger = require('../..');
+
+const options = {
+  info: {
+    version: '1.0.0',
+    title: 'Albums store',
+    description: 'Example where non-required fields are nullable by default',
+    license: {
+      name: 'MIT',
+    },
+  },
+  filesPattern: './index.js',
+  baseDir: __dirname,
+  notRequiredAsNullable: true,
+};
+
+const app = express();
+const port = 3000;
+
+expressJSDocSwagger(app)(options);
+
+/**
+ * @typedef {object} Song
+ * @property {number} id.required - Id
+ * @property {string} title.required - The title
+ * @property {string} artist - The artist
+ * @property {integer} year - The year - int64
+ */
+
+/**
+ * POST /api/v1/songs
+ * @summary Adds a new song
+ * @param {Song} request.body.required - Song to add
+ * @return {string} 200 - Success message
+ */
+app.post('/api/v1/songs', (req, res) => res.send('You saved a song!'));
+
+app.listen(port, () => logger.info(`Example app listening at http://localhost:${port}`));
diff --git a/examples/package.json b/examples/package.json
index 28c4db4..464d252 100644
--- a/examples/package.json
+++ b/examples/package.json
@@ -32,7 +32,8 @@
     "eventEmitter": "node eventEmitter/index.js",
     "eventEmitter:multipleInstance": "node eventEmitter/multipleInstance.js",
     "merge": "node merge/simple.js",
-    "nullableFields": "node nullableFields/index.js"
+    "nullableFields:explicit": "node nullableFields/explicitNullable.js",
+    "nullableFields:byDefault": "node nullableFields/nullableByDefault.js"
   },
   "keywords": [],
   "author": "",
diff --git a/index.d.ts b/index.d.ts
index 1d6dbd3..5c220f9 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -55,6 +55,7 @@ interface Options {
   exposeApiDocs?: boolean;
   apiDocsPath?: string;
   swaggerUiOptions?: SwaggerUiOptions;
+  notRequiredAsNullable?: boolean;
 }
 
 type UserSwagger = Record<string, unknown>;
diff --git a/index.js b/index.js
index e5c6c52..359955b 100644
--- a/index.js
+++ b/index.js
@@ -1,18 +1,20 @@
 const swaggerUi = require('swagger-ui-express');
 const merge = require('merge');
+
+const defaultOptions = require('./config/default');
 const processSwagger = require('./processSwagger');
 const swaggerEvents = require('./swaggerEvents');
 
-const DEFAULT_SWAGGERUI_URL = '/api-docs';
-const DEFAULT_EXPOSE_SWAGGERUI = true;
-const DEFAULT_APIDOCS_URL = '/v3/api-docs';
-const DEFAULT_EXPOSE_APIDOCS = false;
-
-const expressJSDocSwagger = app => (options = {}, userSwagger = {}) => {
+const expressJSDocSwagger = app => (userOptions = {}, userSwagger = {}) => {
   const events = swaggerEvents();
   const { instance } = events;
   let swaggerObject = {};
 
+  const options = {
+    ...defaultOptions,
+    ...userOptions,
+  };
+
   processSwagger(options, events.processFile)
     .then(result => {
       swaggerObject = {
@@ -24,8 +26,8 @@ const expressJSDocSwagger = app => (options = {}, userSwagger = {}) => {
     })
     .catch(events.error);
 
-  if (options.exposeSwaggerUI || DEFAULT_EXPOSE_SWAGGERUI) {
-    app.use(options.swaggerUIPath || DEFAULT_SWAGGERUI_URL, (req, res, next) => {
+  if (options.exposeSwaggerUI) {
+    app.use(options.swaggerUIPath, (req, res, next) => {
       swaggerObject = {
         ...swaggerObject,
         host: req.get('host'),
@@ -35,8 +37,8 @@ const expressJSDocSwagger = app => (options = {}, userSwagger = {}) => {
     }, swaggerUi.serve, swaggerUi.setup(undefined, options.swaggerUiOptions));
   }
 
-  if (options.exposeApiDocs || DEFAULT_EXPOSE_APIDOCS) {
-    app.get(options.apiDocsPath || DEFAULT_APIDOCS_URL, (req, res) => {
+  if (options.exposeApiDocs) {
+    app.get(options.apiDocsPath, (req, res) => {
       res.json(swaggerObject);
     });
   }
diff --git a/processSwagger.js b/processSwagger.js
index c0fcbfb..aafa8dd 100644
--- a/processSwagger.js
+++ b/processSwagger.js
@@ -32,7 +32,7 @@ const processSwagger = (options, logger = defaultLogger) => {
     .then(data => {
       swaggerObject = getPaths(swaggerObject, data);
       logger({ entity: 'paths', swaggerObject });
-      swaggerObject = getComponents(swaggerObject, data);
+      swaggerObject = getComponents(swaggerObject, data, options);
       logger({ entity: 'components', swaggerObject });
       swaggerObject = getTags(swaggerObject, data);
       logger({ entity: 'tags', swaggerObject });
diff --git a/transforms/components/index.js b/transforms/components/index.js
index ac88eab..659f9e4 100644
--- a/transforms/components/index.js
+++ b/transforms/components/index.js
@@ -27,10 +27,11 @@ const addRefSchema = (typeName, applications, elements) => {
   return {};
 };
 
-const formatProperties = properties => {
+const formatProperties = (properties, options = {}) => {
   if (!properties || !Array.isArray(properties)) return {};
   return properties.reduce((acum, property) => {
     const name = getPropertyName(property);
+    const isRequired = property.name.includes(REQUIRED);
     const {
       name: typeName, applications, expression, elements,
     } = property.type;
@@ -46,6 +47,11 @@ const formatProperties = properties => {
         ...addRefSchema(typeName, applications, elements),
         ...(format ? { format } : {}),
         ...addEnumValues(enumValues),
+
+        // Add nullable to non-required fields if option to do that is enabled
+        ...(options.notRequiredAsNullable && !isRequired ? {
+          nullable: true,
+        } : {}),
       },
     };
   }, {});
@@ -57,7 +63,7 @@ const getRequiredProperties = properties => (
 
 const formatRequiredProperties = requiredProperties => requiredProperties.map(getPropertyName);
 
-const parseSchema = schema => {
+const parseSchema = (schema, options = {}) => {
   const typedef = getTagInfo(schema.tags, 'typedef');
   const propertyValues = getTagsInfo(schema.tags, 'property');
   const requiredProperties = getRequiredProperties(propertyValues);
@@ -73,15 +79,15 @@ const parseSchema = schema => {
         required: formatRequiredProperties(requiredProperties),
       } : {}),
       type: 'object',
-      properties: formatProperties(propertyValues),
+      properties: formatProperties(propertyValues, options),
     },
   };
 };
 
-const parseComponents = (swaggerObject = {}, components = []) => {
+const parseComponents = (swaggerObject = {}, components = [], options = {}) => {
   if (!components || !Array.isArray(components)) return { components: { schemas: {} } };
   const componentSchema = components.reduce((acum, item) => ({
-    ...acum, ...parseSchema(item),
+    ...acum, ...parseSchema(item, options),
   }), {});
   return {
     ...swaggerObject,
diff --git a/transforms/utils/combineSchema.js b/transforms/utils/combineSchema.js
index 7182bf6..e978f73 100644
--- a/transforms/utils/combineSchema.js
+++ b/transforms/utils/combineSchema.js
@@ -5,21 +5,21 @@ const VALID_TYPES = ['oneOf', 'anyOf', 'allOf'];
 
 /**
  *  This method checks the first item of the data type list to validate if
- * it contains any of the keywords representing the different schema types
+ * it contains any of the keywords representing the different union types
  * in Swagger: 'oneOf', 'anyOf' or 'allOf'.
  *
  * @param {object[]} elements - List of data types for the property
- * @returns {string|null} Name of the schema type (if any)
+ * @returns {string|null} Name of the union type (if any)
  */
-const getSchemaType = elements => {
-  const schemaType = elements[0].name;
+const getUnionType = elements => {
+  const unionType = elements[0].name;
 
-  if (!VALID_TYPES.includes(schemaType)) {
-    errorMessage(`SchemaType ${schemaType} invalid, it should be one of these ${VALID_TYPES.join(', ')}`);
+  if (!VALID_TYPES.includes(unionType)) {
+    errorMessage(`unionType ${unionType} invalid, it should be one of these ${VALID_TYPES.join(', ')}`);
     return null;
   }
 
-  return schemaType;
+  return unionType;
 };
 
 /**
@@ -45,15 +45,15 @@ const combineSchema = elements => {
     schema = { nullable: true };
   }
 
-  const schemaType = getSchemaType(elements);
-  const types = !schemaType ? elements : elements.slice(1);
+  const unionType = getUnionType(elements);
+  const types = !unionType ? elements : elements.slice(1);
 
-  // If there are multiple types in the list, wrap them into a schema type
+  // If there are multiple types in the list, wrap them into a union type
   // ('oneOf' will be used by default if none is specified)
-  if (types.length > 1 || schemaType === 'allOf') {
+  if (types.length > 1 || unionType === 'allOf') {
     schema = {
       ...schema,
-      [schemaType || 'oneOf']: types.map(type => refSchema(type)),
+      [unionType || 'oneOf']: types.map(type => refSchema(type)),
     };
   } else {
     // If there's only a type in the list, don't wrap it in 'oneOf' or 'anyOf' blocks


Test patch:
diff --git a/test/transforms/components/index.test.js b/test/transforms/components/index.test.js
index 825bd6e..9a1bd15 100644
--- a/test/transforms/components/index.test.js
+++ b/test/transforms/components/index.test.js
@@ -712,4 +712,49 @@ describe('parseComponents method', () => {
     const result = parseComponents({}, parsedJSDocs);
     expect(result).toEqual(expected);
   });
+
+  it('Should parse jsdoc component spec with optional properties nullable by default', () => {
+    const jsdocInput = [`
+      /**
+       * A song
+       * @typedef {object} Song
+       * @property {string} title.required - The title
+       * @property {string} artist - The artist
+       * @property {number} year - The year - int64
+       */
+    `];
+    const expected = {
+      components: {
+        schemas: {
+          Song: {
+            type: 'object',
+            required: [
+              'title',
+            ],
+            description: 'A song',
+            properties: {
+              title: {
+                type: 'string',
+                description: 'The title',
+              },
+              artist: {
+                type: 'string',
+                description: 'The artist',
+                nullable: true,
+              },
+              year: {
+                type: 'number',
+                description: 'The year',
+                format: 'int64',
+                nullable: true,
+              },
+            },
+          },
+        },
+      },
+    };
+    const parsedJSDocs = jsdocInfo()(jsdocInput);
+    const result = parseComponents({}, parsedJSDocs, { notRequiredAsNullable: true });
+    expect(result).toEqual(expected);
+  });
 });