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

OAS 3.1 for JSON Schemas #530

Merged
merged 24 commits into from
Dec 6, 2021
Merged

OAS 3.1 for JSON Schemas #530

merged 24 commits into from
Dec 6, 2021

Conversation

Dashron
Copy link
Contributor

@Dashron Dashron commented Oct 29, 2021

🧰 Changes

This is all the work necessary to support OAS 3.1 for JSON schemas.

pre OAS 3.1 your JSON schema could only be draft 04. 3.1 your JSON schema can be anything, as determined by the $schema value in your schema, the value of jsonSchemaDialect or the default, draft 2020-12.

These updates to the OAS library now ensure that the schemas you are working with always have the accurate $schema value, following the previously mentioned chain of priorities.

Along the way I converted the following to typescript

  • operation
  • get-parameters-as-json-schema

🧬 QA & Testing

  • tests

@Dashron
Copy link
Contributor Author

Dashron commented Nov 22, 2021

There's some really ugly typing going on in here, but let's get the typescript conversion over before we do code cleanup. I gave code cleanup a shot and it was really problematic.

Thanks for the help at the end there @erunion

@Dashron Dashron marked this pull request as ready for review November 22, 2021 21:14
@Dashron Dashron requested a review from erunion November 29, 2021 17:17
@erunion erunion added the enhancement New feature or request label Dec 2, 2021
JSONSchema7,
JSONSchema7Definition,
JSONSchema7TypeName,
} from '@typescript-eslint/experimental-utils/dist/json-schema';
Copy link
Member

Choose a reason for hiding this comment

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

Should this be @types/json-schema? If not, can that dependency get pulled from package.json?

Comment on lines 15 to 16
expect(operation.api).toStrictEqual(petstore);
expect(operation['api']).toStrictEqual(petstore);
Copy link
Member

Choose a reason for hiding this comment

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

Why the change to not use dot notation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At one point api was private, and this bypasses typescript's public/private check. It looks like it's no longer private though so it works with dot notation.

Copy link
Member

Choose a reason for hiding this comment

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

I think there's also a getApi() accessor on Operation if you want to use that instead.

@@ -147,7 +164,9 @@ function searchForExampleByPointer(pointer, examples = []) {
}

// Prevent us from crashing if `examples` is a completely empty object.
const ex = schema.examples[keys.shift()];
const ex = schema.examples[keys.shift() as unknown as number];
// const ex = schema.examples[0];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// const ex = schema.examples[0];

schema.enum = [...new Set(schema.enum)];
if (RMOAS.isSchema(schema, isPolymorphicAllOfChild) && 'enum' in schema && Array.isArray(schema.enum)) {
// If we ever target ES6 for typescript we can drop this array.from. https://stackoverflow.com/questions/33464504/using-spread-syntax-and-new-set-with-typescript/56870548
schema.enum = [...Array.from(new Set(schema.enum))];
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to do the spread operator here.

Suggested change
schema.enum = [...Array.from(new Set(schema.enum))];
schema.enum = Array.from(new Set(schema.enum));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird, that all came from fixing some typing issues, looks to still be fine typewise without it.

src/operation.ts Outdated
identifier: string;

constructor(
api: RMOAS.OASDocument,
oas: Oas | RMOAS.OASDocument,
Copy link
Member

Choose a reason for hiding this comment

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

This seems off because the OAS constructor can't accept a copy of itself as its API definition, and we only ever pass this.api when we create a new instance of this class.

});
}

const oasTags = Object.fromEntries(oasTagMap);
Copy link
Member

Choose a reason for hiding this comment

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

Why the change to use Object.fromEntries here instead of checking .has() on the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember specifically, but I'm sure it's typing related. Happy to mess around with restoring the old version tomorrow morning if you'd like.

if (typeof components[componentType] === 'undefined') {
components[componentType] = {};
}
// Typescript is INCREDIBLY SLOW parsing this one line. I think it's because of the large variety of types that that object could represent
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Typescript is INCREDIBLY SLOW parsing this one line. I think it's because of the large variety of types that that object could represent
// @fixme Typescript is INCREDIBLY SLOW parsing this one line. I think it's because of the large variety of types that that object could represent

if (param.name && param2.name) {
return param.name === param2.name && param.in === param2.in;
} else if (param.$ref && param2.$ref) {
if ((param as RMOAS.ParameterObject).name && (param2 as RMOAS.ParameterObject).name) {
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to clean this code up by typing param on line 215 with param: RMOS.ParameterObject? No TS failures here:

Screen Shot 2021-12-01 at 6 54 10 PM

type: 'object',
properties: {},
};

Object.keys(headers).forEach(key => {
if (headers[key] && headers[key].schema) {
if (headers[key] && (headers[key] as OpenAPIV3.HeaderObject | OpenAPIV3_1.HeaderObject).schema) {
Copy link
Member

Choose a reason for hiding this comment

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

Might want to make a HeaderObject in rmoas.types that shortcuts having to repeat OpenAPIV3.HeaderObject | OpenAPIV3_1.HeaderObject.

@erunion
Copy link
Member

erunion commented Dec 2, 2021

I also made a couple changes for minor things I found, you can see all my changes here: https://github.com/readmeio/oas/compare//58d150e746f52e9a25fdfa4d27424ebce94b412d...1841ce8?expand=1

  • Pulled back some code docs that got deleted
  • Moved import type declarations to the top of the files they're in so we're consistent
  • Moved the ResponseExamples and RequestBodyExamples exports back to using the exports from get-*-examples.ts because they're already there. Also removed the | false declaration that got added because those arrays of examples will never have a falsy entry because we do .filter(Boolean) on it.

@Dashron
Copy link
Contributor Author

Dashron commented Dec 2, 2021

Just pushed up fixes for everything. LMK what you think.

@erunion erunion merged commit 91475da into main Dec 6, 2021
@erunion erunion deleted the feat/oas31SupportForJsonSchema branch December 6, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants