Add tests for additionalProperties #30

bigpopakap · 2020-02-10T18:42:09Z

I like to be able to set additionalProperties: false in schemas so that Ajv's removeAdditional: true option (enabled in #29) can strip out extra properties when a schema says so. (Here are the docs about the removeAdditional option)

bigpopakap · 2020-02-10T18:44:05Z

I broke this out from #29 as requested here. Unfortunately, it's a little confusing right now because this PR includes the commits from #29 since the test I need to be able to pass options to ajv

UPDATE: rebased/synced this PR so now it just includes the single relevant commit

bigpopakap · 2020-02-10T18:44:58Z

src/json-schema.ts

@@ -108,7 +108,7 @@ type PropertiesConstraint<
  : unknown;

 type AdditionalPropertiesConstraint<
-  AdditionalProperties extends SchemaLike | undefined
+  AdditionalProperties extends SchemaLike | (false | undefined)


Recreating my original comment from the other PR here:

I didn't want to allow additionalProperties: true, so I'm specifically doing (false | undefined) instead of (boolean | undefined)

bigpopakap · 2020-02-10T18:45:40Z

src/tsjson.test.ts

@@ -306,6 +306,30 @@ describe("More involved tests with objects", () => {
    const toFail = {};
    expect(() => parser.parse(JSON.stringify(toFail))).toThrow();
  });
+
+  test("Object disallowing additionalProperties", () => {


Recreating my original comment from the other PR here:

Let me know if you want me to add another test where the schema defined additionalProperties: false, but the removeAdditional option is omitted. I thought that might be overkill since that's more towards testing ajv's internals, which we may not need to worry about here.

ostrowr · 2020-02-10T21:03:54Z

I haven't checked this out and tried it yet, so I might be misunderstanding the intended use case here – but boolean should be assignable to SchemaLike. I think just writing S(false) rather than false will accomplish what you're looking to do here without having to widen the type of additionalProperties.

bigpopakap · 2020-02-10T21:07:47Z

Oh wow, yes, I also just noticed #26! I must have tried this before that change landed, and hadn't tested it again since then. That looks like it'll work. I'll give it a shot sometime today, and close this PR if it's been solved.

bigpopakap · 2020-02-11T01:42:40Z

src/tsjson.test.ts

@@ -308,6 +308,175 @@ describe("More involved tests with objects", () => {
  });
 });

