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

Fix a minor bug in circular reference detection of deeply nested input objects #475

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Fix a minor bug in circular reference detection of deeply nested input objects #475

merged 1 commit into from
Mar 15, 2021

Conversation

mcasper
Copy link
Contributor

@mcasper mcasper commented Feb 26, 2021

Issue

When generating input object types with elm-graphql, some input objects get incorrectly flagged as containing loops, and get wrapped in a custom type.

Reference issue: #33

Steps to repro

Create an input object type that contains at least 3 layers of other input objects.

Example (in graphql schema notation):

input TypeOne {
  typeTwo: TypeTwo!
}
input TypeTwo {
  typeThree: TypeThree!
}
input TypeThree {
  typeFour: TypeFour!
}
input TypeFour {
  name: String!
}

Generate this schema using elm-graphql. TypeOne will be generated as:

type alias TypeOneRaw =
    { typeTwo = TypeTwo }

type TypeOne
    = TypeOne TypeOneRaw

Root Cause

The bug originates when we descend down the type trees to inspect the fields of the first input object that are input objects themselves. We correctly push the first input object's name onto "visitedClasses", but also pass the first input object's name as the currently evaluating name in the next loop of the recursion, when it should instead be the second layer child that we're checking.

Fix

This PR updates the logic to pass the correct currently evaluation input object's name, and adds a test to guard against regressions.

…t objects

\# Issue

When generating input object types with elm-graphql, some input objects get incorrectly flagged as containing loops, and get wrapped in a custom type.

Reference issue: #33

\# Steps to repro

Create an input object type that contains at least 3 layers of other input objects.

Example (in graphql schema notation):
```graphql
input TypeOne {
  typeTwo: TypeTwo!
}
input TypeTwo {
  typeThree: TypeThree!
}
input TypeThree {
  typeFour: TypeFour!
}
input TypeFour {
  name: String!
}
```

Generate this schema using elm-graphql. `TypeOne` will be generated as:
```elm
type alias TypeOneRaw =
    { typeTwo = TypeTwo }

type TypeOne
    = TypeOne TypeOneRaw
```
\# Root Cause

The bug originates when we descend down the type trees to inspect the fields of the first input object that are input objects themselves. We correctly push the first input object's name onto "visitedClasses", but also pass the first input object's name as the currently evaluating name in the next loop of the recursion, when it should instead be the second layer child that we're checking.

\# Fix

This PR updates the logic to pass the correct currently evaluation input object's name, and adds a test to guard against regressions.
@mcasper
Copy link
Contributor Author

mcasper commented Mar 15, 2021

@dillonkearns is there anything I can do to help move this along?

@dillonkearns
Copy link
Owner

Hey @mcasper! Sorry for the delay. This is A+ work, thank you so much for the excellent description and test cases 🙏

@dillonkearns dillonkearns merged commit cf26609 into dillonkearns:master Mar 15, 2021
@dillonkearns
Copy link
Owner

This fix is now live with version 4.2.1 of the NPM package 🎉

https://github.com/dillonkearns/elm-graphql/blob/master/CHANGELOG-NPM-PACKAGE.md#421---2021-03-15

@mcasper
Copy link
Contributor Author

mcasper commented Mar 16, 2021

Fantastic, thank you!

@mcasper mcasper deleted the deeply-nested-input-objects-gen-bug branch March 16, 2021 03:55
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.

2 participants