Skip to content

Fix missing parameters in operation object #1090

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.

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 1 commit into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,17 @@ This will compile the code as you change automatically.

**Please fill out the template!** It’s a very lightweight template 🙂.

### TDD
### Use Test-driven Development!

This library is a great usecase for [test-driven development (TDD)](https://en.wikipedia.org/wiki/Test-driven_development). If you’re new to this, the basic workflow is:
Contributing to this library is hard-bordering-on-impossible without a [test-driven development (TDD)](https://en.wikipedia.org/wiki/Test-driven_development) strategy. If you’re new to this, the basic workflow is:

1. First, write a [test](#testing) that fully outlines what you’d _like_ the output to be.
2. Make sure this test **fails** when you run `npm test` (yes, _fails!_)
2. Next, make sure this test **fails** when you run `npm test` (yes, _fails!_)
3. Then, make changes to `src/` until the tests pass.

_Code generation is hard!_ And for that reason, starting with a very clear expectation of your end-goal can make working easier.
Reasoning about code generation can be quite difficult until you “invert your thinking” and approach it output-first. Adopting TDD can turn very unclear/abstract problems into concrete ones with clear steps to resolution.

✨ When starting any task, **write a failing test first!** ✨
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked wording of this to harp on how helpful TDD is when contributing.

I am not that smart. I need TDD to be able to do anything in this library :P


#### Updating snapshot tests

Expand Down
23 changes: 15 additions & 8 deletions src/transform/path-item-object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,32 @@ export default function transformPathItemObject(
if (!operationObject) continue;
const c = getSchemaObjectComment(operationObject, indentLv);
if (c) output.push(indent(c, indentLv));

// fold top-level PathItem parameters into method-level, with the latter overriding the former
const keyedParameters: Record<string, ParameterObject | ReferenceObject> = {};
if (!("$ref" in operationObject)) {
// important: OperationObject parameters come last, and will override any conflicts with PathItem parameters
for (const parameter of [...(pathItem.parameters ?? []), ...(operationObject.parameters ?? [])]) {
// note: the actual key doesn’t matter here, as long as it can match between PathItem and OperationObject
keyedParameters["$ref" in parameter ? parameter.$ref : parameter.name] = parameter;
}
}

if ("$ref" in operationObject) {
output.push(indent(`${method}: ${operationObject.$ref}`, indentLv));
}
// if operationId exists, move into an `operations` export and pass the reference in here
else if (operationObject.operationId) {
const operationType = transformOperationObject(operationObject, { path, ctx: { ...ctx, indentLv: 1 } });
const operationType = transformOperationObject(
{ ...operationObject, parameters: Object.values(keyedParameters) },
{ path, ctx: { ...ctx, indentLv: 1 } }
);
ctx.operations[operationObject.operationId] = {
operationType,
comment: getSchemaObjectComment(operationObject, 1),
};
output.push(indent(`${method}: operations[${escStr(operationObject.operationId)}];`, indentLv));
} else {
// fold top-level PathItem parameters into method-level, with the latter overriding the former
const keyedParameters: Record<string, ParameterObject | ReferenceObject> = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moves this up one level; that’s it!

// important: OperationObject parameters come last, and will override any conflicts with PathItem parameters
for (const parameter of [...(pathItem.parameters ?? []), ...(operationObject.parameters ?? [])]) {
// note: the actual key doesn’t matter here, as long as it can match between PathItem and OperationObject
keyedParameters["$ref" in parameter ? parameter.$ref : parameter.name] = parameter;
}
const operationType = transformOperationObject(
{ ...operationObject, parameters: Object.values(keyedParameters) },
{ path, ctx: { ...ctx, indentLv } }
Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ export interface RequestBodyObject extends Extensable {
*/
export interface MediaTypeObject extends Extensable {
/** The schema defining the content of the request, response, or parameter. */
schema?: SchemaObject;
schema?: SchemaObject | ReferenceObject;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Irrelevant type bug that came up while writing the test

/** Example of the media type. The example object SHOULD be in the correct format as specified by the media type. The example field is mutually exclusive of the examples field. Furthermore, if referencing a schema which contains an example, the example value SHALL override the example provided by the schema. */
example?: any;
/** Examples of the media type. Each example object SHOULD match the media type and specified schema if present. The examples field is mutually exclusive of the example field. Furthermore, if referencing a schema which contains an example, the examples value SHALL override the example provided by the schema. */
Expand Down
112 changes: 112 additions & 0 deletions test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe("openapiTS", () => {
});

describe("3.0", () => {
/** test that custom $refs are ignored rather than throw errors */
test("custom properties", async () => {
const generated = await openapiTS({
openapi: "3.0",
Expand Down Expand Up @@ -74,6 +75,7 @@ export type operations = Record<string, never>;
`);
});

/** test that component examples aren’t parsed (they may be invalid / pseudocode) */
test("components.examples are skipped", async () => {
const generated = await openapiTS({
openapi: "3.0",
Expand Down Expand Up @@ -124,6 +126,7 @@ export type operations = Record<string, never>;
`);
});

/** test that $ref’d parameters make it into the paths object correctly */
test("parameter $refs", async () => {
const generated = await openapiTS(new URL("./fixtures/parameters-test.yaml", import.meta.url));
expect(generated).toBe(`${BOILERPLATE}
Expand Down Expand Up @@ -177,6 +180,115 @@ export type operations = Record<string, never>;
`);
});

/** test that $ref’d parameters, operation parameters, and method parameters all make it into the operation object correctly */
test("operations keep all parameters", async () => {
const schema: OpenAPI3 = {
openapi: "3.0",
info: { title: "Test", version: "1.0" },
paths: {
"/post/{id}": {
get: {
operationId: "getPost",
parameters: [
{ name: "format", in: "query", schema: { type: "string" } },
{ $ref: "#/components/parameters/post_id" },
],
responses: {
200: {
description: "OK",
content: {
"application/json": { schema: { $ref: "#/components/schemas/Post" } },
},
},
},
},
parameters: [{ name: "revision", in: "query", schema: { type: "number" } }],
},
},
components: {
schemas: {
Post: {
type: "object",
properties: {
id: { type: "number" },
title: { type: "string" },
body: { type: "string" },
published_at: { type: "number" },
},
required: ["id", "title", "body"],
},
},
parameters: {
post_id: {
name: "post_id",
in: "path",
schema: { type: "string" },
required: true,
},
},
},
};
const generated = await openapiTS(schema);
expect(generated).toBe(`${BOILERPLATE}
export interface paths {
"/post/{id}": {
get: operations["getPost"];
parameters: {
query: {
revision?: number;
};
};
};
}

export type webhooks = Record<string, never>;

export interface components {
schemas: {
Post: {
id: number;
title: string;
body: string;
published_at?: number;
};
};
responses: never;
parameters: {
post_id: string;
};
requestBodies: never;
headers: never;
pathItems: never;
}

export type external = Record<string, never>;

export interface operations {

getPost: {
parameters: {
query: {
revision?: number;
format?: string;
};
path: {
post_id: components["parameters"]["post_id"];
};
};
responses: {
/** @description OK */
200: {
content: {
"application/json": components["schemas"]["Post"];
};
};
};
};
}
`);
});

/** test that remote $refs are loaded correctly */
test("remote $refs", async () => {
const generated = await openapiTS(new URL("./fixtures/remote-ref-test.yaml", import.meta.url));
expect(generated).toBe(`${BOILERPLATE}
Expand Down