Khan / genqlient

a truly type-safe Go GraphQL client
MIT License
1.02k stars 99 forks source link

`convertDefinition` failing when `name` == concatenated `namePrefix` #305

Open Ynng opened 5 months ago

Ynng commented 5 months ago

Describe the bug

I am encountering "conflicting definition" errors when using specific GraphQL field and type names with genqlient. The error message is as follows:

...../schema.graphql:13: conflicting definition for FooBarA; this can indicate either a genqlient internal error, a conflict between user-specified type-names, or some very tricksy GraphQL field/type names: expected 2 fields, got 1 exit status 1 main.go:9: running "go": exit status 1

To Reproduce The issue occurs under specific conditions:

  1. The schema includes a union whose name is the concatenation of a parent type's name and its field names in camel case.
  2. The query includes fragments named identically to the types:

For a clearer understanding and a practical demonstration, I prepared a minimal reproducible example here: https://github.com/Ynng/genqlient-bug

Expected behavior I expect the code generation process to successfully handle these scenarios without encountering conflicting definition errors.

genqlient version v0.6.0

Additional Information

My guess is that the issue arises when convertDefinition() encounters a name that is the same as namePrefix list concatenated and formatted.

This causes problems down the road for Unions, leading to duplicate "UnionNameImplName" definitions. I think the issue can be traced back to these two lines:

https://github.com/Khan/genqlient/blob/28aafc79a9f9065f7cb7e8bb2df49e5795896aeb/generate/convert.go#L465 https://github.com/Khan/genqlient/blob/28aafc79a9f9065f7cb7e8bb2df49e5795896aeb/generate/convert.go#L496

These lines inadvertently create two conflicting definitions: one from "UnionName + ImplName" and another from "Union + Name + ImplName."

benjaminjkraft commented 5 months ago

Wow, good find. That sure is a way to construct a collision without any type names that are too weird! I think your analysis of the name is approximately right, but maybe a simpler way to see it is simply that FooBar is both fragment FooBar and the field bar on fragment Foo. (The fragment-interface itself should conflict too, I think we just happen to hit the implementation types first.)

You should be able to work around this with a typename directive. If I remember correctly you want to put that on Foo.bar and then it will become the prefix for all the union members from that branch. (Or maybe it works on the fragment FooBar as well, I forget. If not that's something we could add -- the only reason we might not have is because there was no obvious use.)

Ideally we could avoid this entirely, but I don't see an obvious way short of making basically all our type names, or at least all our fragment type names, uglier/longer. (Kinda a breaking change too.) Perhaps it's time for the collision-detector to add a suffix, although we'd have to make sure it's stable in its ordering. Suggestions welcome!