Skip to content

Case where allOf is not included in compiled ts interface definition #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
darcyparker opened this issue Sep 29, 2016 · 14 comments
Closed
Labels

Comments

@darcyparker
Copy link
Contributor

darcyparker commented Sep 29, 2016

Have a look at: https://github.com/darcyparker/json-schema-to-typescript/blob/fe3f08f1d7eb4445619488974f9f89b2353b74c7/test/cases/allOf.ts

As a comparison, dtsgenerator creates this output:

export interface Itest {
    fooAndBar: {
        a: string;
        b: number;
    };
    foo: {
        a: string;
        b: number;
    };
    more: {
        a: string;
    };
}
declare namespace Itest {
    namespace Definitions {
        export interface Bar {
            a: string;
        }
        export interface Foo {
            a: string;
            b: number;
        }
    }
}

Notes:

  • ignore the interface name and namespace Itest...: dtgenerator uses the filename rather than the title of the schema.
  • I like json-schema-typescript's compiler's choice to use the definitions of Bar and Foo in Itest. but ignoring this, you can see it picks up the properties inside allOf that is a sibling of the top level properties. I believe this is correct behavior because ajv compile -s test.json -o test.js creates a validator that checks for more.
@darcyparker
Copy link
Contributor Author

I haven't tested, but a similar test case for anyOf should also be tested.

@bcherny
Copy link
Owner

bcherny commented Sep 29, 2016

What is expected output? Is it

1

export interface Foo {
  a: string;
  b: number;
}
export interface Bar {
  a: string;
}
export interface AllOf {
  fooAndBar: Foo & Bar;
  foo: Foo;
  allOf: {
    more?: Bar;
  };
}

Or

2

export interface Foo {
  a: string;
  b: number;
}
export interface Bar {
  a: string;
}
export interface AllOf {
  fooAndBar: Foo & Bar;
  foo: Foo;
}

Or something else?

@darcyparker
Copy link
Contributor Author

darcyparker commented Sep 29, 2016

I don't consider 2 a candidate.

Your candidate 1 is close I think.

Case 3 (is another which I mentioned in first commit... but I know this is wrong)

export interface Foo {
  a: string;
  b: number;
}
export interface Bar {
  a: string;
}
export interface AllOf {
  fooAndBar: Foo & Bar;
  foo: Foo;
  more: Bar;
}

Note: 3 agrees with dtsgenerator....

However, I played with: http://json-schema-validator.herokuapp.com/index.jsp and discovered some differences in my expectations and what the schema actually defined. In particular: "additionalProperties": true is required when using allOf. (Interestingly the schema was valid with additionalProperties === false ... but the JSON I expected would not fit the schema. And as a consiquence I am not sure the compiled typescript interface was correct interpretation of schema. There may be a bigger issue here to tackle later.)

Revised schema after playing with validating schema and the JSON:

{
  "title": "AllOf",
  "type": "object",
  "properties": {
    "fooAndBar": {
      "type": "object",
      "allOf": [
        {"$ref": "#/definitions/foo"},
        {"$ref": "#/definitions/bar"}
      ]
    },
    "foo": {
      "type": "object",
      "allOf": [
        {"$ref": "#/definitions/foo"}
      ]
    }
  },
  "allOf": [{
    "title": "AnotherAllOf",
    "type": "object",
    "additionalProperties": true,
    "properties": {
      "more": {
        "type": "object",
        "allOf": [
          {"$ref": "#/definitions/bar"}
        ]
      }
    }
  }],
  "definitions": {
    "foo": {
      "properties": {
        "a": { "type": "string" },
        "b": { "type": "integer" }
      },
      "additionalProperties": true,
      "required": ["a", "b"]
    },
    "bar": {
      "properties": {
        "a": { "type": "string" }
      },
      "additionalProperties": true,
      "required": ["a", "b"]
    }
  },
  "required": ["fooAndBar", "foo", "AnotherAllOf"],
  "additionalProperties": true
}

Example JSON that validates:

{
  "fooAndBar": {
    "a": "something",
    "b": 1
  },
  "foo": {
    "a": "something",
    "b": 1
  },
  "AnotherAllOf":{
    "more": {
    "b": 1
    }
  }
}

And based on this valid JSON, I would say this is expected typescript interface:

Case 4:

export interface Foo {
  a: string;
  b: number;
}
export interface Bar {
  a: string;
}
export interface AllOf {
  fooAndBar: Foo & Bar;
  foo: Foo;
  AnotherAllOf: {
    more?: Bar;
  };
}

