Merge pull request #3544 from AliRaZa1121/fix/3519-nullable-optional-enum

kamilmysliwiec · web-flow · commit c1a062f8fe81 · 2025-10-16T10:10:46.000+02:00
fix(plugin): correct enum+nullable handling for `Enum | null` (and ar…
diff --git a/lib/decorators/helpers.ts b/lib/decorators/helpers.ts
@@ -1,8 +1,8 @@
+import { METHOD_METADATA } from '@nestjs/common/constants';
+import { isConstructor } from '@nestjs/common/utils/shared.utils';
 import { isArray, isUndefined, negate, pickBy } from 'lodash';
 import { DECORATORS } from '../constants';
 import { METADATA_FACTORY_NAME } from '../plugin/plugin-constants';
-import { METHOD_METADATA } from '@nestjs/common/constants';
-import { isConstructor } from '@nestjs/common/utils/shared.utils';
 
 export function createMethodDecorator<T = any>(
   metakey: string,
diff --git a/lib/plugin/visitors/model-class.visitor.ts b/lib/plugin/visitors/model-class.visitor.ts
@@ -73,36 +73,36 @@ export class ModelClassVisitor extends AbstractFileVisitor {
 
     const propertyNodeVisitorFactory =
       (metadata: ClassMetadata) =>
-      (node: ts.Node): ts.Node => {
-        const visit = () => {
-          if (ts.isPropertyDeclaration(node)) {
-            this.visitPropertyNodeDeclaration(
-              node,
-              ctx,
-              typeChecker,
-              options,
-              sourceFile,
-              metadata
-            );
-          } else if (
-            options.parameterProperties &&
-            ts.isConstructorDeclaration(node)
-          ) {
-            this.visitConstructorDeclarationNode(
-              node,
-              typeChecker,
-              options,
-              sourceFile,
-              metadata
-            );
+        (node: ts.Node): ts.Node => {
+          const visit = () => {
+            if (ts.isPropertyDeclaration(node)) {
+              this.visitPropertyNodeDeclaration(
+                node,
+                ctx,
+                typeChecker,
+                options,
+                sourceFile,
+                metadata
+              );
+            } else if (
+              options.parameterProperties &&
+              ts.isConstructorDeclaration(node)
+            ) {
+              this.visitConstructorDeclarationNode(
+                node,
+                typeChecker,
+                options,
+                sourceFile,
+                metadata
+              );
+            }
+            return node;
+          };
+          const visitedNode = visit();
+          if (!options.readonly) {
+            return visitedNode;
           }
-          return node;
         };
-        const visitedNode = visit();
-        if (!options.readonly) {
-          return visitedNode;
-        }
-      };
 
     const visitClassNode = (node: ts.Node): ts.Node => {
       if (ts.isClassDeclaration(node)) {
@@ -347,10 +347,10 @@ export class ModelClassVisitor extends AbstractFileVisitor {
     const properties = [
       ...existingProperties,
       !hasPropertyKey('required', existingProperties) &&
-        factory.createPropertyAssignment(
-          'required',
-          createBooleanLiteral(factory, isRequired)
-        ),
+      factory.createPropertyAssignment(
+        'required',
+        createBooleanLiteral(factory, isRequired)
+      ),
       ...this.createTypePropertyAssignments(
         factory,
         node.type,
@@ -397,6 +397,12 @@ export class ModelClassVisitor extends AbstractFileVisitor {
    * Returns an array with 0..2 "ts.PropertyAssignment"s.
    * The first one is the "type" property assignment, the second one is the "nullable" property assignment.
    * When type cannot be determined, an empty array is returned.
+   *
+   * Special handling:
+   * - For unions like `Enum | null` or `Enum | null | undefined` (and their array forms),
+   *   we DO NOT emit a lazy `type: () => ...`. The enum metadata is emitted elsewhere
+   *   by createEnumPropertyAssignment(). Here we only add `nullable: true` when needed.
+   *   This avoids circular dependency errors for optional+nullable enum properties.
    */
   private createTypePropertyAssignments(
     factory: ts.NodeFactory,
@@ -412,6 +418,7 @@ export class ModelClassVisitor extends AbstractFileVisitor {
     }
 
     if (node) {
+      // Array of inline object literal
       if (ts.isArrayTypeNode(node) && ts.isTypeLiteralNode(node.elementType)) {
         const initializer = this.createInitializerForArrayLiteralTypeNode(
           node,
@@ -422,7 +429,10 @@ export class ModelClassVisitor extends AbstractFileVisitor {
           options
         );
         return [factory.createPropertyAssignment(key, initializer)];
-      } else if (ts.isTypeLiteralNode(node)) {
+      }
+
+      // Inline object literal
+      if (ts.isTypeLiteralNode(node)) {
         const initializer = this.createInitializerForTypeLiteralNode(
           node,
           factory,
@@ -432,17 +442,59 @@ export class ModelClassVisitor extends AbstractFileVisitor {
           options
         );
         return [factory.createPropertyAssignment(key, initializer)];
-      } else if (ts.isUnionTypeNode(node)) {
+      }
+
+      // Union types (where nullable/undefined might be part of the union)
+      if (ts.isUnionTypeNode(node)) {
         const { nullableType, isNullable } = this.isNullableUnion(node);
-        const remainingTypes = node.types.filter(
-          (item) => item !== nullableType
-        );
+        const remainingTypes = node.types.filter((t) => t !== nullableType);
 
-        // TODO: When we have more than 1 type left, we could use "oneOf"
+        // If exactly one non-nullish type remains, we can reason about it.
         if (remainingTypes.length === 1) {
+          const nonNullishNode = remainingTypes[0];
+
+          // Detect if the non-nullish side is (or resolves to) an enum (including array of enum).
+          // If yes, DO NOT emit a lazy `type`; only emit { nullable: true } if needed.
+          const resolved = typeChecker.getTypeAtLocation(nonNullishNode);
+          let candidateType = resolved;
+
+          // If it's an array (e.g., Status[] | null), drill into the element type
+          const arrayTuple = extractTypeArgumentIfArray(candidateType);
+          if (arrayTuple) {
+            candidateType = arrayTuple.type;
+          }
+
+          let isEnumType = false;
+          if (candidateType) {
+            if (isEnum(candidateType)) {
+              isEnumType = true;
+            } else {
+              // Handle auto-generated enum unions (string literal unions that represent an enum)
+              const maybeEnum = isAutoGeneratedEnumUnion(candidateType, typeChecker);
+              if (maybeEnum) {
+                isEnumType = true;
+              }
+            }
+          }
+
+          if (isEnumType) {
+            // For enums, skip returning a "type" property (avoid lazy resolver/circular deps).
+            // Only append { nullable: true } if the union contained `null`.
+            // The enum metadata will be added by createEnumPropertyAssignment().
+            return isNullable
+              ? [
+                factory.createPropertyAssignment(
+                  'nullable',
+                  createBooleanLiteral(factory, true)
+                )
+              ]
+              : [];
+          }
+
+          // Not an enum: keep existing behavior (recurse into the remaining node)
           const propertyAssignments = this.createTypePropertyAssignments(
             factory,
-            remainingTypes[0],
+            nonNullishNode,
             typeChecker,
             existingProperties,
             hostFilename,
@@ -459,10 +511,13 @@ export class ModelClassVisitor extends AbstractFileVisitor {
             )
           ];
         }
+        // >1 remaining non-nullish types: fall through (no special handling here).
+        // (Potential future: oneOf support)
       }
     }
 
-    const type = typeChecker.getTypeAtLocation(node);
+    // Fallback: emit a lazy `type: () => Identifier` when we can resolve a referenceable type name
+    const type = typeChecker.getTypeAtLocation(node as any);
     if (!type) {
       return [];
     }
@@ -492,6 +547,8 @@ export class ModelClassVisitor extends AbstractFileVisitor {
     return [factory.createPropertyAssignment(key, initializer)];
   }
 
+
+
   createInitializerForArrayLiteralTypeNode(
     node: ts.ArrayTypeNode,
     factory: ts.NodeFactory,
@@ -592,10 +649,43 @@ export class ModelClassVisitor extends AbstractFileVisitor {
     if (hasPropertyKey(key, existingProperties)) {
       return undefined;
     }
-    let type = typeChecker.getTypeAtLocation(node);
+    // Prefer using the explicit TypeNode when available to get the declared type
+    // (this helps in cases like optional/nullable unions where the TypeNode reflects the union)
+    let type: ts.Type | undefined;
+    try {
+      if ((node as any).type) {
+        type = typeChecker.getTypeFromTypeNode((node as any).type as ts.TypeNode);
+      }
+    } catch (e) {
+      // fallthrough to getTypeAtLocation
+    }
+    if (!type) {
+      type = typeChecker.getTypeAtLocation(node as any);
+    }
     if (!type) {
       return undefined;
     }
+
+    if ((type.flags & ts.TypeFlags.Union) !== 0) {
+      const union = type as ts.UnionOrIntersectionType;
+      const nonNullish = union.types.filter(
+        (t) => (t.flags & (ts.TypeFlags.Null | ts.TypeFlags.Undefined)) === 0
+      );
+      if (nonNullish.length === 1) {
+        type = nonNullish[0];
+      }
+    }
+
+    // Also handle cases where TypeScript emits an auto-generated union like `T | undefined`
+    // (strict mode). In that case, pick the non-undefined member so enum detection works.
+    if (isAutoGeneratedTypeUnion(type)) {
+      const types = (type as ts.UnionOrIntersectionType).types;
+      const nonUndefined = types.find((t: any) => t.intrinsicName !== 'undefined');
+      if (nonUndefined) {
+        type = nonUndefined as ts.Type;
+      }
+    }
+
     if (isAutoGeneratedTypeUnion(type)) {
       const types = (type as ts.UnionOrIntersectionType).types;
       type = types[types.length - 1];
diff --git a/test/plugin/fixtures/nullable.dto.ts b/test/plugin/fixtures/nullable.dto.ts
@@ -9,22 +9,24 @@ enum Status {
 }
 
 export class NullableDto {
-  @ApiProperty()
-  stringValue: string | null;
-  @ApiProperty()
-  stringArr: string[] | null;
-  @ApiProperty()
-  optionalString?: string;
-  @ApiProperty()
-  undefinedString: string | undefined;
-  @ApiProperty()
-  nullableEnumValue: OneValueEnum | null;
-  @ApiProperty()
-  optionalEnumValue?: OneValueEnum;
-  @ApiProperty()
-  undefinedEnumValue: OneValueEnum | undefined;
-  @ApiProperty()
-  enumValue: Status | null;
+    @ApiProperty()
+    stringValue: string | null;
+    @ApiProperty()
+    stringArr: string[] | null;
+    @ApiProperty()
+    optionalString?: string;
+    @ApiProperty()
+    undefinedString: string | undefined;
+    @ApiProperty()
+    nullableEnumValue: OneValueEnum | null;
+    @ApiProperty()
+    optionalEnumValue?: OneValueEnum;
+    @ApiProperty()
+    undefinedEnumValue: OneValueEnum | undefined;
+    @ApiProperty()
+    enumValue: Status | null;
+    @ApiProperty()
+    optionalNullableEnumValue?: Status | null;
 }
 `;
 
@@ -40,7 +42,7 @@ var Status;
 })(Status || (Status = {}));
 export class NullableDto {
     static _OPENAPI_METADATA_FACTORY() {
-        return { stringValue: { required: true, type: () => String, nullable: true }, stringArr: { required: true, type: () => [String], nullable: true }, optionalString: { required: false, type: () => String }, undefinedString: { required: true, type: () => String }, nullableEnumValue: { required: true, nullable: true, enum: OneValueEnum }, optionalEnumValue: { required: false, enum: OneValueEnum }, undefinedEnumValue: { required: true, enum: OneValueEnum }, enumValue: { required: true, nullable: true, enum: Status } };
+        return { stringValue: { required: true, type: () => String, nullable: true }, stringArr: { required: true, type: () => [String], nullable: true }, optionalString: { required: false, type: () => String }, undefinedString: { required: true, type: () => String }, nullableEnumValue: { required: true, nullable: true, enum: OneValueEnum }, optionalEnumValue: { required: false, enum: OneValueEnum }, undefinedEnumValue: { required: true, enum: OneValueEnum }, enumValue: { required: true, nullable: true, enum: Status }, optionalNullableEnumValue: { required: false, nullable: true, enum: Status } };
     }
 }
 __decorate([
@@ -67,4 +69,7 @@ __decorate([
 __decorate([
     ApiProperty()
 ], NullableDto.prototype, "enumValue", void 0);
+__decorate([
+    ApiProperty()
+], NullableDto.prototype, "optionalNullableEnumValue", void 0);
 `;
