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

Be more permissive of duplicate fields with different selections #64

Open
benjaminjkraft opened this issue Aug 25, 2021 · 1 comment
Open
Labels
enhancement New feature or request needs use cases Feature we can add if people find use cases that need it (comment if that's you)

Comments

@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Aug 25, 2021

GraphQL allows you to do this:

query {
  myField {
    # these two get merged:
    subField { subSubField1 subSubField2 }
    subField { subSubField3 subSubField4 }
  }
}

and even this:

query {
  myField {
    subField { subSubField1 subSubField2 }
    ... on OnePossibleConcreteType {
      subField { subSubField3 subSubField4 }  # merged in if the concrete type matches
    }
  }
}

(Well, it sometimes lets you do that. The rules are very arcane.))

For now, to simplify things, I've simply disallowed any duplicate fields of composite type in genqlient. There's no structural limitation preventing us from adding support, if we do find it useful, we'd just need to do more complicated merging in convertSelectionSet. (there would be some subtleties to handle around type naming.) But my guess is this is quite rare (and easy enough to work around).

Note that we do already allow duplicated leaf fields, because they're much easier to deduplicate. We also allow duplicated fields if at least one is in a name fragment, because in that case we needn't merge the fields at all.

benjaminjkraft added a commit that referenced this issue Aug 25, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 26, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
## Summary:
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint

Pull request URL: #65
@benjaminjkraft benjaminjkraft added the needs use cases Feature we can add if people find use cases that need it (comment if that's you) label Sep 3, 2021
@benjaminjkraft benjaminjkraft added the enhancement New feature or request label Sep 11, 2021
@zzh8829
Copy link
Contributor

zzh8829 commented Feb 13, 2024

👍

benjaminjkraft pushed a commit that referenced this issue Feb 16, 2024
before the fix

```
=== RUN   TestGenerate/ComplexNamedFragments.graphql/Build
# command-line-arguments
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2008:8: cannot use new(TopicNewestContentNewestContentArticle) (value of type *TopicNewestContentNewestContentArticle) as type TopicNewestContentNewestContentLeafContent in assignment:
	*TopicNewestContentNewestContentArticle does not implement TopicNewestContentNewestContentLeafContent (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2011:8: cannot use new(TopicNewestContentNewestContentVideo) (value of type *TopicNewestContentNewestContentVideo) as type TopicNewestContentNewestContentLeafContent in assignment:
	*TopicNewestContentNewestContentVideo does not implement TopicNewestContentNewestContentLeafContent (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2026:7: impossible type switch case: *TopicNewestContentNewestContentArticle
	(*v) (variable of type TopicNewestContentNewestContentLeafContent) cannot have dynamic type *TopicNewestContentNewestContentArticle (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2034:7: impossible type switch case: *TopicNewestContentNewestContentVideo
	(*v) (variable of type TopicNewestContentNewestContentLeafContent) cannot have dynamic type *TopicNewestContentNewestContentVideo (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2177:8: cannot use new(UserLastContentLastContentArticle) (value of type *UserLastContentLastContentArticle) as type UserLastContentLastContentLeafContent in assignment:
	*UserLastContentLastContentArticle does not implement UserLastContentLastContentLeafContent (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2180:8: cannot use new(UserLastContentLastContentVideo) (value of type *UserLastContentLastContentVideo) as type UserLastContentLastContentLeafContent in assignment:
	*UserLastContentLastContentVideo does not implement UserLastContentLastContentLeafContent (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2195:7: impossible type switch case: *UserLastContentLastContentArticle
	(*v) (variable of type UserLastContentLastContentLeafContent) cannot have dynamic type *UserLastContentLastContentArticle (missing implementsGraphQLInterfaceSimpleLeafContent method)
/genqlient/generate/testdata/tmp/ComplexNamedFragments.graphql_727791265.go:2203:7: impossible type switch case: *UserLastContentLastContentVideo
	(*v) (variable of type UserLastContentLastContentLeafContent) cannot have dynamic type *UserLastContentLastContentVideo (missing implementsGraphQLInterfaceSimpleLeafContent method)
    /genqlient/generate/generate_test.go:120: generated code does not compile: exit status 2
```

after fix

https://github.com/Khan/genqlient/pull/310/files#diff-3eaa9f1b68120a67e7558f37dd5984e995a6df5d454656180befca84c2dbf53aR2164


I also took a look at #64 which
is the real underlaying issue i wanted to fix but that seems to be
extremely complicated and probably needs multiple weeks of work. the
nested interface issues came up on a query with union spreading. this PR
fixes the problem of spreading when the union is the same fragment,
however if the spread contains different fragment we run into #64 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs use cases Feature we can add if people find use cases that need it (comment if that's you)
Projects
None yet
Development

No branches or pull requests

2 participants