Latest is here: https://github.com/darcyparker/json-schema-to-typescript/blob/1591ff0d7d34741c3bf2996ef672ef2aa829e03f/test/cases/allOf.ts

@bcherny
Copy link
Owner

bcherny commented Sep 29, 2016

I don't consider 2 a candidate.

I put it there because allOf is not on properties, so it probably should not generate any properties. JSON-schema is not well specified; from what I can tell, this is expected behavior, but I'm not sure. Can you dig up any docs one way or another?

Note: 3 agrees with dtsgenerator....

This does not seem to be right - it's flattening more and allOf, and erasing allOf in the process.

In general, let's try to refer to the JSON-schema spec, official examples, and official test suite. I don't trust most of these community implementations.

@darcyparker
Copy link
Contributor Author

darcyparker commented Sep 29, 2016

I agree 3 is wrong after playing with things. 2 is close to correct. And after I modified the schema, case 4 is probably close to correct. (But I am not confident what the right answer is yet.)

I agree going through the official examples you mentioned is the next step and derive expected typescript interface definitions that should be expected.

Side note: (I have lots of these cases of allOf outside the properties in JSON schema I am working with. They were auto-generated JSON schema from some C# code. So I am eager to add support for this case of allOf. But I do want it to be correct. I will talk with one of our C# developers to see what the interface looks like on their side.)

@darcyparker
Copy link
Contributor Author

darcyparker commented Sep 29, 2016

Do you want help with #1 ? Here's how I am thinking I could help:

  • Build a test coverage method that looks at draft 4 test schemas, and reports if there is a missing test case in json-schema-to-typescript
  • A template generator for missing test coverage cases would be nice too.
  • Start writing expected output for each test case... There's no debate about what valid JSON should be because it is in the test case. But there may be debates on valid transform to TS interface. So I expect there will be some review/debate on the expected output.

@darcyparker
Copy link
Contributor Author

I put it there because allOf is not on properties, so it probably should not generate any properties. JSON-schema is not well specified; from what I can tell, this is expected behavior, but I'm not sure. Can you dig up any docs one way or another?

https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/develop/tests/draft4/allOf.json#L5

Notice allOf is not on properties... and look at tests.

@bcherny
Copy link
Owner

bcherny commented Oct 1, 2016

Interesting - so it looks like an alternative syntax. Ie.

"allOf": [
    {
        "properties": {
            "bar": {"type": "integer"}
        },
        "required": ["bar"]
    },
    {
        "properties": {
            "foo": {"type": "string"}
        },
        "required": ["foo"]
    }
]

Is equivalent to

"properties": {
  "bar": {"type": "integer"},
  "foo": {"type": "string"}
},
"required": ["bar", "foo"]

And both should compile to

interface allOf {
  bar: number;
  foo: string;
}

@bcherny
Copy link
Owner

bcherny commented Oct 1, 2016

If you want, feel free to port the official test cases to a branch in this repo. I added you as a collaborator.

@darcyparker
Copy link
Contributor Author

darcyparker commented Oct 1, 2016

Yes, it looks like an alternate syntax. It seems like it is popular pattern in the JSON Schema I have been receiving. The JSON I am receiving is serialized from C# classes... and it looks like the inherited structure of properties are grouped using allOf.

Thanks for the invite to be a collaborator. I am happy to take this on. (I have enjoyed studying the code and watching it iterate the past few weeks. And I think its going to be a valuable tool. I am quite excited about the potential of having these interfaces built automatically for me. And I think others will appreciate it too.)

@bcherny
Copy link
Owner

bcherny commented Oct 1, 2016

Good to hear! I'm working on #44 today and tomorrow; specifically-

  • making sure $refs are resolved correctly
  • making sure circular $refs compile correctly
  • making sure $refs are lazily resolved (so a $ref can come before its declaration)

I'll also be refactoring the code a bit. If you can stick to adding test cases till this is done to avoid merge conflicts, it would be much appreciated. Will keep you posted.

@darcyparker
Copy link
Contributor Author

NP - I will stick to adding the test cases. I won't get to it over the weekend, but will will start early this week.

@bcherny
Copy link
Owner

bcherny commented Oct 1, 2016

Cool - again, thanks for all the help!

@bcherny bcherny added the bug label Oct 12, 2016
@bcherny
Copy link
Owner

bcherny commented Apr 22, 2017

Fixed in v2 - please reopen if you have any issues.

@bcherny bcherny closed this as completed Apr 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants