fix: merge compatible definitions in union types #722

dmchurch · 2021-03-25T19:37:20Z

Most validation keywords apply to only one of the basic types, so a
"string" type and an "array" type can share a definition without
colliding as a ["string", "array"] type, as long as they don't have any
incompatibilities with each other. Modify UnionTypeFormatter to collapse
these disjoint types into a single definition, without using anyOf.

dmchurch · 2021-03-25T19:40:54Z

I've classed this as a "fix" because it doesn't change the semantic meaning of the output, and it doesn't enable new features for schema developers. Feel free to upgrade it to a "feat", though.

Most validation keywords apply to only one of the basic types, so a "string" type and an "array" type can share a definition without colliding as a ["string", "array"] type, as long as they don't have any incompatibilities with each other. Modify UnionTypeFormatter to collapse these disjoint types into a single definition, without using anyOf.

domoritz

Thanks for the pull request. Please take a look at the tests.

src/Utils/mergeDefinitions.ts

domoritz · 2021-03-25T20:25:23Z

I think you probably just need to update the Vega-Lite test case. You can use the provided script to do it automatically.

@domoritz

Change the array.flat() call to use spread notation, since both members are guaranteed to be arrays (unlike the types call, above). Thanks for the suggestion, @domoritz! Co-authored-by: Dominik Moritz <domoritz@gmail.com>

dmchurch · 2021-03-25T21:43:30Z

Right! I'll get on the tests now. Some of them weren't passing even in my original checkout, but at the very least I can make sure that anything affected by this change gets updated.

domoritz · 2021-03-25T21:47:51Z

Try yarn test:update

The vega-lite test originally worked after updating the snapshot, but there was a broken reference that got mistakenly purged by removeUnreachable (which assumed that any definition would only ever have a single property that could hold references). Added a validation check to vega-lite.test to register the failure, updated addReachable to check all possible properties (unless $ref is set; the spec says that everything else gets ignored in that case).

dmchurch · 2021-03-26T04:42:24Z

I've actually got a couple test cases I want to add to exercise some of the weirder edge cases involving definition collapse, but I'm setting this down for the night. In any event, the existing test cases already exercise it fairly well.

In addition to the raw schema tests which just check that nothing has changed since the last snapshot, this adds makeExemplar, which builds a raw value using the parsed BaseType representation of a TypeScript type tree. This way, tests can specify valid/invalid data to be verified using the Ajv validator in addition to raw schema output tests.

In a few simple cases like (1 | number), mergeDefinitions will now correctly collapse two not-quite-disjoint type definitions together, by discarding the validation restrictions on the more-restricted type. This replaces the string-merging logic in UnionTypeFormatter, which was discarding annotations.

dmchurch · 2021-03-28T05:22:07Z

Whew! That got much bigger than expected. I added some schema validation tests, which uncovered some issues with the existing code, which required a more elaborate test harness, which means the test harness needs more tests... you get the idea. One of these days I'll get all those yaks shaved.

dmchurch · 2021-03-28T05:27:10Z

...and I literally just now realized that the makeExemplar code ought to be living on-or-near the BaseType classes themselves, which would clean up the huge mess of if-else-if-else in makeExemplar.ts. Oh well, there are more revisions I need to make anyway...

Feel free to offer critique/direction if this isn't going in a useful direction, btw, I'm not averse to scrapping things and rebuilding.

dmchurch · 2021-03-28T05:37:24Z

...and mergeDefinitions would get so very much smaller if I just called it twice, once in each direction. Yeah, don't merge this yet, I've got work to do.

domoritz · 2021-03-28T15:56:45Z

Thank you for working on this and making sure the feature is clean. I definitely think this is a good improvement.

domoritz

Overall, this looks great. Please wrap up the remaining issues and mark the pull request as ready for (final) review.

domoritz · 2021-04-04T17:52:54Z

test/utils.ts

-    return new SchemaGenerator(program, createParser(program, config), createFormatter(config), config);
+interface Validator<T> {
+    (value: T): boolean | Promise<unknown>;
+    //                    ^^^^^^^^^^^^^^^^ *Why* does validateSchema() sometimes return this? Who knows! >.<


Please fix this ;-)

Haha, that's not us! Ajv has a very unfortunate coding error, the .d.ts file has a different signature from the online documentation. This is just working around that.

I suppose I could open an issue on their repo, though.

That would be great and then add a link to it in the code. Thanks!

domoritz · 2021-04-04T17:53:34Z

test/utils.ts

+                pass,
+                message: pass
+                    ? () => hint() + "Expected validation to fail, but it did not." + printReceived()
+                    : () =>


Let's use template strings here.

domoritz · 2021-04-04T17:55:32Z

test/vega-lite/schema.json

+          "items": {
+            "type": "string"
+          },
+          "type": [


This is strange. So The type is either string or array but the items property above only makes sense for array.

This is as intended/expected, actually! The type-specific JSON schema validation constraints specifically only apply when the value is of the appropriate type, which is why a lot of definitions can get collapsed together. If you included "items" without "type" it would mean "The value can be any type, but if it's an array its elements must be strings". So, this schema means "The value can be an array or a string. If the value is an array, its elements must be strings".

That's why all the type-specific properties have different names - "minLength" only applies to strings, but the same concept applied to arrays is "minItems".

I see. As long as we can still use the schema to generate APIs from it (which we do), I'm okay.

domoritz · 2021-04-04T17:55:54Z

src/Utils/makeExemplar.ts

+import { UnionType } from "../Type/UnionType";
+
+export function makeExemplar(type: BaseType | undefined): unknown {
+    return makeExemplars(type)[0];


There is a lot of untested code in this file.

Yeah, I definitely need to refactor this into something cleaner. On my to-do list, certainly.

domoritz · 2021-04-04T17:57:33Z

src/Utils/makeExemplar.ts

@@ -0,0 +1,230 @@
+import { AliasType } from "../Type/AliasType";


This file is only used in tests, right? Then it shouldn't be in lib utils.

domoritz · 2021-04-04T17:59:11Z

src/Utils/mergeDefinitions.ts

+import { RawTypeName } from "../Schema/RawType";
+
+const isIntegerKey = "\0isInteger";
+type _Definition = Definition & { [isIntegerKey]?: true };


Let's clean this up. Import Definition above as something else so we don't need the _ here.

domoritz · 2021-04-04T17:59:42Z

src/Utils/mergeDefinitions.ts

+            result.type = Array.from(new Set(type1.concat(type2)));
+        } else if (type1 || type2) {
+            // one is typed and one isn't - unmergeable
+            return null;


See the untested warnings below. Let's add some more tests.

domoritz · 2021-04-04T18:00:09Z

src/Utils/removeUnreachable.ts

-        }
-    } else if (definition.not) {
+    } else {
+        addReachable(definition.anyOf, definitions, reachable);


This looks great

domoritz · 2021-05-07T22:53:09Z

@dmchurch do you plan to wrap this up soon?

domoritz · 2021-06-29T23:44:05Z

@dmchurch can you please finish up this pull request?

dmchurch changed the title ~~Merge compatible definitions in union types~~ fix: Merge compatible definitions in union types Mar 25, 2021

dmchurch force-pushed the merge-definitions branch from 9e8c27d to 246b65c Compare March 25, 2021 19:42

domoritz reviewed Mar 25, 2021

View reviewed changes

src/Utils/mergeDefinitions.ts Outdated Show resolved Hide resolved

domoritz changed the title ~~fix: Merge compatible definitions in union types~~ fix: merge compatible definitions in union types Mar 25, 2021

Update src/Utils/mergeDefinitions.ts

6b0e19b

Change the array.flat() call to use spread notation, since both members are guaranteed to be arrays (unlike the types call, above). Thanks for the suggestion, @domoritz! Co-authored-by: Dominik Moritz <domoritz@gmail.com>

dmchurch added 4 commits March 27, 2021 16:40

Simplifying logic in addReachable, adding tests

291ebf9

Adding enum-merging to mergeDefinitions, unit test

58f0dc9

domoritz marked this pull request as draft March 28, 2021 15:57

domoritz reviewed Apr 4, 2021

View reviewed changes

domoritz changed the base branch from master to next October 5, 2021 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: merge compatible definitions in union types #722

fix: merge compatible definitions in union types #722

dmchurch commented Mar 25, 2021

dmchurch commented Mar 25, 2021

domoritz left a comment

domoritz commented Mar 25, 2021

dmchurch commented Mar 25, 2021

domoritz commented Mar 25, 2021

dmchurch commented Mar 26, 2021

dmchurch commented Mar 28, 2021

dmchurch commented Mar 28, 2021

dmchurch commented Mar 28, 2021

domoritz commented Mar 28, 2021

domoritz left a comment

domoritz Apr 4, 2021

dmchurch Apr 6, 2021

domoritz Apr 6, 2021

domoritz Apr 4, 2021

domoritz Apr 4, 2021

dmchurch Apr 6, 2021

domoritz Apr 6, 2021

domoritz Apr 4, 2021

dmchurch Apr 6, 2021

domoritz Apr 4, 2021

domoritz Apr 4, 2021

domoritz Apr 4, 2021

domoritz Apr 4, 2021

domoritz commented May 7, 2021

domoritz commented Jun 29, 2021

		@@ -0,0 +1,230 @@
		import { AliasType } from "../Type/AliasType";

fix: merge compatible definitions in union types #722

Are you sure you want to change the base?

fix: merge compatible definitions in union types #722

Conversation

dmchurch commented Mar 25, 2021

dmchurch commented Mar 25, 2021

domoritz left a comment

Choose a reason for hiding this comment

domoritz commented Mar 25, 2021

dmchurch commented Mar 25, 2021

domoritz commented Mar 25, 2021

dmchurch commented Mar 26, 2021

dmchurch commented Mar 28, 2021

dmchurch commented Mar 28, 2021

dmchurch commented Mar 28, 2021

domoritz commented Mar 28, 2021

domoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz commented May 7, 2021

domoritz commented Jun 29, 2021