Skip to content

Conversation

@nth-commit
Copy link

@nth-commit nth-commit commented Nov 13, 2019

Made some changes to the schema serialization, so that the intention of the caller is preserved. Specifically, whether or not they want reference schemas to be inlined.

I broke some tests around OpenApiComponents, which is another example of the behaviour change, but I think the tests relied on the bug 😄

More info in #428

@msftclas
Copy link

msftclas commented Nov 13, 2019

CLA assistant check
All CLA requirements met.

@nth-commit
Copy link
Author

Haven't tested properly but pretty sure this will infinite loop on circular references. See further comments on: #428

@nth-commit nth-commit force-pushed the 428-serializable-recursive-schema-references-2 branch from 06a3ae7 to f72eaf9 Compare November 14, 2019 23:29
@DerekChasse
Copy link

I'm feeling that, depending on what @darrelmiller comes back with, this could be considered a breaking change. I'm still confused as to the intent of IOpenApiReferenceable and the intent of SerializeWithoutReference.

From name alone, it does sound like it should resolve and expand references, but perhaps that's not the intent. This begs the question on how this code would work with external references? There are already many open bugs/questions around this. Also, when is enough? Are we chasing and expanding references until all are resolved? What is the performance impact if we are resolving and expanding references that are 10 children deep? 20? 100?

For a more gentle approach, I would recommend adding something like an optional OpenApiWriterSettings argument to the constructors of OpenApiJsonWriter and OpenApiYamlWriter (Really in OpenApiWriterBase) This writer settings class would contain indicators on what to do with local references, and other things in the future. This is a similar pattern which Newtonsoft takes with their JsonSerializerSettings class.

@darrelmiller
Copy link
Member

@DerekChasse @nth-commit I'm going to try and get this fix into the 1.2 release. I agree that having a setting for that writer that choose whether to write out references as references or inline is an ideal solution. We will also have to have a further qualification to decide what to do with external references. I do something very similar in the reader settings.

The SerializeWithoutReference was just a special case to allow us to serialize top level components. However, that has a bug at the moment that I need to fix first. Currently the following is not supported properly:

openapi: 3.0.0
info:
  title: blag
  version: 1.0.0
paths:
  /: 
    get:
      responses:
        200:
          $ref: '#/components/responses/bob'
components:
  responses:
    bob:
      $ref: '#/components/responses/otherbob'
    otherbob:
      description: ok

This scenario is not that common for OpenAPI descriptions in a single file but it is much more common when referencing schemas in external files. Once I have fixed this we can look into enabling inlining of refs. The slight challenge is that because I didn't use the Visitor for the writer, we will have to duplicate the loop detection into the writer. That shouldn't be a huge deal.

@nth-commit
Copy link
Author

A few years later... Just bumped into this. So glad this works now, huge thanks @darrelmiller 🙌

@nth-commit nth-commit closed this Jul 26, 2023
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.

4 participants