search

nearform / openapi-transformer-toolkit

Automate design-first API workflows by generating schemas and types from OpenAPI specs.
21 stars 4 forks source link

What does `-p` option do? #305

Open jmjf opened 2 months ago

jmjf commented 2 months ago

Problem

Working to fix https://github.com/nearform/openapi-transformer-toolkit/issues/294, I realized I cannot figure out what the -p, --properties option is supposed to do. That's probably a "me" problem. Can someone please explain so we can understand how to fix the option for oas2tson or remove it.

If the team can provide some feedback on intent and preferred direction in light of discussion on https://github.com/nearform/openapi-transformer-toolkit/pull/230, I don't mind trying to put together code and a PR.

Background

In oas2json and oas2tson, the option is defined as:

  .option(
    '-p, --properties <string>',
    'Comma-separated list of properties to convert from the OpenAPI file'
  )

The option is not available for oas2ts or json2ts.

The examples use -p paths, like openapi-transformer-toolkit oas2json -i ./openapi.yml -o ./schemas -p paths.

For oas2json, adding -p paths outputs a paths directory with JSON Schema files for the paths. Based on the description, I'd expect -p User,Tag to output only User and Tag. But adding a comma separated list fails with the message, "Failed to convert non-object attribute, skipping." Based on the code, components.schemas is always added to the list of properties.

For oas2tson, as reverted, -p paths fails. According to comments in https://github.com/nearform/openapi-transformer-toolkit/pull/224, pets/findByStatus becomes const pets-findByStatus because - is not valid character in a JavaScript identifier. The - is not a problem for oas2json because oas2json writes JSON Schema, not variables.

Note PR seems to be trying to implement path generation, which later spawned https://github.com/nearform/openapi-transformer-toolkit/pull/230 mentioned above. There, is seems like the team isn't convinced the tool should support TSON path generation. If it shouldn't, should it support JSON path generation either?

The name pets-findByStatus is really the filename for the output file. For example, oas2json writes file names like pets-findByStatus.json and pets-{petId}.json. The name is generated by filenamify(name, { replacement: '-' }) in both oas2tson and oas2json.

Questions

Looking at oas2tson vs. oas2json, It feels wrong that they're so different. Maybe too simplistic, but TSON is just JSON with no quotes around the member names and a const <name> = in front of it and as const on the end. Somewhere in Nodeland there must be a JavaScript code generator that can take an object and generate output suitable for including in a file. json2ts is basically what we need, but with the values instead of the types. Then something like oas2json -> JSON.parse() -> object2json + as const should work.

simoneb commented 2 months ago

@ilteoood I'd like to hear your thoughts on this

ilteoood commented 2 months ago

It should give you the ability to choose the fields of an openapi file to convert.

In the following example, paths is an entry in the yaml that you can choose to process.

Screenshot_2024-08-12-10-46-02-790_com.github.android-edit.jpg

I think it has been created to allow the user to define what needs to be converted: request objects, response objects, general definitions or query parameters. Said so, it's not something we should throw away.

jmjf commented 2 months ago

Okay.

So the intent isn't to generate only the named schema but more about classes of objects.

Does anyone have thoughts on the naming challenges? oas2json can use names oas2tson can't. But making oas2tson names valid by forcing strict camel case makes valid OpenAPI specs break. It may need to use a separator instead.

Thoughts on preferred approaches?

On Mon, Aug 12, 2024, 04:49 Matteo Pietro Dazzi @.***> wrote:

It should give you the ability to choose the fields of an openapi file to convert.

In the following example, paths is an entry in the yaml that you can choose to process.

Screenshot_2024-08-12-10-46-02-790_com.github.android-edit.jpg (view on web) https://github.com/user-attachments/assets/29b91af1-6dab-45ff-87f8-410f5095efc3

I think it has been created to allow the user to define what needs to be converted: request objects, response objects, general definitions or query parameters. Said so, it's not something we should throw away.

— Reply to this email directly, view it on GitHub https://github.com/nearform/openapi-transformer-toolkit/issues/305#issuecomment-2283419148, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASIV63V64YDMRI23CJMD4DZRBZIVAVCNFSM6AAAAABMLFBZBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBTGQYTSMJUHA . You are receiving this because you authored the thread.Message ID: @.***>

ilteoood commented 2 months ago

I don't have a strong feeling about it, however you want to approach it, the final outcome is the important matter

jmjf commented 2 months ago

I'll work on making paths work with the following, unless some sees problems with this approach. Probably this weekend.

So, pets/{petId} get a file and const named pets__petId. Or is it safe to reduce the double to one ? Is there a risk of pets/petId or a similar collision?

TODO (note to self): check OpenAPI for other special characters to consider for paths and the major branches of the model that might benefit from TSON consts. Focus on Fastify schema capabilities for now.

On Tue, Aug 13, 2024, 06:46 Matteo Pietro Dazzi @.***> wrote:

I don't have a strong feeling about it, however you want to approach it, the final outcome is the important matter

— Reply to this email directly, view it on GitHub https://github.com/nearform/openapi-transformer-toolkit/issues/305#issuecomment-2285943011, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASIV65JK35KRH2S7OTWWDDZRHPXTAVCNFSM6AAAAABMLFBZBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBVHE2DGMBRGE . You are receiving this because you authored the thread.Message ID: @.***>

ilteoood commented 2 months ago

Honestly speaking I would avoid replacing / and {} with something else, because you could create some collisions with other path naming that the user could have. Since those symbols have a specific meaning for OAS, I would use that.

jmjf commented 2 months ago

The existing code for both oas2json and oas2tson replaces / in paths and other names from the spec with - to get a filename. That works for oas2json because pets-{petId} is valid filename. pets/{petId} adds a directory layer.

oas2tson needs to write valid JavaScript/TypeScript identifier names. /, -, { and } are not valid characters in an identifier name. For example

// invalid identifier because /, {, }
const pets/{petId} = {
   ...
} as const;

// invalid identifier because -
const pets-petId = {
   ...
} as const;

oas2tson needs to use either _ or $ as a separator in the identifier name to get valid identifiers.

The files oas2tson writes should match the identifier name to avoid something like import { pets_petId } from 'path/to/tson/pets-{petId}'. I think _ is less likely to be a filename problem than $ (environment variable trigger).

Does this reasoning make sense for oas2tson given the need to write valid JavaScript/TypeScript?

