-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feat/make oas2tson broader #318
Conversation
…; comment tsType in output
.gitignore
Outdated
@@ -5,6 +5,7 @@ node_modules | |||
.idea | |||
# Visual Studio Code | |||
.vscode | |||
.devcontainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devcontainers can be useful for the development. Why ignoring them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed the project didn't have devcontainers intentionally. But I've removed from ignore and committed mine.
package.json
Outdated
"json2ts": "./dist/esm/src/cli.js json2ts -i ./example/output/json/components.schemas -o ./example/output/types -c ./example/json-schema-to-typescript-config.json", | ||
"json2ts": "./dist/esm/src/cli.js json2ts -i ./example/output/oas2json/components.schemas -o ./example/output/json2ts -c ./example/json-schema-to-typescript-config.json", | ||
"lint": "eslint .", | ||
"preoas2json": "npm run build", | ||
"oas2json": "./dist/esm/src/cli.js oas2json -i ./example/openapi.yml -o ./example/output/json", | ||
"oas2json": "./dist/esm/src/cli.js oas2json -i ./example/openapi_kitchen_sink.yml -o ./example/output/oas2json", | ||
"preoas2ts": "npm run build", | ||
"oas2ts": "./dist/esm/src/cli.js oas2ts -i ./example/openapi.yml -o ./example/output/types -c ./example/json-schema-to-typescript-config.json", | ||
"oas2ts": "./dist/esm/src/cli.js oas2ts -i ./example/openapi_kitchen_sink.yml -o ./example/output/oas2ts -c ./example/json-schema-to-typescript-config.json", | ||
"preoas2tson": "npm run build", | ||
"oas2tson": "./dist/esm/src/cli.js oas2tson -i ./example/openapi.yml -o ./example/output/ts", | ||
"oas2tson": "./dist/esm/src/cli.js oas2tson -i ./example/openapi_kitchen_sink.yml -o ./example/output/oas2tson", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually don't care if the json files are generated using an OAS: we just need to know that a folder contains json schemas.
Said so, we should rollback from oas2json
to json
. The same applies to json2ts
and types
.
I would also revert openapi_kitchen_sink
to plain openapi
Changing all of this is not bringing any advantage atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/commands/oas2tson.ts
Outdated
@@ -1,6 +1,6 @@ | |||
import $RefParser, { ParserOptions } from '@apidevtools/json-schema-ref-parser' | |||
import { Command } from 'commander' | |||
import filenamify from 'filenamify' | |||
// import filenamify from 'filenamify' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's not used must be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that in cleanup. Thanks.
src/commands/oas2tson.ts
Outdated
@@ -159,8 +208,7 @@ const main = () => { | |||
options.input, | |||
options.output, | |||
options.properties, | |||
options.excludeDereferencedIds, | |||
options.logger | |||
options.excludeDereferencedIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone wants to pass a custom instance of pino, in this way they can't anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember removing that, but apparently I did. Fixed.
@@ -257,4 +257,270 @@ tap.test('oas2tson', async t => { | |||
'Customer.ts is created correctly' | |||
) | |||
}) | |||
|
|||
t.test('runCommand function with --path', async t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With all you changed, I'm not sure that this unique test will be enough to cover it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took that from the previous attempt to get -p paths
working, which was reverted because it assumed hard camel case naming.
From a lines coverage perspective, tap
is reporting slightly better coverage than before because I removed tsType
in adaptSchema
(not needed; may fail to compile in ajv
strict mode).
Are you looking for more functional coverage? If so, which -p
keywords would you like tested?
I think most keywords in the components
hierarchy are covered by components.schemas
, but I can add a couple of others if you think they're distinct enough.
If it helps, the changes in oas2tson.ts
come down to:
- Generating valid JS/TS names. (Many small helper functions, regexps, etc.)
isForOutput
to control items added tooutputSchemasMetaData
Much of oas2tson
is about naming, though. So, yes, many things changed to get names right so dereferencing would work. 😄
Maybe the pet
path would be a better option than store/order
? The pet
path includes $ref
s to schemas, an example, a request body, and a response. They'll be dereferenced in paths__pet
but must generate as expected if pet
matches.
@ilteoood Thanks for the feedback. Most of the issues you raised are addressed as noted in comments. For tests, #318 (comment) -- I'm willing to buff the tests and would like some guidance on what you'd like to see covered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the output of both json schemas and ts types.
As far as I can see, none of this is actually usable.
args: | ||
VARIANT: 20-bookworm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args: | |
VARIANT: 20-bookworm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have a default value, there's no need to repeat it here
@@ -1,4 +1,4 @@ | |||
openapi: 3.0.3 | |||
openapi: 3.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to have stronger tests, can we store both versions and run tests with both versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openapi-schema-to-json-schema supports OpenAPI 3.0, not OpenAPI 3.1.
Until openapi-schema-to-json-schema
supports OpenAPI 3.1 and newer versions of JSON Schema, I don't think suggesting this tool supports OpenAPI 3.1 is safe. Maybe someone knows of a better option that supports OpenAPI 3.0 (required for some API gateways) and OpenAPI 3.1 and can add an issue to switch to it.
Among key differences:
- 3.1 supports the
examples
(multiple examples) key in schemas; 3.0 supportsexample
(single example) only. I know the difference causes issues when building proxies in some API gateways. I thinkopenapi-schema-to-json-schema
handles it, but can't remember. - 3.0 supports
nullable: true
; 3.1 does not supportnullable
, preferring a compound (array oroneOf
type, see link in notes added to README);openapi-schema-to-json-schema
fails with the array type option even though it produces the same output asnullable: true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's not safe, why use it for the tests and examples? It's misleading.
@@ -188,6 +189,42 @@ $ openapi-transformer-toolkit oas2tson -i ./openapi.yml -o ./schemas -p paths | |||
-h, --help Display help for command | |||
``` | |||
|
|||
#### Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is the audience for these notes? The users or who develops this module?
In the first scenario, we can leave it here. Otherwise, we should move it to something like CONTRIBUTING.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People consuming the output of openapi-transformer-toolkit
. Reasoning:
securitySchemas
fails ---p components.securitySchemas
failing is not a bug (and why)- what to use for
-p
-- this question prompted the issue that led to this PR - prefixing -- so people understand why this choice was made and what prefixes to expect
- TSON paths don't work for Fastify -- the original intent of
oas2tson
was to support Fastify with the JSON schema type provider; I was hoping paths would work, but examining the output realized they wouldn't; preparing others for a similar disappointment - change to
nullable
-- this difference was a challenge for me getting the test spec right and required some digging to find the answer
@@ -0,0 +1,28 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does storing these kind of files make sense?
They are not a valid json schema, so how can we use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed oas2tson
only, so any issues with other commands are existing issues not related to this PR or the issue that motivated it.
I don't know how reliable they are, but I found a couple of online JSON schema validators and both say it is valid JSON Schema by drafts 4 through 7.
Maybe I misunderstood #305 (comment). I thought this comment meant -p
should everything. If you had a narrower scope in mind, I can adjust it I know what it is.
My thoughts on the topic for oas2tson
...
oas2tson
supports Fastify validation and serialization json-schema-to-ts
type provider per #35 . Given that intent and what the OpenAPI spec defines:
components.schemas
-- makes sense because we can use them to composeRouteOptions
components.requestBodies
-- probably won't work for abody
validation schema, but could work if you select thex.content.['application/json
]member, maybe? ([OpenAPI](https://spec.openapis.org/oas/latest.html#request-body-object); note the
content` field)components.headers
-- don't match what Fastify expects for a header validation schema (OpenAPI)components.parameters
-- don't match what Fastify expects forparams
,querystring
, etc. (OpenAPI)components.examples
-- aren't used by Fastify directlycomponents.links
-- aren't used by Fastify directlycomponents.callbacks
-- aren't used by Fastify directlywebhooks
-- aren't used by Fastify as far as I can tell; not my area of expertise, so may be missing somethingpaths
-- cannot be used by Fastify asRouteOptions
Which leads me to think supporting anything by components.schemas
in oas2tson
may not add value. For oas2tson
users, the best approach is likely define schemas for everything, then define parameters in terms of schemas as below.
components:
schemas:
PetQuerySchema:
type: object
properties:
...
parameters:
PetQuery:
in: query
schema:
$ref: '#/components/schemas/PetQuerySchema'
...
Then use the TSON schemas:
const routeSchema = {
querystring: PetQuerySchema // from oas2tson ouput PetQuerySchema.ts
...
}
For other commands, supporting more parts of the OpenAPI spec may make sense. I don't know, but note that oas2ts
and json2ts
do not support -p
and only export components.schemas
.
Any problems in other commands' output are existing problems and should be handled in separate issues -- and are likely due to the fact that openapi-schema-to-json-schema
supports only Schema Object and Parameter Object. That may feed back into the original question about -p.
{ | ||
"value": { | ||
"id": 1, | ||
"name": "Rover", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even this is not a valid json schema. The type is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In OpenAPI, examples don't have types, but this is likely an issue with openapi-schema-to-json-schema
only supporting Schema Object and Parameter Object and supporting only OpenAPI 3.0. I believe components.examples
is OpenAPI 3.1.
@@ -0,0 +1,6 @@ | |||
export const ShipDate = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why something like this would be useful?
This is a constant object with constant keys and values. Not a typescript type that we can re-use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oas2tson
produces constant objects that represent OpenAPI schemas to support the JSON schema type provider in Fastify. The corresponding TypeScript type in example/output/types/ShipDate.d.ts
is export type ShipDate = Date;
.
Your question comes down to choices about how to build an OpenAPI schema. My background is data modeling, metadata, large data warehouses, data management, etc. That leads me to value metadata about data elements so people know what they are and know issues to consider. (For example, consider the differences between a "city" as defined by legal boundaries and a "city" as defined by the post office (may not match) and needing to know which is in play in different data objects.) OpenAPI specs are decent metadata stores as outlined below.
# Fields.yaml
components:
schemas:
ShipDate:
description: |-
A detailed description about ShipDate because you're using your OpenAPI spec as a metadata repository for your application because your company doesn't have one but you have enough experience to know metadata is critical for any data source if you want it to be usable.
And you like using `markdown` so you can reference [links](https://example.com/blah-blah-blah), include tables, etc., as needed to make API documentation useful for API consumers, not just the afterthought that many internal APIs I see seem to think of it as.
type: string
format: date-time
...
# Orders.yaml
Order:
type: object
properties:
shipDate:
$ref: 'Fields.yaml#/components/schemas/ShipDate'
...
# all properties members ref `Fields.yaml`
OrderQuery:
type: object
properties:
shipDate:
$ref: '#/components/schemas/ShipDate'
# all properties members ref `Fields.yaml`
...
# and anywhere else I need ShipDate -- DRY.
import {ShipDate as ShipDateSchema} from `.../ShipDate.ts`
const schema = {
params: ShipDateSchema
}
// Arguably, shipDate shouldn't be a path parameter; pretend it's a similarly defined id
fastify.get('/some/url/:shipDate', { schema } , handler1)
fastify.get('/another/url/:shipDate', { schema }, handler2)
fastify.get('/yetanother/url/:shipDate', { schema }, handler3)
@ilteoood I added comments on specific points. Many of your questions are answered by the following statement.
|
I'm thinking this PR should not be merged. I believe removing Outputs not usefulIn this comment, I outline the different Lack of tool support
Based on a quick comparison to OpenAPI examples, it looks like outputs for
It's up to the maintainers to decide if they want
Proposed next stepsI will create an issue and PR for I can create an issue and PR to remove I can create an issue and PR to ensure |
Hey folks, where do we stand with this PR? |
Imho, it should be closed |
Agreed. The extra outputs really aren't usable as schemas for Fastify (4.x,
haven't had a chance to check 5.x yet, but guessing same based on migration
guide.)
…On Thu, Oct 3, 2024, 07:56 Matteo Pietro Dazzi ***@***.***> wrote:
Imho, it should be closed
—
Reply to this email directly, view it on GitHub
<#318 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AASIV6Y6HSDSMQ642MQKZY3ZZUWIPAVCNFSM6AAAAABNKSSCKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJRGIZTENRTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Thanks for all the work on this, sometimes it just doesn't go as we hoped! |
Add
-p paths
and others support tooas2tson
.Closes #305
_
separators for valid JS/TS names.schemas
, with last node of OpenAPI "path" to the schema referenced to avoid name collisions.tsType: "Date"
inoas2tson
becauseoas2tson
doesn't need it and it causesajv
strict mode to fail.oas2tson
andoas2json
support.oas2tson
docs with useful notes