Pass through required properties to allOf/anyOf/oneOf children #342

hedgepigdaniel · 2022-02-15T07:48:39Z

Including #336, but:

extended to also work for anyOf/oneOf.
modified to avoid a bug where parent fields were incorrectly marked as required

hedgepigdaniel · 2022-02-15T07:51:27Z

tests/generated/v3.0/full-swagger-scheme.ts

@@ -15947,13 +15947,13 @@ export class Api<SecurityDataType extends unknown> extends HttpClient<SecurityDa
        | { status?: "queued" | "in_progress"; [key: string]: any }
        | ({ status?: "completed"; [key: string]: any } & { status?: "queued" | "in_progress"; [key: string]: any })
      ) & {
-        name: string;
+        name?: string;


This is correct

hedgepigdaniel · 2022-02-15T07:53:27Z

tests/generated/v3.0/full-swagger-scheme.ts

        head_sha: string;
        details_url?: string;
        external_id?: string;
        status?: "queued" | "in_progress" | "completed";
        started_at?: string;
-        conclusion?: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";
+        conclusion: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";


This unfortunately is incorrect. This is the abbridged/relevant schema:

"schema": { "type": "object", "properties": { "name": { "type": "string", "description": "The name of the check. For example, \"code-coverage\"." }, "status": { "type": "string", "description": "The current status. Can be one of `queued`, `in_progress`, or `completed`.", "enum": ["queued", "in_progress", "completed"] }, "conclusion": { "type": "string", "description": "**Required if you provide `completed_at` or a `status` of `completed`**. The final conclusion of the check. Can be one of `success`, `failure`, `neutral`, `cancelled`, `skipped`, `timed_out`, or `action_required`. \n**Note:** Providing `conclusion` will automatically set the `status` parameter to `completed`. Only GitHub can change a check run conclusion to `stale`.", "enum": [ "success", "failure", "neutral", "cancelled", "skipped", "timed_out", "action_required" ] } }, "anyOf": [ { "properties": { "status": { "enum": ["completed"] } }, "required": ["conclusion"], "additionalProperties": true }, { "properties": { "status": { "enum": ["queued", "in_progress"] } }, "additionalProperties": true } ] },

As in, conclusion is required, but only if status is completed. But the generated type indicates that it is required in all cases, which is incorrect.

hedgepigdaniel · 2022-02-15T07:57:07Z

src/schema.js

@@ -181,13 +181,19 @@ const filterContents = (contents, types) => _.filter(contents, (type) => !_.incl
 const complexSchemaParsers = {
  [SCHEMA_TYPES.COMPLEX_ONE_OF]: (schema) => {
    // T1 | T2
-    const combined = _.map(schema.oneOf, complexTypeGetter);
+    const combined = _.map(
+      schema.oneOf.map((s) => _.merge({ required: schema.required }, s)),


This logic isn't quite right, because it assumes that schema.required makes sense in the parent schema. But like the example in "full-swagger-scheme` shows, this isn't always the case.

Fixing this properly would require the logic in complexSchemaParsers to be lifted up a bit (i.e. if there is an anyOf/oneOf/allOf, then the entire type of the parent schema needs to be mapped, not just the fields in the anyOf.

But maybe a good first step is just to filter schema.required down to those fields that are directly defined in the nested schema. That way it would at least tighten the type for simple cases, without introducing this bug.

hedgepigdaniel · 2022-02-16T03:35:44Z

The last commit fixes the bug I mentioned with required referencing fields defined on the parent.

PatrickShaw · 2022-02-21T04:26:36Z

src/schema.js

@@ -178,16 +178,57 @@ const getObjectTypeContent = (schema) => {
 const complexTypeGetter = (schema) => getInlineParseContent(schema);
 const filterContents = (contents, types) => _.filter(contents, (type) => !_.includes(types, type));

+const makeAddRequiredToChildSchema = (parentSchema) => (childSchema) => {
+  let required = childSchema.required || [];


LTS for Node 12 is ending in April 2022 so depending on the level of support swagger-typescript-api provides to Node you could use null coalescing (i.e. ??)

Yes, I tried to avoid that because personally (unfortunately) I still use this with Node.js 12. IMO the best way to address this is to add a build step, but that's out of scope for this PR.

PatrickShaw · 2022-02-21T04:28:08Z

src/schema.js

@@ -178,16 +178,57 @@ const getObjectTypeContent = (schema) => {
 const complexTypeGetter = (schema) => getInlineParseContent(schema);
 const filterContents = (contents, types) => _.filter(contents, (type) => !_.includes(types, type));

+const makeAddRequiredToChildSchema = (parentSchema) => (childSchema) => {


Imo it would be simpler to read if you did:

({ required = [], properties = [], ...other }) =>

Instead of

(childSchema) =>

As it keeps default logic/values all in one place

Could be. I personally prefer creating a new variable instead of reassigning function parameters (destructured or not). But happy to change depending on what the maintainers prefer.

PatrickShaw · 2022-02-21T04:28:49Z

src/schema.js

+  // Identify properties that are required in the child schema, but
+  // defined only in the parent schema (TODO: this only works one level deep)
+  const parentPropertiesRequiredByChild = required.filter(
+    (key) => !_.keys(childSchema.properties).includes(key) && _.keys(parentSchema.properties).includes(key),


I believe childSchema.properties should be just properties

It could be, but I think childSchema.properties makes more sense here, since it's referring to the properties that are in the child schema, not the properties variable that will be modifiied and returned (which includes properties that are not in the child schema).

PatrickShaw · 2022-02-21T04:56:57Z

src/schema.js

+      (key) =>
+        !required.includes(key) && (_.keys(properties).includes(key) || _.keys(parentSchema.properties).includes(key)),
+    ),
+  );


Goes back to my point about Node support but: You could use a Set instead of !required.includes(key). I think it'd be slightly clearer in terms of what you're trying to achieve (I assume avoiding duplicates)

Yes, honestly I was just trying to stick to the convention, the surrounding code uses lodash data structures so I used them too 🤷 - happy to change if the maintainers prefer.

Yep fair enough. Leave it up to the author :)

PatrickShaw · 2022-02-21T04:59:29Z

src/schema.js

+  required = required.concat(
+    (parentSchema.required || []).filter(
+      (key) =>
+        !required.includes(key) && (_.keys(properties).includes(key) || _.keys(parentSchema.properties).includes(key)),


_.keys(properties).includes(key) || _.keys(parentSchema.properties).includes(key)

Is it necessary to filter out keys that do not exist in the properties? I wouldn't have thought it would cause any issues if they were added

I'm not sure, but given that logically required is a set, the most correct thing seems to be to avoid duplicates.

PatrickShaw · 2022-02-21T05:10:59Z

src/schema.js

+  // Add such properties to the child so that they can be overriden and made required
+  properties = {
+    ...properties,
+    ...parentPropertiesRequiredByChild.reduce(


Why only fields required by the child?

To my knowledge allOf should apply ALL fields, regardless of whether they're optional/required. Are optional fields being handled elsewhere in this code base and if so shouldn't this code live in the same spot as that logic?

Maybe more importantly: Rather than overriding the original properties I believe the childSchema's properties and the parent schema's properties should be merged together so that both the child and parent's type conditions are validated against. As per: https://json-schema.org/understanding-json-schema/reference/combining.html#allof

the given data must be valid against all of the given subschemas

Maybe it's just out of scope for this PR?

As per: https://json-schema.org/understanding-json-schema/reference/combining.html#allof

the given data must be valid against all of the given subschemas

I think that's referring to all of the subschemas, not the combination of parent/children. Although yes, my understanding is that it must be valid against parent and child.

Why only fields required by the child?
the childSchema's properties and the parent schema's properties should be merged together

These fields aren't being added to check that their shape is correct. The generated types for oneOf/anyOf already inherit from the parent schema type at the typescript level - so effectively they are already merged.

The reason to add them here (duplicating the definitions from the parent) is so that they can be marked as required in typescript. Hence, this only includes fields that are defined in the parent, but made required in the child.

Another option might be to do it in typescript, e.g.

type ChildSchema = ParentSchema & { child_field: string; parent_field_made_optional_in_the_child: Exclude<undefined, ParentSchema['parent_field_made_optional_in_the_child']> };

This would handle fields from grandparent schemas, and nparent schemas - but it requires the parent schema to have a name, which currently it doesn't.

Are optional fields being handled elsewhere in this code base and if so shouldn't this code live in the same spot as that logic?

I'm not sure what you mean - open to suggestions if you have them.

hedgepigdaniel · 2022-02-22T07:01:13Z

tests/generated/v3.0/full-swagger-scheme.ts

+            >;
+          }
+        | ({ description: string } & {
+            files: Record<


@PatrickShaw notice that files has exactly the same shape as the one on (new) line 10665. The entire definition is duplicated here - not because the shape is different, just so that it can be made a compulsory property. The type from the parent schema starts on line 10663 - and it has no name, so we can't refer to it here.

js2me · 2022-08-15T09:42:07Z

Hello @hedgepigdaniel !
Thanks for your PR!

* Feature: allow passing "patch" option to "swagger2openapi" (#283) Co-authored-by: cassel <[email protected]> * Loads prettier config from user's project (#286) * fix: problem with missing HttpResponse type with --to-js option * fix: missing extractRequestBody field in type * fix(reject): fix reject and exit with code 1 if error (#295) * fix(reject): fix reject and exit with code 1 if error * feat: add a parameter to sort types and types properties (#299) Co-authored-by: rcatoio <[email protected]> * feat: add an option to disable throwing an error when request.ok is not true (#297) Co-authored-by: rcatoio <[email protected]> Co-authored-by: Sergey S. Volkov <[email protected]> * fix: fix missing `title` of object type (#303) * chore: refresh auto generated code; chore: add axios to dev deps * bump: up to 9.3.0 * bump: up to 9.3.1; fix: axios * chore: fix typo (#376) * 304 Support readOnly properties (#365) * allow mutually exclusive input options (#327) Co-authored-by: Sergey S. Volkov <[email protected]> * Update fetch-http-client.eta (#329) * BREAKING_CHANGE: add --extract-response-body option; bump: version to 9.4.0 * feat: added authorization token to headers Co-authored-by: MarcinFilipek <[email protected]> * feat: --extract-response-error option * chore: refresh code gen tests * fix: change COMPLEX_NOT_OF to COMPLEX_NOT (#356) * Fix request format and allow for files in request - With code review changes (#350) * fix pick format allow for file array in form data requests * fixed code review suggestions Co-authored-by: Daniele De Matteo <[email protected]> Co-authored-by: Sergey S. Volkov <[email protected]> * Pass through required properties to allOf/anyOf/oneOf children (#342) * Pass through required properties to allOf children * extend fix to anyOf and oneOf * fix: account for children making parent fields required Co-authored-by: Anders Cassidy <[email protected]> Co-authored-by: Daniel Playfair Cal <[email protected]> Co-authored-by: Sergey S. Volkov <[email protected]> * Add option to change main Api class name (#320) * Update api.eta * Update index.d.ts * Update index.js * Update index.js * Update config.js Co-authored-by: Sergey S. Volkov <[email protected]> * fix: missing stringifyFormItem method; bump: up version to 10.0.0 * docs: update docs, changelog, fix: problem with complex types (makeAddRequiredToChildSchema) Co-authored-by: Xavier Cassel <[email protected]> Co-authored-by: cassel <[email protected]> Co-authored-by: Adrian Wieprzkowicz <[email protected]> Co-authored-by: EvgenBabenko <[email protected]> Co-authored-by: RoCat <[email protected]> Co-authored-by: rcatoio <[email protected]> Co-authored-by: 卡色 <[email protected]> Co-authored-by: 江麻妞 <[email protected]> Co-authored-by: Kasper Moskwiak <[email protected]> Co-authored-by: Ben Watkins <[email protected]> Co-authored-by: bonukai <[email protected]> Co-authored-by: baggoedw <[email protected]> Co-authored-by: Marcus Dunn <[email protected]> Co-authored-by: Daniele De Matteo <[email protected]> Co-authored-by: Daniel Playfair Cal <[email protected]> Co-authored-by: Anders Cassidy <[email protected]> Co-authored-by: Daniel Playfair Cal <[email protected]>

js2me · 2022-08-17T22:04:41Z

@all-contributors please add @hedgepigdaniel for code bugs and add @PatrickShaw for review

allcontributors · 2022-08-17T22:04:43Z

@js2me

I could not determine your intention.

Basic usage: @all-contributors please add @Someone for code, doc and infra

For other usages see the documentation

js2me · 2022-08-17T22:05:03Z

@all-contributors please add @hedgepigdaniel for code bugs

allcontributors · 2022-08-17T22:05:12Z

@js2me

I've put up a pull request to add @hedgepigdaniel! 🎉

js2me · 2022-08-17T22:05:53Z

@all-contributors please add @PatrickShaw for review

allcontributors · 2022-08-17T22:06:01Z

@js2me

I've put up a pull request to add @PatrickShaw! 🎉

cyclops23 and others added 2 commits February 15, 2022 18:46

Pass through required properties to allOf children

9628643

extend fix to anyOf and oneOf

3ff93cf

hedgepigdaniel commented Feb 15, 2022

View reviewed changes

hedgepigdaniel mentioned this pull request Feb 15, 2022

Pass through required properties to allOf children #336

Closed

fix: account for children making parent fields required

5d8c187

hedgepigdaniel force-pushed the required-next branch from c00448e to 5d8c187 Compare February 16, 2022 03:06

PatrickShaw reviewed Feb 21, 2022

View reviewed changes

hedgepigdaniel commented Feb 22, 2022

View reviewed changes

PatrickShaw approved these changes Feb 24, 2022

View reviewed changes

Merge branch 'next' into required-next

8957081

js2me added the merge label Aug 15, 2022

Merge branch 'next' into required-next

9e47ef0

js2me merged commit 5a09a7a into acacode:next Aug 15, 2022

allcontributors bot mentioned this pull request Aug 17, 2022

docs: add hedgepigdaniel as a contributor for code, bug #387

Merged

allcontributors bot mentioned this pull request Aug 17, 2022

docs: add PatrickShaw as a contributor for review #388

Merged

js2me mentioned this pull request Aug 29, 2022

Required properties in swagger.json create duplicate interfaces #394

Open

Uh oh!

Pass through required properties to allOf/anyOf/oneOf children #342

Pass through required properties to allOf/anyOf/oneOf children #342

Uh oh!

Conversation

hedgepigdaniel commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedgepigdaniel commented Feb 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedgepigdaniel Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js2me commented Aug 15, 2022

Uh oh!

js2me commented Aug 17, 2022

Uh oh!

allcontributors bot commented Aug 17, 2022

Uh oh!

js2me commented Aug 17, 2022

Uh oh!

allcontributors bot commented Aug 17, 2022

Uh oh!

js2me commented Aug 17, 2022

Uh oh!

allcontributors bot commented Aug 17, 2022

Uh oh!

Uh oh!

hedgepigdaniel commented Feb 15, 2022 •

edited

Loading

hedgepigdaniel Feb 22, 2022 •

edited

Loading