Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9628643
Pass through required properties to allOf children
cyclops23 Jan 19, 2022
3ff93cf
extend fix to anyOf and oneOf
Feb 15, 2022
5d8c187
fix: account for children making parent fields required
Feb 16, 2022
8957081
Merge branch 'next' into required-next
js2me Aug 15, 2022
9e47ef0
Merge branch 'next' into required-next
js2me Aug 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions src/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,57 @@ const getObjectTypeContent = (schema) => {
const complexTypeGetter = (schema) => getInlineParseContent(schema);
const filterContents = (contents, types) => _.filter(contents, (type) => !_.includes(types, type));

const makeAddRequiredToChildSchema = (parentSchema) => (childSchema) => {
Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let required = childSchema.required || [];
Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. ??)

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let properties = childSchema.properties || {};

// Inherit all the required fields from the parent schema that are defined
// either on the parent schema or on the child schema
// TODO: any that are defined at grandparents or higher are ignored
required = required.concat(
(parentSchema.required || []).filter(
(key) =>
!required.includes(key) && (_.keys(properties).includes(key) || _.keys(parentSchema.properties).includes(key)),
Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_.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

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
);
Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@PatrickShaw PatrickShaw Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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),
Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe childSchema.properties should be just properties

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

);

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

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@PatrickShaw PatrickShaw Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(additionalProperties, key) => ({
...additionalProperties,
[key]: (parentSchema.properties || {})[key],
}),
{},
),
};

return _.merge(
{
required: required,
properties: properties,
},
childSchema,
);
};

const complexSchemaParsers = {
[SCHEMA_TYPES.COMPLEX_ONE_OF]: (schema) => {
// T1 | T2
const combined = _.map(schema.oneOf, complexTypeGetter);
const combined = _.map(schema.oneOf.map(makeAddRequiredToChildSchema(schema)), complexTypeGetter);

return checkAndAddNull(schema, filterContents(combined, [TS_KEYWORDS.ANY]).join(" | "));
},
[SCHEMA_TYPES.COMPLEX_ALL_OF]: (schema) => {
// T1 & T2
const combined = _.map(schema.allOf, complexTypeGetter);
const combined = _.map(schema.allOf.map(makeAddRequiredToChildSchema(schema)), complexTypeGetter);
return checkAndAddNull(
schema,
filterContents(combined, [...JS_EMPTY_TYPES, ...JS_PRIMITIVE_TYPES, TS_KEYWORDS.ANY]).join(
Expand All @@ -205,7 +246,7 @@ const complexSchemaParsers = {
},
[SCHEMA_TYPES.COMPLEX_ANY_OF]: (schema) => {
// T1 | T2 | (T1 & T2)
const combined = _.map(schema.anyOf, complexTypeGetter);
const combined = _.map(makeAddRequiredToChildSchema(schema), complexTypeGetter);
const nonEmptyTypesCombined = filterContents(combined, [
...JS_EMPTY_TYPES,
...JS_PRIMITIVE_TYPES,
Expand Down
2 changes: 1 addition & 1 deletion tests/generated/v3.0/allof-example.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export interface Pet {
pet_type: string;
}

export type Dog = Pet & { bark?: boolean; breed?: "Dingo" | "Husky" | "Retriever" | "Shepherd" };
export type Dog = Pet & { bark?: boolean; breed: "Dingo" | "Husky" | "Retriever" | "Shepherd" };

export type Cat = Pet & { hunts?: boolean; age?: number };

Expand Down
66 changes: 59 additions & 7 deletions tests/generated/v3.0/full-swagger-scheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10610,9 +10610,41 @@ export class Api<SecurityDataType extends unknown> extends HttpClient<SecurityDa
*/
gistsUpdate: (
gistId: string,
data: (any | any | null) & {
data: (
| { description: string }
| {
files: Record<
string,
(
| { content: string }
| { filename: string | null }
| object
| ({ content: string } & { filename: string | null } & object)
) & { content?: string; filename?: string | null }
>;
}
| ({ description: string } & {
files: Record<
Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

string,
(
| { content: string }
| { filename: string | null }
| object
| ({ content: string } & { filename: string | null } & object)
) & { content?: string; filename?: string | null }
>;
})
) & {
description?: string;
files?: Record<string, (any | any | object | null) & { content?: string; filename?: string | null }>;
files?: Record<
string,
(
| { content: string }
| { filename: string | null }
| object
| ({ content: string } & { filename: string | null } & object)
) & { content?: string; filename?: string | null }
>;
},
params: RequestParams = {},
) =>
Expand Down Expand Up @@ -15917,9 +15949,21 @@ export class Api<SecurityDataType extends unknown> extends HttpClient<SecurityDa
owner: string,
repo: string,
data: (
| { status?: "completed"; [key: string]: any }
| { status?: "queued" | "in_progress"; [key: string]: any }
| ({ status?: "completed"; [key: string]: any } & { status?: "queued" | "in_progress"; [key: string]: any })
| {
status?: "completed";
conclusion: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";
name: string;
head_sha: string;
[key: string]: any;
}
| { status?: "queued" | "in_progress"; name: string; head_sha: string; [key: string]: any }
| ({
status?: "completed";
conclusion: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";
name: string;
head_sha: string;
[key: string]: any;
} & { status?: "queued" | "in_progress"; name: string; head_sha: string; [key: string]: any })
) & {
name: string;
head_sha: string;
Expand Down Expand Up @@ -15988,9 +16032,17 @@ export class Api<SecurityDataType extends unknown> extends HttpClient<SecurityDa
repo: string,
checkRunId: number,
data: (
| { status?: "completed"; [key: string]: any }
| {
status?: "completed";
conclusion: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";
[key: string]: any;
}
| { status?: "queued" | "in_progress"; [key: string]: any }
| ({ status?: "completed"; [key: string]: any } & { status?: "queued" | "in_progress"; [key: string]: any })
| ({
status?: "completed";
conclusion: "success" | "failure" | "neutral" | "cancelled" | "skipped" | "timed_out" | "action_required";
[key: string]: any;
} & { status?: "queued" | "in_progress"; [key: string]: any })
) & {
name?: string;
details_url?: string;
Expand Down
6 changes: 4 additions & 2 deletions tests/schemas/v3.0/allof-example.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ components:
discriminator:
propertyName: pet_type
Dog: # "Dog" is a value for the pet_type property (the discriminator value)
allOf: # Combines the main `Pet` schema with `Dog`-specific properties
required:
- breed
allOf: # Combines the main `Pet` schema with `Dog`-specific properties
- $ref: '#/components/schemas/Pet'
- type: object
# all other properties specific to a `Dog`
Expand All @@ -48,4 +50,4 @@ components:
hunts:
type: boolean
age:
type: integer
type: integer