Skip to content

Ensure allOf and properties are merged into SchemaObject #903

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

Closed
wants to merge 1 commit into from

Conversation

omonk
Copy link
Contributor

@omonk omonk commented Aug 1, 2024

Description

Ensures that schema.allOf and schema.properties are merged together in the createSchema method. See linked issue below for more information

Motivation and Context

Fixes #902

How Has This Been Tested?

Tests have been added, I've also run gen-all on the demo repo with the new changes and everything compiles successfully

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • [ x ] I have updated the documentation accordingly.
  • [ x ] I have read the CONTRIBUTING document.
  • [ x ] I have added tests to cover my changes if appropriate.
  • [ x ] All new and existing tests passed.

Copy link

github-actions bot commented Aug 1, 2024

Visit the preview URL for this PR (updated for commit d865093):

https://docusaurus-openapi-36b86--pr903-7ss8i0sx.web.app

(expires Sat, 31 Aug 2024 14:54:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@sserrata
Copy link
Member

sserrata commented Aug 1, 2024

Hi @omonk, thanks for the contribution! I think this implementation makes sense but I'm also wondering if we could follow the approach used by createNodes, where, instead of returning after each schema/node match, we collect and store them in an array before ultimately returning them all.

@sserrata
Copy link
Member

sserrata commented Aug 2, 2024

Hi @omonk,upon second inspection, I was reminded of a couple of things:

  1. In createEdges we are processing allOf before properties
  2. In createEdges we assume we are all the "edge" and return immediately on first match of a schema type

After switching the order, i.e. processing properties before allOf I am seeing the entire schema rendered.

Parsing OpenAPI schemas has, without a doubt, been the most challenging aspect of this project. That said, thanks to your contribution, it's clear the library could benefit from more tests and potentially more variations of example OpenAPI schemas to provide greater coverage of various cases.

@sserrata
Copy link
Member

sserrata commented Aug 2, 2024

Hi @omonk, I created #904 to address the original issue you reported. Although your solution technically worked, I believe that mergeAllOf should be reserved only for allOf and not include any sibling or same-level schemas. Interested to hear your feedback on the proposed fix as well as the addition of tests. Thanks!

@sserrata sserrata closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema $ref fails to combine allOf and properties for nested properties
2 participants