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

Config option to only make cyclical struct fields pointers #2174

Merged

Conversation

ianling
Copy link
Contributor

@ianling ianling commented May 18, 2022

This PR #701 caused all struct fields whose type is a struct to be generated as pointers. This was necessary to resolve issues with cyclical relationships between structs (e.g. StructA contains a field of type StructB and vice versa) because those will cause compiler errors.

However this was overkill and also caused non-cyclical struct relationships to turn into pointers. For example:

type StructA {
    field_one: String!
}

type StructB {
    field_one: Int!
    field_two: StructA!
}

...would unnecessarily generate StructB.field_two with the type *StructA instead of just StructA.

This PR adds a config option that allows a user to retain the pointer change from the linked PR for cyclical relationships, but otherwise gqlgen will not change the type to a pointer for cases like the example I gave above.

The default behavior of gqlgen remains the same, so this will not affect any existing users unless they add struct_fields_always_pointers: false to their config.

Examples of behavior with this PR, using struct_fields_always_pointers: false:

  • Cyclical relationships generate pointers (matches current behavior)
type StructA {
    b: StructB!
}

type StructB {
    a: StructA!
}
type StructA struct {
    B *StructB
}
type StructB struct {
    A *StructA
}
  • Non-cyclical relationships do not generate pointers (new behavior)
type Name {
    first: String!
    last: String!
}

type Person {
    name: Name!
    age: Int!
}
type Name struct {
    First string
    Last string
}
type Person struct {
    Name Name
    Age int
}
  • Recursive structs generate pointers (matches current behavior)
type StructA {
    a: StructA!
}
type StructA struct {
    A *StructA
}

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@coveralls
Copy link

coveralls commented May 18, 2022

Coverage Status

Coverage increased (+0.09%) to 75.026% when pulling e15da34 on ianling:cyclical_struct_relationships into 4cdf702 on 99designs:master.

@ianling ianling marked this pull request as draft May 18, 2022 20:13
@ianling ianling force-pushed the cyclical_struct_relationships branch 2 times, most recently from 133f106 to ae1d2e4 Compare May 18, 2022 21:05
@ianling ianling marked this pull request as ready for review May 18, 2022 21:05
@ianling ianling marked this pull request as draft May 18, 2022 21:12
@ianling
Copy link
Contributor Author

ianling commented May 18, 2022

Need to update docs and examples, but is this change okay otherwise? @StevenACoffman

This will be a breaking change for any users of gqlgen whose schemas include non-cyclical struct fields (see my example of this above), as this change alters the generated structs in that case.

To make this less bad, we could make this behavior non-default using a config option, but I do think this should be the default behavior, as it is what a developer would expect to happen, in my opinion. That said, the slice pointer thing is also behind a config option and does not behave the way I would expect by default, so this would not be the only time where something unexpected happens that requires changing the config.

@StevenACoffman
Copy link
Collaborator

So this will cause a lot of churn for existing users if they don't specifically opt-in, so I'd ideally like this changed behavior to be a config option if possible.

@ianling ianling force-pushed the cyclical_struct_relationships branch from ae1d2e4 to 480886c Compare May 19, 2022 19:49
@ianling ianling changed the title Only make cyclical struct fields pointers Config option to only make cyclical struct fields pointers May 19, 2022
@ianling ianling marked this pull request as ready for review May 19, 2022 19:51
@ianling ianling force-pushed the cyclical_struct_relationships branch from 480886c to 6ad563d Compare May 19, 2022 19:55
@ianling
Copy link
Contributor Author

ianling commented May 19, 2022

I added a config option for this which defaults to gqlgen's behavior today without this PR, so that it doesn't affect any users unless they explicitly opt in to the new behavior using the config option.

I also added tests that validate the old behavior and the new behavior, and added the new config option to the config doc page. The examples did not need to be changed since the default behavior of gqlgen has not changed.

@StevenACoffman
Copy link
Collaborator

Can you clarify what you mean by "default to old behavior"? Before PR #701 was merged is one "old behavior", and before this PR is merged is a different "old behavior"?

@ianling
Copy link
Contributor Author

ianling commented May 20, 2022 via email

@ianling ianling force-pushed the cyclical_struct_relationships branch from 6ad563d to e15da34 Compare May 20, 2022 19:25
@ianling
Copy link
Contributor Author

ianling commented May 20, 2022

Fixed the golangci-lint issue...

@StevenACoffman StevenACoffman merged commit ddd825e into 99designs:master Jun 4, 2022
@ianling ianling deleted the cyclical_struct_relationships branch June 6, 2022 19:46
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.

3 participants