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

Reject operation or argument names that are Go keywords #195

Merged
merged 2 commits into from
May 12, 2022

Conversation

benjaminjkraft
Copy link
Collaborator

@benjaminjkraft benjaminjkraft commented May 12, 2022

GraphQL is pretty restrictive about its identifiers, so for the most
part we can and do safely use GraphQL identifiers in the Go we generate
with attention only to conflicts with other such identifiers. But we do
need to check one thing, which is that the identifier isn't a Go
keyword. (If it is, the generated code will almost certainly fail to
compile, but often with a confusing error message.) In this commit I add
such checks.

The most likely place to run into trouble here is argument names, which
are often one word and are used as-is. Operation names, if unexported,
can also be keywords, although in practice they're usually multiword.
Field names are always exported, thus safe. Generated type names are
always prefixed, camel-cased, with the operation name, so they always
contain an uppercase letter (even if the operation name is lowercase),
but type-names specified by typename may collide, so we check those.
In theory we could check type-names specified by bind, but these must
be defined by the user, so their code will already fail to compile, so I
didn't bother. I think that's all the places to consider, although it's
hard to be sure. In summary, we check argument names, operation names,
and user-specified type names.

Fixes #194.

Test plan: make check

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

GraphQL is pretty restrictive about its identifiers, so for the most
part we can and do safely use GraphQL identifiers in the Go we generate
with attention only to conflicts with other such identifiers. But we do
need to check one thing, which is that the identifier isn't a Go
keyword. (If it is, the generated code will almost certainly fail to
compile, but often with a confusing error message.) In this commit I add
such checks.

The most likely place to run into trouble here is argument names, which
are often one word and are used as-is.  Operation names, if unexported,
can also be keywords, although in practice they're usually multiword.
Field names are always exported, thus safe.  Generated type names are
always prefixed, camel-cased, with the operation name, so they always
contain an uppercase letter (even if the operation name is lowercase),
but type-names specified by `typename` may collide, so we check those.
In theory we could check type-names specified by `bind`, but these must
be defined by the user, so their code will already fail to compile, so I
didn't bother.  I think that's all the places to consider, although it's
hard to be sure.  In summary, we check argument names, operation names,
and user-specified type names.

Test plan: make check
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking!

So sad I won't be able to use goto as a graphql variable name...

@benjaminjkraft
Copy link
Collaborator Author

Never fear, you can still use goTo!

@benjaminjkraft benjaminjkraft merged commit 39cd158 into main May 12, 2022
@benjaminjkraft benjaminjkraft deleted the benkraft.keywords branch May 12, 2022 22:00
@JuanitoFatas
Copy link

Sorry to ask here. I was hitting by this and wonder what if a mutation requires an input that is the same name as Go Keywords (e.g. type), is there a way to get around this?

mutation Mutation {
  resourceCreate(name: "Test", type: "type-1") {
    resource {
      name
      type
    }
  }
}

@benjaminjkraft
Copy link
Collaborator Author

That query as written should work, I believe. Of course the query you actually want is probably something like

mutation Mutation($name: String, $type: String) {
  resourceCreate(name: $name, type: $type) {
    resource {
      name
      type
    }
  }
}

In such a case there's no restriction on the variable name -- you could just as easily name it $typ or $type_. If you are still having problems, please open a new issue with a reproduction of the problem!

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.

Generate fails when building a mutation that takes enum as input.
3 participants