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.

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

Support single operation parameter #1535

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

tvanier
Copy link

@tvanier tvanier commented Feb 15, 2024

Changes

First, thanks for openapi-typescript!
I came across a generation failure when compiling the spec below (Azure OpenAI inference).
It appears some operations are defined with one single parameter object, not as array - see an example here.

I am not 100% sure this is valid OpenAPI, but since openapi-typescript@7 compiles it successfully, I thought it would be good to fix the problem in 6.x.

openapi-typescript https://raw.githubusercontent.com/Azure/azure-rest-api-specs/main/specification/cognitiveservices/data-plane/AzureOpenAI/inference/preview/2023-12-01-preview/inference.json

file:///.../node_modules/openapi-typescript/dist/transform/operation-object.js:19
                for (const param of operationObject.parameters ?? []) {
                                                    ^

TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))
    at transformOperationObject (file:///.../node_modules/openapi-typescript/dist/transform/operation-object.js:19:53)
    at openapiTS (file:///.../node_modules/openapi-typescript/dist/index.js:137:39)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async generateSchema (file:///.../node_modules/openapi-typescript/bin/cli.js:94:18)
    at async main (file:///.../node_modules/openapi-typescript/bin/cli.js:170:5)

How to Review

All iteration over operation or path parameters should call getParametersArray to make sure parameters are iterable.

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

Copy link

changeset-bot bot commented Feb 15, 2024

🦋 Changeset detected

Latest commit: 3473462

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-typescript Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@drwpow
Copy link
Owner

drwpow commented Feb 15, 2024

Awesome, thank you! Happy to approve & merge this if you’d be so kind as to add a patch changeset (instructions in the 🦋 comment; TLDR run npx changeset from the project root)

@drwpow
Copy link
Owner

drwpow commented Feb 15, 2024

I am not 100% sure this is valid OpenAPI

Many people use OpenAPI schemas in production that probably violate some aspect of the technical spec in some way or another. Since we’re converting to static TypeScript types, too, there can often be more opinionated interpretation than most realize. So the general approach of this library is to Support as much of the official spec as possible (only barring technical limitations of TypeScript).

Beyond the spec, we’ll generally allow for fixes to common footguns/rough edges so long as it doesn’t deviate from the spec, and isn’t a huge maintenance burden to maintain. Your PR is a great example of this—all the existing tests pass; we just covered an additional usecase without any breaking changes. That’s always a win in my book.

@tvanier
Copy link
Author

tvanier commented Feb 15, 2024

Thanks @drwpow , fully agreed on the flexible approach. I think I added a patch changeset, let me know if anything is missing or incorrect.
For info I use openapi-typescript + fetch to connect to different AI providers (OpenAI, Azure, Mistral, Ollama etc), works great!

}`);
});

it("handles non-array parameters", () => {
Copy link
Owner

@drwpow drwpow Feb 15, 2024

Choose a reason for hiding this comment

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

Great tests 🎉

@drwpow
Copy link
Owner

drwpow commented Feb 15, 2024

Thanks again! Changeset is perfect 🙂

@drwpow drwpow merged commit 48ca069 into drwpow:6.x Feb 15, 2024
6 checks passed
@tvanier
Copy link
Author

tvanier commented Feb 19, 2024

Thanks @drwpow for merging this PR! Do you think it could be included in an upcoming 6.7.x release?

@tvanier
Copy link
Author

tvanier commented Mar 12, 2024

Hi @drwpow , may I ask if there is a plan to release these PR changes in version 6.7.5 when you get a chance? Thanks!

@drwpow
Copy link
Owner

drwpow commented Mar 12, 2024

@tvanier yes thank you for the nudge! I needed the nudge. Normally these get deployed automatically but I had to pause it until 7.x is stable (which is very close; just needs the last couple items done on the roadmap). Will release it later today.

@drwpow
Copy link
Owner

drwpow commented Mar 12, 2024

Released!

@tvanier
Copy link
Author

tvanier commented Mar 13, 2024

Thanks @drwpow , release 6.7.5 works good! And I started to migrate to v7 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants