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

Recursive/circular logic not correct - 2.0.0-alpha.32 #585

Closed
viralanomaly opened this issue Jul 31, 2018 · 8 comments
Closed

Recursive/circular logic not correct - 2.0.0-alpha.32 #585

viralanomaly opened this issue Jul 31, 2018 · 8 comments

Comments

@viralanomaly
Copy link
Contributor

viralanomaly commented Jul 31, 2018

I've included a "full" spec that reproduces this problem. I expect Option 3 of schema "My Thing" to render, but it is labeling itself as recursive. It happens on any item in the allOf that has a reference that was already resolved elsewhere in that particular allOf object, I believe. This is taking away most of the utility behind using objects with allOf for me.

openapi: 3.0.1
info:
  version: 0.0.1
  title: My cool API

paths:
  # My Thing
  /mything:
    post:
      tags:
      - My thing
      summary: Add My Thing
      description: Add a new my thing
      operationId: api.addMyThing
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/MyThingCreate'
      responses:
        '201':
          description: Created
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/MyThingCreateResponse'

components:
  schemas:
    ThingA:
      type: object
      properties:
        thinga:
          type: string
    ThingB:
      type: object
      properties:
        thingb:
          type: string
    ThingC:
      type: object
      properties:
        thingc:
          type: string
    MyThing:
      allOf:
        - oneOf:
          - allOf:
            - title: Option 1
            - $ref: '#/components/schemas/ThingB'
          - allOf:
            - title: Option 2
            - $ref: '#/components/schemas/ThingA'
          - allOf:
            - title: Option 3
            - $ref: '#/components/schemas/ThingB'
            - $ref: '#/components/schemas/ThingA'
        - $ref: '#/components/schemas/ThingC'

    MyThingCreate:
      allOf:
        - $ref: '#/components/schemas/MyThing'
        - required:
          - thinga
          - thingb
    MyThingCreateResponse:
      type: object
      properties:
        thing1:
          type: string


  securitySchemes:
    bearerAuth:
      type: http
      scheme: bearer
      bearerFormat: JWT
security:
  - bearerAuth: []

image

@viralanomaly
Copy link
Contributor Author

If I remove ThingC and the outermost allOf within "MyThing", this renders as expected.

@viralanomaly
Copy link
Contributor Author

Is there a way to set forceCircular outside of this file? If I could just set it, my spec loads much better.

This still doesn't fix the fact that the code is assuming a circular reference just because a reference has already been visited.

diff --git a/src/services/OpenAPIParser.ts b/src/services/OpenAPIParser.ts
index 8037705..3bace31 100644
--- a/src/services/OpenAPIParser.ts
+++ b/src/services/OpenAPIParser.ts
@@ -146,7 +146,7 @@ export class OpenAPIParser {
    * @param obj object to dereference
    * @param forceCircular whether to dereference even if it is cirular ref
    */
-  deref(obj: OpenAPIRef | T, forceCircular: boolean = false): T {
+  deref(obj: OpenAPIRef | T, forceCircular: boolean = true): T {
     if (this.isRef(obj)) {
       const resolved = this.byRef(obj.$ref)!;
       const visited = this._refCounter.visited(obj.$ref);
@@ -183,7 +183,7 @@ export class OpenAPIParser {
   mergeAllOf(
     schema: OpenAPISchema,
     $ref?: string,
-    forceCircular: boolean = false,
+    forceCircular: boolean = true,
   ): MergedOpenAPISchema {
     schema = this.hoistOneOfs(schema);

@RomanHotsiy
Copy link
Member

@viralanomaly, thanks for detailed issue report!

Is there a way to set forceCircular outside of this file? If I could just set it, my spec loads much better.

No, I am working on a proper fix. It should be released in the next alpha.

@RomanHotsiy
Copy link
Member

@viralanomaly I checked it out and it seems I need to rewrite circular detecting logic completely. It's complex right and unmaintainable :(

I have an idea how to do it properly but it will take some time.

Expect this to be fixed at the end of the next week.

@viralanomaly
Copy link
Contributor Author

The circular logic looked pretty... special. Thanks for looking into it!

@viralanomaly
Copy link
Contributor Author

Have you made any progress on this, @RomanGotsiy ?

@RomanHotsiy
Copy link
Member

Not really. Before that, I have to make some changes to APIDevTools/json-schema-ref-parser and they are not so obvious.
I made a shortcut by patching the lib and verified it works but now I need to implement it properly and made a PR.

Already discussed possibilities with lib author so it is moving.

Thanks for reminding 🙌

@viralanomaly
Copy link
Contributor Author

Did this fix expose forceCircular as well, or only fix the circular logic?

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

No branches or pull requests

2 participants