Repository navigation
Merge duplicate selections across fragments and reject overlapping fragment types - #860
Merged
Merged
Conversation
Contributor
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
There was a problem hiding this comment.
🟡 Changes recommended
Query selection merging and overlap validation still have unresolved correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR merges duplicate GraphQL selections and rejects overlapping fragment types.
Changes:
- Adds overlap validation and duplicate-selection merging.
- Documents the behavior in the README.
- Adds input, error, and golden-output fixtures.
File summaries
| File | Summary |
|---|---|
README.md |
Documents overlap rejection and duplicate-selection behavior. |
gen/rules/src/main/scala/clue/gen/QueryGen.scala |
Implements validation and merging; unresolved correctness issues remain. |
gen/output/src/main/scala/test/StarWarsDuplicateSelectionSum.scala |
Golden output for multi-variant duplicate merging. |
gen/output/src/main/scala/test/StarWarsDuplicateSelectionSingle.scala |
Golden output for single-fragment duplicate merging. |
gen/input/src/main/scala/test/StarWarsOverlappingFragments.scala |
Error fixture for overlapping fragments. |
gen/input/src/main/scala/test/StarWarsDuplicateSelectionSum.scala |
Input fixture for multi-variant merging. |
gen/input/src/main/scala/test/StarWarsDuplicateSelectionSingle.scala |
Input fixture for single-fragment merging. |
Review details
Suppressed comments (3)
gen/rules/src/main/scala/clue/gen/QueryGen.scala:757
- Deprecation is schema metadata, not part of the response shape being merged. An interface field can be deprecated while the implementing object's declaration is not; selecting that field at the base level and again in the object fragment then produces equal GraphQL selections with different
ClassParam.deprecationvalues, so this comparison refuses to merge them and the generated subtype gets duplicate parameters. Compare the response name/type for merging and preserve an appropriate deprecation annotation when collapsing the group.
private def sameSelection(a: ClassParam, b: ClassParam): Boolean =
a.name == b.name && a.tpe.structure == b.tpe.structure && a.deprecation == b.deprecation
gen/rules/src/main/scala/clue/gen/QueryGen.scala:1009
- The subtype accumulator only carries
classesfrom the variant fragments here; nested classes built for the base selections remain inbaseAccumulator.classes. Consequently a valid duplicate object selection such asfriends { name }at the base plus... on Human { friends { name } }produces one mergedfriendsparameter but separateCharacter.FriendsandCharacter.Human.Friendsdefinitions, so the subtype can reference an incompatible nested type (or lose the base fields). The base and variant nested class definitions need to be reused/merged along with the parameter merge.
mergeDuplicateSelections(
ClassAccumulator(
accumulators.map(_._1).combineAll.classes,
(baseAccumulators.map { case (accumlator, idx) =>
(accumlator.withOverrideParams, idx)
gen/rules/src/main/scala/clue/gen/QueryGen.scala:803
- A non-null field selected unconditionally at the base and again with a field-level
@include/@skipis a valid GraphQL merge, butforceOptionalmakes the twoClassParam.tpevalues differ, so this predicate leaves both parameters inparAccumand generates duplicate constructor arguments. The merge needs to account for directive-induced optionality (an unconditional occurrence should keep the field non-optional; if all occurrences are conditional it should be optional) instead of treating the generated types as conflicting content.
if (group.tail.forall(sameSelection(_, group.head)))
List(group.head.copy(overrides = group.exists(_.overrides)))
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
The suppressed comments were addressed in a new commit too. |
There was a problem hiding this comment.
🟡 Changes recommended
QueryGen.scala has a critical unresolved order-sensitive nested-selection comparison, plus a minor comment wording issue.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
gen/rules/src/main/scala/clue/gen/QueryGen.scala:801
sameParamsis used bysameClassas response-shape equality, but this comparison makes both source order and deprecation metadata significant. For example,friends { name id }at the base andfriends { id name }in a Human fragment are the same GraphQL selection, yet this returns false and leaves duplicatefriendsparameters/classes; likewise, an interface field deprecated while the implementation is not prevents otherwise identical nested classes from merging. Compare the normalized parameter name/type/override shape instead; deprecation is not part of the response shape.
x.name == y.name && x.tpe.structure == y.tpe.structure && x.overrides == y.overrides &&
x.deprecation == y.deprecation
gen/rules/src/main/scala/clue/gen/QueryGen.scala:805
sameClassesapplies the same positional comparison to nested class definitions. A duplicated object selection such as{ author { id } editor { id } }versus{ editor { id } author { id } }has the same GraphQL response shape, but this returns false and prevents the outer selection from merging, again emitting duplicate parameters/classes. Match nested classes by name and shape rather than their source order.
private def sameClasses(a: List[Class], b: List[Class]): Boolean =
a.sizeIs == b.size && a.zip(b).forall { case (x, y) => sameClass(x, y) }
gen/rules/src/main/scala/clue/gen/QueryGen.scala:811
- This new comment is missing a word and is ungrammatical: “The single param a group...” should be rewritten as a complete sentence.
// The single param a group of [[sameSelection]] params collapses to.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Nested duplicate selections can remain unmerged and produce non-compiling generated case classes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
gen/rules/src/main/scala/clue/gen/QueryGen.scala:826
sameSumcomparesSumDiscriminatorwith case-class equality, so the order of nested fragment variants becomes part of equality. Two otherwise identical nested polymorphic selections with theirHuman/Droidfragments in opposite order produce the same response shape (and, with overlap rejected, the same one-to-one mapping), but this returnsfalse, so the outer duplicate is left as duplicate params/classes and the generated code still fails to compile. Compare the discriminator cases as an order-insensitive mapping while retaining the key and fallback.
private def sameSum(a: Sum, b: Sum): Boolean =
sameParams(a.params, b.params) && sameClasses(a.nested, b.nested) &&
sameClasses(a.instances, b.instances) && a.discriminator == b.discriminator
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new duplicate-selection merge logic can throw at generator runtime for scalar fields due to an unguarded nested.head access when no nested classes exist.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+895
to
+898
| val mergeable: Boolean = | ||
| group.tail.forall(sameSelection(_, group.head)) && | ||
| nested.drop(1).forall(sameClass(_, nested.head)) | ||
|
|
…optional/deprecated duplicates - Overlap validation (and the decoder's discriminator cases) only consider concrete types the current type can return, so two interface fragments overlapping on an object outside the current union are no longer rejected. - Duplicate selection merging ignores deprecation and a top-level Option (from @include/@Skip or non-null narrowing); the base-level type wins, deprecated if any occurrence is. - Identical nested classes selected at the base level and inside a variant are generated once, in the sum's companion, so the variant's override matches the trait's type instead of shadowing it. Differing sub-selections are left alone (compile error, as before). - Fixtures: StarWarsDuplicateSelectionNested, StarWarsDuplicateSelectionConditional. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
rpiaggio
force-pushed
the
fragment-merge-and-overlap
branch
from
September 18, 2026 17:26
9356bdf to
2f85670
Compare
Collaborator
Author
|
copilot's comments were addressed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes to polymorphic selection generation (follow-up to #855).
Duplicate selections. A field selected both at the base level and inside a fragment
(
hero { name ... on Human { name homePlanet } }, which GraphQL merges into one responsevalue) was emitted twice in the variant's case class, producing code that doesn't compile.
Identical repeated selections are now merged, keeping
overridewhere the base level had it.Same name with different content is left alone and still surfaces as a compile error.
Overlapping fragment types. Fragments whose type conditions share a concrete type
(
... on Humantogether with... on Pilot, an interface onlyHumanimplements) generatedan unreachable case: the decoder routed
"Human"to the first one and the other's fieldswere silently lost. This is now a validation error asking to merge them into one fragment.
Includes README notes for both, an error fixture for the overlap, and two golden fixtures for
the merge (single fragment with
Other, and two fragments).