Skip to content
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

tuple validation #28

Closed
marcoqu opened this issue Jun 5, 2018 · 10 comments
Closed

tuple validation #28

marcoqu opened this issue Jun 5, 2018 · 10 comments

Comments

@marcoqu
Copy link
Contributor

marcoqu commented Jun 5, 2018

Hi,
when generating a schema for a tuple-like structure, I noticed some issues:

type Test = [number, number]

generates a schema that allows for additional values, while it shouldn't.

{
    "$schema": "http://json-schema.org/draft-06/schema#",
    "definitions": {
        "Test": {
            "type": "array",
            "items": [
                {
                    "type": "number"
                },
                {
                    "type": "number"
                }
            ],
            "minItems": 2,
            "additionalItems": {
                "anyOf": [
                    {
                        "type": "number"
                    },
                    {
                        "type": "number"
                    }
                ]
            }
        }
    },
    "$ref": "#/definitions/Test"
}

also,

type Test = [number, number, number|undefined]

requires at least three values, while it should require at least 2 and at most three.

Anyways, great project.

@domoritz
Copy link
Member

domoritz commented Jun 5, 2018

Yes, that's a bug. I don't know how easy it is to get this information from the AST but I'd love to get a PR for this.

@marcoqu
Copy link
Contributor Author

marcoqu commented Jun 6, 2018

I could have a look, but it looks like the additionalItems property was explicitely added, while I think tuples should not allow additionalItems. But maybe there's something I'm missing.

return {
    type: "array",
    items: tupleDefinitions,
    minItems: tupleDefinitions.length,
    ...(tupleDefinitions.length > 1 ? {additionalItems: {anyOf: tupleDefinitions}} : {}),
};

could become

return {
    type: "array",
    items: tupleDefinitions,
    minItems: tupleDefinitions.length,
    maxItems: tupleDefinitions.length,
};

And then one would need to look into the undefined items in the tail of the tuple to lower the minItems count?

@marcoqu
Copy link
Contributor Author

marcoqu commented Jun 7, 2018

I looked into it.
Apparently "When accessing an element outside the set of known indices, a union type is used instead".

So, while this const tuple:[string, number] = ["", 1, 2] gives an error. This tuple[4] = 99 is valid. Link

Still, I believe it goes against the expected behavior of a schema generator. Maybe you could want an option for this? --strictTuples?

With regard to the lowering the value of minItems if the trailing elements of the tuple include undefined, it appears the in typescript there is no way of having optional arguments to a tuple. Nonetheless, lowering the value of minItems would have the expected effect of getting undefined if the item is not set...

Not sure how you want to go about this.

@domoritz
Copy link
Member

domoritz commented Jun 7, 2018

Thanks for looking into this!

In this library, I chose not to add many options and have the most correct defaults. If you want more options, they could be added to https://github.com/YousefED/typescript-json-schema. I hope that makes sense.

@marcoqu
Copy link
Contributor Author

marcoqu commented Jun 7, 2018

Ok, thanks!
Since I see you are involved in both, can i ask you what's the difference between this project and YousefED/typescript-json-schema?

@domoritz
Copy link
Member

domoritz commented Jun 7, 2018

Good question! YousefED/typescript-json-schema uses typeAtLocation while this project uses the AST itself. Using the AST is more complicated but tracks aliases better. I use this project for Vega-Lite and that's my main use case. I'm not going to fix anything unless it is relevant for Vega-Lite. YousefED/typescript-json-schema has a lot more customization options and the code is much shorter. However, since it is less complex, it sometimes does not work as well. Both projects benefit from each other. For instance, I try to share test cases where I can and if there is a fix that applies to both projects, I usually copy it over.

@marcoqu
Copy link
Contributor Author

marcoqu commented Jun 7, 2018

Got it.
I'm using this for a vscode extension for generating json schemas from types. I believe for the moment I'll maintain a close fork of your version since it seems to be the most complete.

Thanks!

@domoritz
Copy link
Member

domoritz commented Jun 7, 2018

I see. Well, let me ask then, would you be willing to help me maintain this library? that is, respond to issues about tuples and help resolve issues around tuples when there is a new typescript version? The changes you made could be useful in Vega-Lite as well.

@marcoqu
Copy link
Contributor Author

marcoqu commented Jun 7, 2018 via email

@domoritz
Copy link
Member

domoritz commented Jun 7, 2018

Sweet. Can you send a PR with --strictTuples? Make sure to add some tests and update the docs. I will test it against Vega-Lite and if it makes sense, I'm happy to make strictTuples the default. Sounds good?

domoritz pushed a commit that referenced this issue Jun 8, 2018
With the `--strictTuples` option enabled, the generated schema does not allow for additional properties. 

See issue #28
@marcoqu marcoqu closed this as completed Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants