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.

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

Correctly handle generic model name resolution #427

Merged
merged 1 commit into from
Aug 19, 2019
Merged

Correctly handle generic model name resolution #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion src/metadataGeneration/typeResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,16 @@ export class TypeResolver {

private getTypeName(typeName: string, genericTypes?: ts.NodeArray<ts.TypeNode>): string {
if (!genericTypes || !genericTypes.length) { return typeName; }
return typeName + genericTypes.map((t) => this.getAnyTypeName(t)).join('');

const resolvedName = genericTypes.reduce((acc, generic) => {
if (ts.isTypeReferenceNode(generic) && generic.typeArguments && generic.typeArguments.length > 0) {
return [...acc, this.getTypeName(generic.typeName.getText(), generic.typeArguments)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@WoH I mentioned this in another recent PR, but there's a performance hit for using the spread operator since you are iterating through the acc array on every loop of the reduce. The implementation of the reduce could still be a pure function if it mutates the seed since the seed is not considered to be part of the inputs. In fact, it's much more standard in what I've seen to just use acc.push. Now I'm aware that genericTypes probably has a very small length, so the performance advantages are probably minimal, but I also think that .push is more readable. Here's the JSPerf from last time to demonstrate the performance advantages: https://jsperf.com/reduce-with-new-set-vs-seed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than have you make this acc.push update @WoH, I wanted to be considerate of the number of excellent contributions you've made. So I've made the update as part of #428 which fixes the other issue that Asterix presented to us. So I'll merge this PR so I can fold it into mine. Sound good? :)

} else {
return [...acc, this.getAnyTypeName(generic)];
}
}, [] as string[]);

return typeName + resolvedName.join('');
}

private getAnyTypeName(typeNode: ts.TypeNode): string {
Expand Down
4 changes: 4 additions & 0 deletions tests/fixtures/testModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ export interface TestModel extends Model {
modelsEnumIndirect?: TestSubEnumModelContainer;
typeAliasCase1?: TypeAliasModelCase1;
TypeAliasCase2?: TypeAliasModelCase2;
// tslint:disable-next-line
genericNested?: GenericRequest<Array<TypeAliasModel1>>;
// tslint:disable-next-line
genericNested2?: GenericRequest<Array<TypeAliasModel2>>;
}

export interface TypeAliasModel1 {
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/swagger/definitionsGeneration/definitions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,12 @@ describe('Definition generation', () => {
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
expect(propertySchema['x-nullable']).to.eq(true, `for property ${propertyName}[x-nullable]`);
},
genericNested: (propertyName, propertySchema) => {
expect(propertySchema.$ref).to.eq('#/definitions/GenericRequestArrayTypeAliasModel1');
},
genericNested2: (propertyName, propertySchema) => {
expect(propertySchema.$ref).to.eq('#/definitions/GenericRequestArrayTypeAliasModel2');
},
};

Object.keys(assertionsPerProperty).forEach(aPropertyName => {
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/swagger/schemaDetails3.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,12 @@ describe('Definition generation for OpenAPI 3.0.0', () => {
expect(propertySchema).to.not.haveOwnProperty('additionalProperties', `for property ${propertyName}`);
expect(propertySchema.nullable).to.eq(true, `for property ${propertyName}.nullable`);
},
genericNested: (propertyName, propertySchema) => {
expect(propertySchema.$ref).to.eq('#/components/schemas/GenericRequestArrayTypeAliasModel1');
},
genericNested2: (propertyName, propertySchema) => {
expect(propertySchema.$ref).to.eq('#/components/schemas/GenericRequestArrayTypeAliasModel2');
},
};

const testModel = currentSpec.spec.components.schemas[interfaceModelName];
Expand Down