Nullable in query parameters not correct

Fixes cyclosproject#293
FlorianGrimm · Nov 26, 2023 · 208f69d · 208f69d
1 parent d141c24
commit 208f69d
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 40 deletions.
diff --git a/lib/gen-utils.ts b/lib/gen-utils.ts
@@ -171,28 +171,17 @@ export function escapeId(name: string) {
 }
 
 /**
- * Returns the TypeScript type for the given type and options
+ * Appends | null to the given type
  */
-export function tsType(schemaOrRef: SchemaOrRef | undefined, options: Options, openApi: OpenAPIObject, container?: Model): string {
-  if (!schemaOrRef) {
-    // No schema
-    return 'any';
-  }
-  if (schemaOrRef.$ref) {
-    // A reference
-    const resolved = resolveRef(openApi, schemaOrRef.$ref);
-    const nullable = !!(resolved && (resolved as SchemaObject).nullable);
-    const prefix = nullable ? 'null | ' : '';
-    const name = simpleName(schemaOrRef.$ref);
-    if (container && container.name === name) {
-      // When referencing the same container, use its type name
-      return prefix + container.typeName;
-    } else {
-      return prefix + qualifiedName(name, options);
-    }
+function maybeAppendNull(type: string, nullable: boolean) {
+  if (` ${type} `.includes('null') || !nullable) {
+    // The type itself already includes null
+    return type;
   }
-  const schema = schemaOrRef as SchemaObject;
+  return (type.includes(' ') ? `(${type})` : type) + (nullable ? ' | null' : '');
+}
 
+function rawTsType(schema: SchemaObject, options: Options, openApi: OpenAPIObject, container?: Model): string {
   // An union of types
   const union = schema.oneOf || schema.anyOf || [];
   if (union.length > 0) {
@@ -208,10 +197,7 @@ export function tsType(schemaOrRef: SchemaOrRef | undefined, options: Options, o
   // An array
   if (type === 'array' || schema.items) {
     const items = schema.items || {};
-    let itemsType = tsType(items, options, openApi, container);
-    if ((items as any)['nullable'] && !itemsType.includes(' | null')) {
-      itemsType += ' | null';
-    }
+    const itemsType = tsType(items, options, openApi, container);
     return `Array<${itemsType}>`;
   }
 
@@ -248,10 +234,7 @@ export function tsType(schemaOrRef: SchemaOrRef | undefined, options: Options, o
       if (!propRequired) {
         result += '?';
       }
-      let propertyType = tsType(property, options, openApi, container);
-      if ((property as SchemaObject).nullable) {
-        propertyType = `${propertyType} | null`;
-      }
+      const propertyType = tsType(property, options, openApi, container);
       result += `: ${propertyType};\n`;
     }
     if (schema.additionalProperties) {
@@ -281,10 +264,33 @@ export function tsType(schemaOrRef: SchemaOrRef | undefined, options: Options, o
     return 'Blob';
   }
 
-  // A simple type
+  // A simple type (integer doesn't exist as type in JS, use number instead)
   return type === 'integer' ? 'number' : type;
 }
 
+/**
+ * Returns the TypeScript type for the given type and options
+ */
+export function tsType(schemaOrRef: SchemaOrRef | undefined, options: Options, openApi: OpenAPIObject, container?: Model): string {
+  if (!schemaOrRef) {
+    // No schema
+    return 'any';
+  }
+
+  if (schemaOrRef.$ref) {
+    // A reference
+    const resolved = resolveRef(openApi, schemaOrRef.$ref) as SchemaObject;
+    const name = simpleName(schemaOrRef.$ref);
+    // When referencing the same container, use its type name
+    return maybeAppendNull((container && container.name === name) ? container.typeName : qualifiedName(name, options), !!resolved.nullable);
+  }
+
+  // Resolve the actual type (maybe nullable)
+  const schema = schemaOrRef as SchemaObject;
+  const type = rawTsType(schema, options, openApi, container);
+  return maybeAppendNull(type, !!schema.nullable);
+}
+
 /**
  * Resolves a reference
  * @param ref The reference name, such as #/components/schemas/Name, or just Name

diff --git a/lib/property.ts b/lib/property.ts
@@ -21,9 +21,6 @@ export class Property {
     openApi: OpenAPIObject) {
 
     this.type = tsType(this.schema, options, openApi, model);
-    if ((schema as SchemaObject)?.nullable && !this.type.startsWith('null | ')) {
-      this.type = 'null | ' + this.type;
-    }
     this.identifier = escapeId(this.name);
     const description = (schema as SchemaObject).description || '';
     this.tsComments = tsComments(description, 1, (schema as SchemaObject).deprecated);

diff --git a/test/all-operations.json b/test/all-operations.json
@@ -79,8 +79,10 @@
           {
             "name": "get2",
             "description": "GET param 2",
+            "required": true,
             "schema": {
-              "type": "integer"
+              "type": "integer",
+              "nullable": true
             }
           },
           {

diff --git a/test/all-operations.spec.ts b/test/all-operations.spec.ts
@@ -214,7 +214,7 @@ describe('Generation tests using all-operations.json', () => {
     expect(params[2].type).toBe('RefString');
     expect(params[2].in).toBe('query');
     expect(params[3].name).toBe('get2');
-    expect(params[3].type).toBe('number');
+    expect(params[3].type).toBe('number | null');
     expect(params[3].in).toBe('query');
     expect(params[4].name).toBe('get3');
     expect(params[4].var).toBe('get3');

diff --git a/test/all-types.spec.ts b/test/all-types.spec.ts
@@ -97,7 +97,7 @@ describe('Generation tests using all-types.json', () => {
       expect(decl.name).toBe('NullableObject');
       // There's no support for additional properties in typescript-parser. Check as text.
       const text = ts.substring(decl.start || 0, decl.end || ts.length);
-      expect(text).toContain('= {\n\'name\'?: string;\n}');
+      expect(text).toContain('= ({\n\'name\'?: string;\n})');
       done();
     });
   });
@@ -285,14 +285,14 @@ describe('Generation tests using all-types.json', () => {
       expect(decl.name).toBe('AdditionalProperties');
       expect(decl.properties.length).toBe(3);
       expect(decl.properties[0].name).toBe('age');
-      expect(decl.properties[0].type).toBe('null | number');
+      expect(decl.properties[0].type).toBe('number | null');
       expect(decl.properties[1].name).toBe('description');
       expect(decl.properties[1].type).toBe('string');
       expect(decl.properties[2].name).toBe('name');
       expect(decl.properties[2].type).toBe('string');
       expect(decl.properties[2].isOptional).toBeFalse();
       const text = ts.substring(decl.start || 0, decl.end || ts.length);
-      expect(text).toContain('[key: string]: ABRefObject | null | number | string | undefined;');
+      expect(text).toContain('[key: string]: ABRefObject | number | null | string | undefined;');
       done();
     });
   });
@@ -310,13 +310,13 @@ describe('Generation tests using all-types.json', () => {
       expect(decl.name).toBe('Nullables');
       expect(decl.properties.length).toBe(3);
       expect(decl.properties[0].name).toBe('inlinedNullableObject');
-      expect(decl.properties[0].type).withContext('inlinedNullableObject property').toBe('null | {\n\'someProperty\': string;\n}');
+      expect(decl.properties[0].type).withContext('inlinedNullableObject property').toBe('({\n\'someProperty\': string;\n}) | null');
       expect(decl.properties[0].isOptional).toBeFalse();
       expect(decl.properties[1].name).toBe('nullableObject');
-      expect(decl.properties[1].type).withContext('nullableObject property').toBe('null | NullableObject');
+      expect(decl.properties[1].type).withContext('nullableObject property').toBe('NullableObject | null');
       expect(decl.properties[1].isOptional).toBeFalse();
       expect(decl.properties[2].name).toBe('withNullableProperty');
-      expect(decl.properties[2].type).withContext('withNullableProperty property').toBe('{\n\'someProperty\': null | NullableObject;\n}');
+      expect(decl.properties[2].type).withContext('withNullableProperty property').toBe('{\n\'someProperty\': NullableObject | null;\n}');
       expect(decl.properties[2].isOptional).toBeFalse();
       done();
     });
@@ -410,7 +410,7 @@ describe('Generation tests using all-types.json', () => {
       assertProperty('booleanProp', 'boolean');
       assertProperty('anyProp', 'any');
 
-      assertProperty('nullableObject', 'null | NullableObject');
+      assertProperty('nullableObject', 'NullableObject | null');
       assertProperty('refEnumProp', 'RefEnum', true);
       assertProperty('refObjectProp', 'ABRefObject', true);
       assertProperty('unionProp', 'Union');