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

refactor: cleaning up the messy getSchema library and other improvements #581

Merged
merged 7 commits into from
Jan 24, 2022

Conversation

erunion
Copy link
Member

@erunion erunion commented Jan 17, 2022

🧰 Changes

  • 🗑 Deleting Deprecating the getSchema library.
  • 🆕 Added an Operation.getRequestBodyMediaTypes() accessor that returns a list of every media type that's supported in the operations request body.
  • 🆕 Operation.getRequestBody() now supports retrieval of either the first JSON-like request body, or whatever is first available, if you don't supply a media type to retrieve.
    • This completely replaces the much-maligned getSchema library.
  • 🆕 Operation.getParameters() now supports retrieval and merging of common parameters.
    • I've also added an override into the Callback class so it can contextually handle common callback parameters like it already supports for common summary and description.
  • 🆕 Operation.hasParameters() is a new shortcut method for running a length check on Operation.getParameters().
  • ♻️ getParametersAsJsonSchema() has been refactored to accept an Operation, or Callback, instance instead of an OperationObject object.
    • It's also been updated internally to use the new hasParameters(), getParameters(), and getRequestBody() accessors.

🧬 QA & Testing

  • Everything here look ok?
  • All tests still passing?

@erunion erunion added enhancement New feature or request refactor Issues about tackling technical debt labels Jan 17, 2022
@@ -5,160 +5,53 @@ Array [
Object {
"label": "Body Params",
"schema": Object {
"$ref": "#/components/schemas/Pet",
Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored the test that this snapshot is part of to dereference the petstore first, so these refs are no longer getting added into compiled JSON Schema.

@@ -1,55 +0,0 @@
import type * as RMOAS from '../rmoas.types';
Copy link
Member Author

Choose a reason for hiding this comment

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

👋 smell ya later

const commonParams = getCommonParams();

if (commonParams.length !== 0) {
const commonParamsNotInParams = commonParams.filter((param: RMOAS.ParameterObject) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This code now lives within dedupeCommonParameters and is run as part of Operation.getParameters().

@@ -162,15 +157,7 @@ export default function getParametersAsJsonSchema(
};
}

function getCommonParams() {
if (api && 'paths' in api && path in api.paths && 'parameters' in api.paths[path]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is just part of Operation.getParameters() now.

Comment on lines +309 to +313
const components = transformComponents();

const typeKeys = Object.keys(types);
return [getRequestBody()]
.concat(...getParameters())
return [transformRequestBody()]
.concat(...transformParameters())
Copy link
Member Author

@erunion erunion Jan 17, 2022

Choose a reason for hiding this comment

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

Renamed all these get* methods to be transform* so it's clearer what this is doing.

} else if ('examples' in requestBody) {
examples.push({ examples: requestBody.examples });
// @todo this `examples` array be `Array<RMOAS.SchemaObject>` but that doesn't like the `examples` schema
const examples = [] as Array<any>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This Array<any> is annoying but I had a lot of trouble with this block and the prevSchemas type in toJSONSchema.

For example if this is just const examples = []; TS throws this:

src/operation/get-parameters-as-json-schema.ts:142:73 - error TS2322: Type '({ example: any; } | { examples: Record<string, ReferenceObject | ExampleObject> | { [media: string]: ReferenceObject | ExampleObject; }; })[]' is not assignable to type 'PrevSchemasType'.
  Type '{ example: any; } | { examples: Record<string, ReferenceObject | ExampleObject> | { [media: string]: ReferenceObject | ExampleObject; }; }' is not assignable to type 'SchemaObject'.
    Type '{ examples: Record<string, OpenAPIV3_1.ReferenceObject | OpenAPIV3.ExampleObject> | { [media: string]: OpenAPIV3.ReferenceObject | OpenAPIV3.ExampleObject; }; }' is not assignable to type 'SchemaObject'.
      Type '{ examples: Record<string, OpenAPIV3_1.ReferenceObject | OpenAPIV3.ExampleObject> | { [media: string]: OpenAPIV3.ReferenceObject | OpenAPIV3.ExampleObject; }; }' is not assignable to type 'JSONSchema7 & { deprecated?: boolean; readOnly?: boolean; writeOnly?: boolean; example?: unknown; examples?: unknown[]; nullable?: boolean; xml?: unknown; externalDocs?: unknown; components?: ComponentsObject; 'x-readme-ref-name'?: string; }'.
        Type '{ examples: Record<string, OpenAPIV3_1.ReferenceObject | OpenAPIV3.ExampleObject> | { [media: string]: OpenAPIV3.ReferenceObject | OpenAPIV3.ExampleObject; }; }' is not assignable to type 'JSONSchema7'.
          Types of property 'examples' are incompatible.
            Type 'Record<string, ReferenceObject | ExampleObject> | { [media: string]: ReferenceObject | ExampleObject; }' is not assignable to type 'JSONSchema7Type'.
              Type 'Record<string, ReferenceObject | ExampleObject>' is not assignable to type 'JSONSchema7Type'.
                Type 'Record<string, ReferenceObject | ExampleObject>' is not assignable to type 'string'.

142     const cleanedSchema = toJSONSchema(requestSchema, { globalDefaults, prevSchemas: examples, refLogger });
                                                                            ~~~~~~~~~~~

  src/lib/openapi-to-json-schema.ts:33:3
    33   prevSchemas?: PrevSchemasType;
         ~~~~~~~~~~~
    The expected type comes from property 'prevSchemas' which is declared here on type 'toJsonSchemaOptions'


Found 1 error.

If it's const examples = [] as Array<RMOAS.SchemaObject>; TS throws this:

src/operation/get-parameters-as-json-schema.ts:136:23 - error TS2322: Type 'Record<string, ReferenceObject | ExampleObject> | { [media: string]: ReferenceObject | ExampleObject; }' is not assignable to type 'unknown[] | (any[] & unknown[]) | (JSONSchema6Type[] & unknown[]) | (JSONSchema7Type & unknown[])'.
  Type 'Record<string, ReferenceObject | ExampleObject>' is not assignable to type 'unknown[] | (any[] & unknown[]) | (JSONSchema6Type[] & unknown[]) | (JSONSchema7Type & unknown[])'.
    Type 'Record<string, ReferenceObject | ExampleObject>' is not assignable to type 'string & unknown[]'.
      Type 'Record<string, ReferenceObject | ExampleObject>' is not assignable to type 'string'.

136       examples.push({ examples: mediaTypeObject.examples });
                          ~~~~~~~~

  node_modules/openapi-types/dist/index.d.ts:80:9
    80         examples?: OpenAPIV3.BaseSchemaObject['example'][];
               ~~~~~~~~
    The expected type comes from property 'examples' which is declared here on type 'SchemaObject'


Found 1 error.

I get it's a problem with the ReferenceObject, as usual, I'm just not sure how to fight that battle here.

Copy link
Contributor

@Dashron Dashron Jan 24, 2022

Choose a reason for hiding this comment

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

I dug into this a bit, I don't think it's a ReferenceObject issue. I think it's a legit type inconsistency, and very possibly a bug highlighted by ts.

If we go with the second option, Array<RMOAS.SchemaObject> it expects the examples to be Array<any>, because that's the spec for JSON schema.

But mediaTypeObject.exexamples isn't Array<any> it's Record<string, ReferenceObject | ExampleObject> as defined in OAS.

It seems line 136 of get-parameters-as-json-schema is mixing up JSON Schema examples and OAS examples.

Line 134 works because it's actually grabbing the value via example, but 136 needs to grab the value prop off each example.

e.g.

    // @todo this `examples` array be `Array<RMOAS.SchemaObject>` but that doesn't like the `examples` schema
    const examples = [] as Array<RMOAS.SchemaObject>;
    if ('example' in mediaTypeObject) {
      examples.push({ example: mediaTypeObject.example });
    } else if ('examples' in mediaTypeObject) {
      examples.push({
        examples: Object.values(mediaTypeObject.examples).map((example: RMOAS.ExampleObject) => example.value),
      });
    }

@erunion erunion marked this pull request as ready for review January 17, 2022 02:30
@erunion erunion requested review from a team, Dashron and julshotal and removed request for a team January 17, 2022 02:30
@@ -645,6 +714,16 @@ export class Callback extends Operation {

return this.schema?.description ? this.schema.description.trim() : undefined;
}

getParameters(): Array<RMOAS.ParameterObject> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Like with the getSummary and getDescription overrides that the Callback class has it also needs one for parameters in order to support common parameters as callback common parameters exist in the schema above the callback (this.parentSchema), and not at the this.api.paths[method] level that they're at for standard operations.

A bit annoying but there's good test coverage in place.

Copy link
Contributor

@Dashron Dashron left a comment

Choose a reason for hiding this comment

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

looks good, let's chat about my comment and once that's resolved 👍 . seems much cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants