YousefED / typescript-json-schema

Generate json-schema from your Typescript sources
BSD 3-Clause "New" or "Revised" License
3.17k stars 323 forks source link

@items not merging correctly #358

Open robertmassaioli opened 4 years ago

robertmassaioli commented 4 years ago

I have this code:

export interface AppHostManifest {
  /**
   * The ASAP Issuers.
   *
   * @uniqueItems true
   * @minItems 1
   * @maxItems 50
   * @items.type string
   * @items.minLength 2
   * @items.maxLength 100
   */
  ['asap-issuers']?: string[];
}

I run the following command:

typescript-json-schema src/models/AppHostManifestTypes.ts AppHostManifest --refs --aliasRefs --titles --noExtraProps --required --strictNullChecks --out=src/models/AppHostManifestSchema.json

I get the following output:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "additionalProperties": false,
    "properties": {
        "asap-issuers": {
            "description": "The ASAP Issuers that are allowed to access the CaaS services.",
            "items": {
                "type": "string"
            },
            "maxItems": 50,
            "minItems": 1,
            "title": "asap-issuers",
            "type": "array",
            "uniqueItems": true
        }
    },
    "type": "object"
}

I expected the following output:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "additionalProperties": false,
    "properties": {
        "asap-issuers": {
            "description": "The ASAP Issuers that are allowed to access the CaaS services.",
            "items": {
                "type": "string",
                "minLength": 2,
                "maxLength": 100
            },
            "maxItems": 50,
            "minItems": 1,
            "title": "asap-issuers",
            "type": "array",
            "uniqueItems": true
        }
    },
    "type": "object"
}

I think that this is a bug. When I change the definition to Array<string> then I get the following output and you can see the minLength and maxLength be added in to the items definition:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "additionalProperties": false,
    "definitions": {
        "Array": {
            "items": {
                "type": "string"
            },
            "title": "Array",
            "type": "array"
        }
    },
    "properties": {
        "asap-issuers": {
            "$ref": "#/definitions/Array",
            "description": "The ASAP Issuers that are allowed to access the CaaS services.",
            "items": {
                "maxLength": 100,
                "minLength": 2,
                "type": "string"
            },
            "maxItems": 50,
            "minItems": 1,
            "title": "asap-issuers",
            "uniqueItems": true
        }
    },
    "type": "object"
}

But it still looks ugly. Not sure what is going wrong with items generation here. https://github.com/YousefED/typescript-json-schema/tree/master/test/programs/annotation-items suggests that it should "just work".

robertmassaioli commented 4 years ago

Well, this is confusing, checking out master and making this change:

Screen Shot 2020-08-11 at 8 57 13 am

And making this change to the output schema of the test:

Screen Shot 2020-08-11 at 8 57 53 am

Results in the test passing:

Screen Shot 2020-08-11 at 8 57 58 am

This seems to suggest that, on master, the code works as expected. Weird. Maybe there are changes on master that have not been released yet?

Let's see, my projects yarn.lock file has:

typescript-json-schema@^0.42.0:
  version "0.42.0"

Which is, apparently, the latest version on NPM: https://www.npmjs.com/package/typescript-json-schema

Checking out v0.42.0 of the code with my changes to the test cases intact returns the following:

Screen Shot 2020-08-11 at 9 02 25 am

Okay, so it still works, so the version that I am using should generate the output that I expect. I'm going to investigate further but I'm not sure why this library works in one place and not in another. My current top candidates are:

robertmassaioli commented 4 years ago

Okay, upgrading my version of typescript to v3.9.7 in my branch checkout with modifications to the tests to include this case still resulted in a positive test passing.

I think that it might be safe to say that this is not Typescript version related. Hmm.

robertmassaioli commented 4 years ago

Screen Shot 2020-08-11 at 9 21 13 am

Okay, for the first time, I have reproduced the problem from the typescript-json-schema repository. I can't seem to reproduce it through the test cases, only the CLI tool. Interesting.

robertmassaioli commented 4 years ago

Okay, I was able to work around the issue by adding the @type parameter like so:

  /**
   * The ASAP Issuers that are allowed to access the CaaS services.
   *
   * @type array
   * @uniqueItems true
   * @minItems 1
   * @maxItems 50
   * @items.type string
   * @items.minLength 2
   * @items.maxLength 100
   */
  ['asap-issuers']?: string[];

But fundamentally I should not have to do this.

robertmassaioli commented 4 years ago

Okay, I've identified a patch that fixes my particular case but breaks a large number of tests:

diff --git a/typescript-json-schema.ts b/typescript-json-schema.ts
index d6ec4f5..6605451 100644
--- a/typescript-json-schema.ts
+++ b/typescript-json-schema.ts
@@ -151,6 +151,20 @@ function unique(arr: string[]): string[] {
     return r;
 }

+function mergeProperties(dest: any, source: any): void {
+    if (typeof dest === 'object' && typeof source === 'object') {
+        for (const k in source) {
+            if (Object.prototype.hasOwnProperty.call(source, k)) {
+                if (typeof source[k] === 'object') {
+                    mergeProperties(dest[k], source[k]);
+                } else {
+                    dest[k] = source[k];
+                }
+            }
+        }
+    }
+}
+
 /**
  * Try to parse a value and returns the string if it fails.
  */
@@ -755,11 +769,7 @@ export class JsonSchemaGenerator {
         }

         if (schemas.length === 1) {
-            for (const k in schemas[0]) {
-                if (schemas[0].hasOwnProperty(k)) {
-                    definition[k] = schemas[0][k];
-                }
-            }
+            mergeProperties(definition, schemas[0]);
         } else {
             definition[unionModifier] = schemas;
         }
@@ -799,11 +809,7 @@ export class JsonSchemaGenerator {
         }

         if (schemas.length === 1) {
-            for (const k in schemas[0]) {
-                if (schemas[0].hasOwnProperty(k)) {
-                    definition[k] = schemas[0][k];
-                }
-            }
+            mergeProperties(definition, schemas[0]);
         } else {
             definition.allOf = schemas;
         }
fxalgrain commented 2 years ago

The issue still here and but the @type array still working too. I have identify that the issue is coming when property is optionnal

This will works:

  /** 
   * @items.type string
   * @items.format email
   */
  emails2: string[];

This will not works:

  /**
   * @items.type string
   * @items.format email
   */
  emails2?: string[];

But you can workaround via

  /**
   * @type array
   * @items.type string
   * @items.format email
   */
  emails2?: string[];