I propose to make the replacements affect oas2tson only and PR it so it works. Then we can decide if oas2json needs any changes (probably doesn't).

ilteoood commented 2 months ago

The problem of using _ as a separator relies on a collision if someone defines the paths "pets/{petId}" and "pets_petId". On a REST standard perspective the second path doesn't make any sense, but technically speaking is something that can be defined. The same could apply also for the dollar sign.

I see what's the issue here, we are trying to map an URL without char limitations to something that is limited like a TypeScript type...

jmjf commented 2 months ago

pets/{petId} could map to pets__petId or keep the trailing _ to reduce collision risk, but I acknowledge that doesn't solve the problem you raise.

I'm open to ideas if someone sees a better solution.

On Tue, Aug 13, 2024, 13:20 Matteo Pietro Dazzi @.***> wrote:

The problem of using _ as a separator relies on a collision if someone defines the paths "pets/{petId}" and "pets_petId". On a REST standard perspective the second path doesn't make any sense, but technically speaking is something that can be defined. The same could apply also for the dollar sign.

I see what's the issue here, we are trying to map an URL without char limitations to something that is limited like a TypeScript type...

— Reply to this email directly, view it on GitHub https://github.com/nearform/openapi-transformer-toolkit/issues/305#issuecomment-2286747112, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASIV66I6EM3I7LKVGPCMITZRI57FAVCNFSM6AAAAABMLFBZBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBWG42DOMJRGI . You are receiving this because you authored the thread.Message ID: @.***>

jmjf commented 2 months ago

I thought through this problem today to plan what I need to do with oas2tson to support -p paths. The naming challenges noted in previous comments make this problem complex. Some OpenAPI features cannot be supported in oas2tson without butchering OpenAPI names beyond recognition.

I plan to start coding tomorrow, but this may take a while. If anyone has suggestions or solutions for some of the problems or sees issues I missed, let me know.

TL;DR:

First thoughts

Running oas2json -p paths writes output to <target-directory>/components.schemas and <target-directory>/paths. oas2tson (and oas2ts) write all output to the target directory. With the example pet store schema:

What motivated these features?

Exporting paths and other parts of the spec besides components.schemas with oas2json -- https://github.com/nearform/openapi-transformer-toolkit/issues/54

Exporting as TSON (oas2tson) -- https://github.com/nearform/openapi-transformer-toolkit/issues/35

What does oas2json do?

Does oas2json support the following top-level members of the spec?

npm run oas2json -- -p <member-name>

Does oas2json support the following lower level members of the spec?

Both oas2json and oas2tson always export components.schemas and work.

oas2tson requires JavaScript friendly names. The OpenAPI spec does not require JavaScript friendly names and requires names that aren't JavaScript friendly for some things. For example, the components.callbacks examples include 'http://notificationServer.com?transactionId={$request.body#/id}&email={$request.body#/email}' as a member name. That's fine if you wrap it in quotes, but oas2tson unquotes names, so it doesn't work for oas2tson. It may be that the main reason oas2tson has so few problems with components.* members is that its users are JS/TS folks who use JS/TS naming patterns in their components.schemas.

oas2json references $refs, but oas2tson embeds $refs. So, if I $ref: '#/components/examples/Foo' in components.schemas.Pet, the full definition of Foo will be embedded in Pet.tson, not referenced. API authors consuming the TSON cannot change myExample and affect Pet -- ignoring the fact oas2tson declares everything as const so it's immutable.

I added sections to the example spec to support testing what oas2json and oas2tson do with different -p values. In the list below, the first result is oas2json.

components can include the following. I didn't try to test these cases.

Callbacks may appear in paths and webhooks. Do not expect oas2tson to support paths and webhooks with callbacks.

Decisions

jmjf commented 2 months ago

@ilteoood; @simoneb

As I finished that long comment outlining the problem, I had a crazy thought. What's your opinion on this?

oas2tson should take the JSON from oas2json and build const <name> = <JSON-text> as const;, where:

That would get output something like the following for the /user path after applying prettier. Note the content section uses JS-unfriendly names, but is still valid JS/TS because they're wrapped in quotes.

const user = {
  post: {
    tags: ['user'],
    summary: 'Create user',
    description: 'This can only be done by the logged in user.',
    operationId: 'createUser',
    requestBody: {
      description: 'Created user object',
      content: {
        'application/json': {
          schema: {
            $ref: 'User.json'
          }
        },
        'application/xml': {
          schema: {
            $ref: 'User.json'
          }
        },
        'application/x-www-form-urlencoded': {
          schema: {
            $ref: 'User.json'
          }
        }
      }
    },
    responses: {
      default: {
        description: 'successful operation',
        content: {
          'application/json': {
            schema: {
              $ref: 'User.json'
            }
          },
          'application/xml': {
            schema: {
              $ref: 'User.json'
            }
          }
        }
      }
    }
  },
  title: '/user',
  $id: 'user.json'
} as const;`

So, oas2tson becomes something like:

Later

Plan

Even later

It's more complex than that because oas2tson wants to dereference $refs.

In oas2tson.ts, line 54 has a comment that suggests the code supports expanding $refs where the ref is to components/schemas only. If a path uses $refs to components/headers, components/parameters, etc., the original oas2tson design does not support it. The same is true for schemas that reference components/examples.

This effort will involve either expanding how oas2tson handles dereferencing so paths and references to components/examples will work or deciding that isn't practical.

More to come. At this point, I believe I'll need a week or three to work through the details.

jmjf commented 2 months ago

@ilteoood, @simoneb

Why does oas2json strip the path information from refs?

npm run oas2json -- -p paths

Look at example/output/json/paths/pet.json and note the $refs to Pet are all Pet.json without #/components/schemas/. On case-insensitive file systems, the pet route is referencing itself. On case sensitive file systems, it's "file not found" unless you also have a Pets path, which isn't what you really want. If I $ref headers, parameters, and other components, in the path, same problem.

The same thing happens in the other subdirectories. For example, add to the following to components in the example schema.

  callbacks:
    userCreatedCallback:
      'https://notify.example.com?event=UserCreated':
        post:
          operationId: notifyUserCreatedCB
          summary: notify on user create
          requestBody:
            description: Callback payload
            content:
              'application/json':
                schema:
                  $ref: '#/components/schemas/User'
          responses:
            '200':
              description: Callback succeeded
            '4XX':
              description: Callback failed

and run with -p components.callbacks. In the JSON, the $ref to User has no path, so file not found if you try to find the corresponding file.

The stripping happens in oas2json.ts lines 51-54

https://github.com/nearform/openapi-transformer-toolkit/blob/aced54d3a61c326795b2a050adf9e8094ce0c336/src/commands/oas2json.ts#L51C1-L54C7

I could change that to get a path like ../components.schemas/Pet.json. But I'd like to know why it's the way it is to be sure I don't break something else or do something that breaks the intent of the tool. I'm assuming one wouldn't write something like $ref: '#/components/schemas/Pet/status.

I think that change would require changing json2ts to strip the paths because it supports components.schemas only. (Line 27 passes undefined to oas2json for propertiesToExport, so will only get components.schemas.)

Digging through this, I'm back to the idea that oas2tson is just oas2json plus the processJSON function to dereference and wrap the export const... part around the JSON.

ilteoood commented 2 months ago

It's made in this way to be easily imported by ajv, which doesn't need a path to import the file but just its title.

jmjf commented 2 months ago

Thank you. Another TIL -- why oas2tson duplicates code from other parts of the tool. Any day I can say, "TIL..." several times is a good day. :smile:

I think that pushes me toward prefixing names for everything but schemas to avoid collisions (in the current example, something like Pet.tson (schema), RequestBodies_Pet.tson, Paths_pet.tson) instead of using directories. More to come.

jmjf commented 2 months ago

Notes on where this seems to be going in oas2tson.ts.

COMPONENT_REF_REGEXP is same as in oas2json.ts

const INVALID_JSNAME_REGEXP = /[^A-Za-z0-9$_]/g

In processSchema

When exporting a header like X-Expires-After, what's done so far gets me X_Expires_After.ts with a valid name.

// vs. X-Expires-After -> invalid JS name
export const X_Expires_After = {
  description: "date in UTC when token expires",
  schema: { type: "string", format: "date-time" },
  title: "X-Expires-After",
  $id: "X_Expires_After.json",
} as const;

When prefixing is working, I expect headers_X_Expires_After.ts and export const headers_X_Expires_After .... I admit isn't normal JS/TS naming, but for oas2tson, using the base example openapi.yml, path Pet, schema Pet, and request body Pet produce colliding filenames and ref names and building directories to manage it seems questionable.

Currently failing looking for PetJSON-XML-Form.json (in /pet, $ref: '#/components/requestBodies/PetJSON-XML-Form'). I think adding the prefix and getting paths and webhooks in COMPONENTS_REF_REGEXP will solve that problem. That change should make the ref requestBodies_PetJSON_XML_FORM.json.

TBD:

jmjf commented 2 months ago

@ilteoood, @simoneb

Please look at the output I'm generating and give feedback.

Schema: https://github.com/jmjf/openapi-transformer-toolkit/blob/feat/make-oas2tson-broader/example/openapi_kitchen_sink.yml

Output: https://github.com/jmjf/openapi-transformer-toolkit/tree/feat/make-oas2tson-broader/example/output/oas2tson

Does this result seem reasonable given the constraints of JavaScript names?

Comparing the schema to the output:

The "kitchen sink" schema includes other changes vs. openapi.yml to exercise the code.

Dependent nodes of components must be included in the -p list. I'm running with -p components.headers,components.requestBodies,components.responses,components.parameters,components.links,components.callbacks,paths,webhooks, which I got to by adding to the list based on what failed until it stopped failing. That means somewhere in the paths there are references that depend on components/parameters and components/links.

Because the dependent nodes must be in the processed schema output for refs to resolve for dereferencing, I'm not sure how to get around that yet.

TODO: Figure out how these changes affect tests and update tests.

TBD: Why do I get paths__pet ( double _ )and webhooks_userCreated ( single )? And which is preferable for non-components nodes? (Answer: Because paths begin with /, which gets converted to `, thenpaths+_+pet. But the testwebhooksentry doesn't begin with/, sowebhooks++userCreated`.)

jmjf commented 2 months ago

Example output now includes a components.examples example (used in paths/pet put), we can generate.

-p paths works without listing all dependencies in the command line. To do this, oas2tson calls processSchema for all keywords it can support so processJSON will find all files it needs for refs and puts only keywords from -p in outputSchemasMetaData.

processSchema returns if schema is not an object. Previously, it called Object.entries(schema) and threw an error. This change is necessary for -p paths without dependencies. It also means -p paths,fred will output schemas and paths instead of failing on fred.

Added code to comment out tsType: "Date" in output because ajv in strict mode fails to compile schemas with tsType. The other option is to not add tsType in adaptSchema because oas2tson doesn't need it. (Carried over from oas2json, which adds tsType for oas2ts.) -- LMK if you prefer to remove tsType in adaptSchema.

Added notes to README that I believe are valuable to tool users. LMK if it should be in another part of the README.

I think the only remaining item for this issue is updating existing tests and adding new tests if needed.