+describe("additionalProperties", () => {


@ostrowr these tests are better at highlighting what I'm trying to fix. The type checking is a little off when mixing properties with additionalProperties. These tests that I have added fail both with and without my changes to json-schema.ts. I.e. the issue exists in the current master branch, and my changes to json-schema.ts do not fix them.

It seems very difficult to get better type checking for this scenario, so I'd appreciate any pointers or suggestions you may have and I'll be happy to continue working on this.

Thanks!

Unrelated, but I wanted to say anyway: I found the expectType() function quite clever! That was a nifty way to quickly and easily do typing tests

Thanks!

I'm going to take a closer look at this PR tonight/tomorrow – appreciate the new tests.

You know this code way better than I do, but my guess is that the fix will require combining PropertiesConstraint and AdditionalPropertiesConstraint the same was that AdditionalItems is included in ItemsConstraint

Ex.

PropertiesConstraint< Properties extends { [k: string]: SchemaLike } | undefined, Required extends readonly (keyof Properties)[] | undefined, AdditionalProperties extends SchemaLike | undefined // add this, and remove AdditionalPropertiesConstraint > = // stuff

I don't know if it's possible in Typescript to define an `object like this:

type PropertiesConstraint = { [P in keyof Properties]: Properties[P][typeof InternalTypeSymbol], [k: /* all other properties */]: unknown }

so for additionalProperties: S(true) the best might be that it gives you { [k: string]: unknown } and loses all type information for properties. But for additionalProperties: S(false), it's probably possible to have it revert back to the properties schemas.

Yeah, it seems like a perfect typing is blocked by microsoft/TypeScript#17867

Individual property access seems to be correctly typed, it's just the expectType<Validated<typeof schema>>(...) checks that fail.

This may just be impossible at this point, unfortunately.

bigpopakap · 2020-02-12T22:50:34Z

I just push some updates. It still isn't completely done (sorry for the noise), but I think this PR gets a little closer to a solution now. I'll clean up the tests sometime soon, and will push another version that actually proposes a full solution.

…lProperties

bigpopakap · 2020-02-14T18:02:53Z

Ok, I think this PR is in its final state. I don't think it's possible to do any better than is already there. I added some tests for some expected behavior. Some assertions are commented out and reference a Typescript issue that's blocking them, mostly for documentation. If you find these tests useful, we can merge this PR. Otherwise, I don't think there is any feature improvement I can make here.

Thank you for all your time and attention!

ostrowr · 2020-02-15T18:04:00Z

OK, this all makes sense! This type system isn't quite sound but is hopefully still useful for your use-case.

I'll go ahead and merge these new tests!

one thing to note is that the expectType function isn't actually that useful – if the thing you're testing is assignable to the thing you're expecting, then it will return true even if they're not exactly equal. I have an idea about how to improve this and will push that up shortly, then cut a release so everyone can use the ajv properties.

bigpopakap · 2020-02-15T20:38:33Z

Thank you @ostrowr!

Yeah, it'll be great to get a new version so I can start using the ajv properties. And I'm glad you found these tests useful.

The type system is actually quite great! I think this whole project is super useful and well done. Having this simple Typescript wrapper around JSON schemas is more than enough for my use cases. I understand that not all JSON schema features will be replicable in Typescript, but you've documented all of them really well in the README. And in this particular case, it seems like we're limited by Typescript itself.

I look forward to seeing what you do about the expectType function! As I was writing these tests, it did occur to me that, like you say, something like expectType<unknown> is not very useful :P Would you mind tagging me in a PR or commit so I can see what you do (just for my own learning)?

ostrowr · 2020-02-17T18:48:30Z

I look forward to seeing what you do about the expectType function! As I was writing these tests, it did occur to me that, like you say, something like expectType is not very useful :P Would you mind tagging me in a PR or commit so I can see what you do (just for my own learning)?

@bigpopakap I'm not sure if I'm going to merge this, because working on the branch it's pretty clear that I'm going to have to write some crazy types to get tests to pass typechecking, but https://github.com/ostrowr/ts-json-validator/compare/expect-true is the basic idea. Same equality relation I use in https://ostro.ws/2019/12/09/taking-types-too-far/

bigpopakap mentioned this pull request Feb 10, 2020

Allow passing options through to Ajv #29

Merged

bigpopakap commented Feb 10, 2020

View reviewed changes

bigpopakap commented Feb 11, 2020

View reviewed changes

test(additionalProperties): add tests for type-checking for additiona…

20d58e7

…lProperties

bigpopakap changed the title ~~Allow setting additionalProperties to false~~ Add tests for additionalProperties Feb 14, 2020

ostrowr merged commit 19cf111 into ostrowr:master Feb 15, 2020

bigpopakap mentioned this pull request Feb 15, 2020

fix(additionalProperties): treat additionalProperties as optional #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for additionalProperties #30

Add tests for additionalProperties #30

bigpopakap commented Feb 10, 2020

bigpopakap commented Feb 10, 2020 •

edited

Loading

bigpopakap Feb 10, 2020

bigpopakap Feb 10, 2020

ostrowr commented Feb 10, 2020 •

edited

Loading

bigpopakap commented Feb 10, 2020

bigpopakap Feb 11, 2020

bigpopakap Feb 11, 2020

ostrowr Feb 11, 2020

bigpopakap Feb 12, 2020

bigpopakap Feb 14, 2020

bigpopakap commented Feb 12, 2020

bigpopakap commented Feb 14, 2020

ostrowr commented Feb 15, 2020

bigpopakap commented Feb 15, 2020

ostrowr commented Feb 17, 2020

Add tests for additionalProperties #30

Add tests for additionalProperties #30

Conversation

bigpopakap commented Feb 10, 2020

bigpopakap commented Feb 10, 2020 • edited Loading

bigpopakap Feb 10, 2020

Choose a reason for hiding this comment

bigpopakap Feb 10, 2020

Choose a reason for hiding this comment

ostrowr commented Feb 10, 2020 • edited Loading

bigpopakap commented Feb 10, 2020

bigpopakap Feb 11, 2020

Choose a reason for hiding this comment

bigpopakap Feb 11, 2020

Choose a reason for hiding this comment

ostrowr Feb 11, 2020

Choose a reason for hiding this comment

bigpopakap Feb 12, 2020

Choose a reason for hiding this comment

bigpopakap Feb 14, 2020

Choose a reason for hiding this comment

bigpopakap commented Feb 12, 2020

bigpopakap commented Feb 14, 2020

ostrowr commented Feb 15, 2020

bigpopakap commented Feb 15, 2020

ostrowr commented Feb 17, 2020

bigpopakap commented Feb 10, 2020 •

edited

Loading

ostrowr commented Feb 10, 2020 •

edited

Loading