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 application/openapi content type #1347

Closed
1 task done
JElgar opened this issue Sep 17, 2023 · 5 comments · Fixed by #1348
Closed
1 task done

Support application/openapi content type #1347

JElgar opened this issue Sep 17, 2023 · 5 comments · Fixed by #1348
Labels
enhancement New feature or request openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue!

Comments

@JElgar
Copy link

JElgar commented Sep 17, 2023

Description

I have an API which provides an endpoint to fetch the schema but returns the content-type header as application/vnd.oai.openapi; charset=utf-8. Briefly looking at https://stackoverflow.com/a/52542061/13473952 it seems that this content type is a bit fake but might be real at somepoint.

Currently when the API returns this content type an exception is thrown.

This is the error
✨ openapi-typescript 6.6.1
file:///home/jelgar/.npm/_npx/21d31b7563e6adbc/node_modules/openapi-typescript/dist/load.js:143
  const currentSchema = options.schemas[schemaID].schema;
                                                  ^

TypeError: Cannot read properties of undefined (reading 'schema')
  at load (file:///home/jelgar/.npm/_npx/21d31b7563e6adbc/node_modules/openapi-typescript/dist/load.js:143:53)
  at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
  at async openapiTS (file:///home/jelgar/.npm/_npx/21d31b7563e6adbc/node_modules/openapi-typescript/dist/index.js:58:5)
  at async generateSchema (file:///home/jelgar/.npm/_npx/21d31b7563e6adbc/node_modules/openapi-typescript/bin/cli.js:94:18)
  at async main (file:///home/jelgar/.npm/_npx/21d31b7563e6adbc/node_modules/openapi-typescript/bin/cli.js:170:5)

This seems to make sense given the current process for determining what format the file is in here
https://github.com/drwpow/openapi-typescript/blob/33b87df44e96442734bdd4682253312a32dc18c7/packages/openapi-typescript/src/load.ts#L129
which seems to be based on either an extension at the end of the url or if the content type includes yaml (which obviously isn't the case for application/vnd.oai.openapi). (It works for json as +json is in the spec)

Proposal

It feels like there are a couple of options here:
Add support for these semi fake content types:

application/vnd.oai.openapi      (YAML variant)
application/vnd.oai.openapi+json (JSON only variant)

And/or allow callers to override the expected response type (yaml/json) via a cli flag, something like

npx openapi-typescript https://url --schema-content-type json/yaml --output output.ts

(quite frankly I like having this as a fall back [even if the above is implemented] as if the service Im relying on returns a crazy content type but I know its yaml/json it would be nice to be able to override the detection)

Not sure if you think either of these make sense/if I have just missed an option I can already use but more than happy to contribute either.

Checklist

@JElgar JElgar added enhancement New feature or request PRs welcome PRs are welcome to solve this issue! openapi-ts Relevant to the openapi-typescript library labels Sep 17, 2023
@drwpow
Copy link
Contributor

drwpow commented Sep 17, 2023

which seems to be based on either an extension at the end of the url or if the content type includes yaml

right that really is the only content type that’s not supported—something that doesn’t give any hint that application/openapi or application/vnd.oai.opanapi is assumed YAML. Actually the scanning for partial content types is specifically for the +json extension which appears frequently in other related uses.

I’m not opposed to adding support for this, even if it’s “unofficial.” but are you aware of any prior art that suggests that the default is YAML? Would adding +yaml be out of the question here? I’m only scared to treat YAML as the default only to have someone else request that the exact same but for JSON. But if there’s some Swagger behavior, or even a consensus among several libraries that this is the way to parse it, that’s good enough. Just want to make sure we do whatever is the “majority” opinion.

As additional information, I’d like to find ways to “know” whether to parse something as YAML or JSON before parsing, mostly for error purposes. if you did have a syntax error, I want to be able to show the proper error message, and not leave people scratching their heads why a syntax error in their JSON schema threw a YAML parse error, etc.

@JElgar
Copy link
Author

JElgar commented Sep 17, 2023

I’m only scared to treat YAML as the default only to have someone else request that the exact same but for JSON

Yeah makes sense, totally with you! It looks like this is the current draft for adding the content types to ietf. It looks like the application/openapi type has been remove and instead you always have to provide +json and +yaml (which makes a lot of sense!) https://datatracker.ietf.org/doc/draft-ietf-httpapi-rest-api-mediatypes/. Seems like that means adding 'support' for application/openapi is meaningless so forget that.

Allow callers to override the expected response type (yaml/json) via a cli flag

That being said how do you feel about this suggestion? I would be nice if I could still use the tool despite the API im using being a bit crazy (and using a fake content type)

As additional information, I’d like to find ways to “know” whether to parse something as YAML or JSON before parsing, mostly for error purposes. if you did have a syntax error, I want to be able to show the proper error message, and not leave people scratching their heads why a syntax error in their JSON schema threw a YAML parse error, etc.

What do you mean by "know" here. Are you suggesting you should peek into the response data and try and determine if it json or yaml?

For the error message it feels like it would be nice to have a else statement inside the if (schema.protocol.startsWith("http")) block to raise an exception if its not detected as yaml or json (that would catch the error I'm having here).

@drwpow
Copy link
Contributor

drwpow commented Sep 18, 2023

🤔 we do something “dumb” for readable streams where we just check if it starts with { but the more I think about all the extra work we’re doing checking file extensions and the content-type header it’s probably unnecessary. Since we’re dealing with OpenAPI schemas that means a JSON response would have to start with {, right?

I’m wondering if that would just be a better solution across the board. There’s not an intentional reason why the two code paths diverge.

What do you mean by "know" here.

I just mean I want to always show a YAML error for YAML, and a JSON error for JSON. that means having some idea which is which before parsing (or, even if you blindly tried to parse as both to see if either worked but both threw, which error message do you show?). So I think just checking for { may just work? Determining valid JSON vs valid YAML is easy and isn’t really my concern; it’s correctly determining invalid JSON from invalid YAML that’s tricky. And thus the need to know before parsing.

@drwpow
Copy link
Contributor

drwpow commented Sep 18, 2023

@JElgar published openapi-typescript@beta that attempts this approach. Let me know if it behaves as expected or not with the given content types.

@JElgar
Copy link
Author

JElgar commented Oct 21, 2023

Sorry very late but this has fixed the issue, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openapi-ts Relevant to the openapi-typescript library PRs welcome PRs are welcome to solve this issue!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants