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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow json schema with circular refs to be converted to OpenAPI schema #3897

Merged
merged 1 commit into from Oct 24, 2019

Conversation

@raymondfeng
Copy link
Member

raymondfeng commented Oct 8, 2019

Fixes #3706

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

Copy link
Member

bajtos left a comment

The proposal looks reasonable, let's discuss few details.

@raymondfeng raymondfeng force-pushed the issue-3706 branch from 378c4ab to 1cec3d6 Oct 11, 2019
@raymondfeng raymondfeng requested a review from bajtos Oct 12, 2019
benchmarkId: {type: 'string'},
color: {type: 'string'},
},
definitions: {ReportState: {$ref: '#/components/schemas/ReportState'}},

This comment has been minimized.

Copy link
@bajtos

bajtos Oct 15, 2019

Member

I find it weird to have a definition that's referencing itself, it's effectively creating a cyclic reference/infinite loop. Can we remove this entry please?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 15, 2019

Author Member

I added a test without definitions.

@raymondfeng raymondfeng force-pushed the issue-3706 branch 2 times, most recently from 4c37338 to 36c7dc0 Oct 15, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Oct 15, 2019

@bajtos PTAL.

if (schema != null) return schema;

const result: SchemaObject = {
[converted]: false,

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Oct 17, 2019

Contributor

'x-loopback-converted' is never set to true. It is just a placeholder to verify the result object. why not directly declare it as a field here ?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Oct 17, 2019

Author Member

x-* is extensibility elements for OpenAPI specs. We delete the flag (equivalent to true) after the schema object is fully converted.

Copy link
Member

bajtos left a comment

Almost there!

@raymondfeng raymondfeng force-pushed the issue-3706 branch from 36c7dc0 to b66c221 Oct 17, 2019
@raymondfeng raymondfeng requested a review from bajtos Oct 17, 2019
@raymondfeng raymondfeng force-pushed the issue-3706 branch from b66c221 to 2913529 Oct 21, 2019
@raymondfeng raymondfeng merged commit cd5ca92 into master Oct 24, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 91.831%
Details
@raymondfeng raymondfeng deleted the issue-3706 branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.