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

Infinite loop on recursive oneOf component #451

Open
smeyffret opened this issue Mar 19, 2022 · 4 comments
Open

Infinite loop on recursive oneOf component #451

smeyffret opened this issue Mar 19, 2022 · 4 comments

Comments

@smeyffret
Copy link

smeyffret commented Mar 19, 2022

Describe the bug
Given a schema that has a recursive oneOf element, openapi-to-graphql fails to generate the graphql schema with a Maximum call stack size exceeded error. I suspect there is an infinite loop while trying to convert the recursive element.

I'm using Smithy (union type) to generate the openapi file (see section below for openapi file). I came across that issue when trying to model arithmetic expressions. Here is an example with recursive oneOf:

union Expression {
    not: Expression,
    value: String
}

To Reproduce
Steps to reproduce the behavior:

  1. Download this minimal openapi service definition example RecursionExample.swagger.api.json.txt
  2. Run openapi-to-graphql RecursionExample.swagger.api.json.txt (using "openapi-to-graphql-cli": "2.6.3")
  3. See error Maximum call stack size exceeded
RangeError: Maximum call stack size exceeded
    at Function.enabled (node_modules/@amzn/openapi-to-appsync/node_modules/debug/src/common.js:198:18)
    at Function.get [as enabled] (node_modules/@amzn/openapi-to-appsync/node_modules/debug/src/common.js:123:53)
    at debug (node_modules/@amzn/openapi-to-appsync/node_modules/debug/src/common.js:66:15)
    at createOrReuseOt (node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3558:13)
    at getGraphQLType (node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3478:20)
    at node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3649:20
    at Array.map (<anonymous>)
    at createOrReuseUnion (node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3648:60)
    at getGraphQLType (node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3488:20)
    at createFields (node_modules/@amzn/openapi-to-appsync/node_modules/openapi-to-graphql/dist/index.js:3846:28)

Expected behavior
I'd expect openapi-to-graphql to not fail to generate the schema, and to, by order of preference:

  1. generate a union type (preferred)
  2. generate a plain structure (if not possible)
  3. generate a JSON blob (last resort)

Example of ideal union type that should be generated (if possible):

union Expression = ValueExpression | NotExpression
type NotExpression {
    not: Expression!
}
type ValueExpression {
    value: String!
}

Example of alternative that would still be acceptable:

type Expression {
    not: Expression
    value: String
}

This is not great as we lose the concept of union type, but it is better than having the whole schema generation failing.

Finally, if none of those are possible, JSON would be a last resort.

@smeyffret
Copy link
Author

Any idea / suggestion on how to work around this issue?

@smeyffret
Copy link
Author

I might have found the root cause, checkAmbiguousMemberTypes needs to be moved after assigning def.graphQLType (here) so that when fetching for the fields of the current def, it won't try to re-create the union type.

I tried with my schema and it worked with that change. I could submit a pull request but I'm not too familiar with github and I don't know how to do that (I tried creating a separate branch and pushing it but I don't have permission). I also couldn't write a proper unit tests for that, if the schema is too simple then GraphQL will complain:

Schema must contain uniquely named types but contains multiple types named "Expression".

I could spend more time trying to write a proper unit-test, but I'd like to have confirmation from someone first, since that issue doesn't seem to have much traction.

@smeyffret
Copy link
Author

I got some help and figured out how to submit a pull request (#457).

@smeyffret
Copy link
Author

Any update?

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

1 participant