Skip to content

don't expand abstract types to object fragments where possible - #732

Merged
temaEmelyan merged 5 commits into
masterfrom
abstract-types-fragments
Jul 14, 2026
Merged

don't expand abstract types to object fragments where possible#732
temaEmelyan merged 5 commits into
masterfrom
abstract-types-fragments

Conversation

@temaEmelyan

@temaEmelyan temaEmelyan commented Jul 2, 2026

Copy link
Copy Markdown
Member

Please make sure you consider the following:

  • Add tests that use __typename in queries
  • Does this change work with all nadel transformations (rename, type rename, hydration, etc)? Add tests for this.
  • Is it worth using hints for this change in order to be able to enable a percentage rollout?
  • Do we need to add integration tests for this change in the graphql gateway?
  • Do we need a pollinator check for this?

@temaEmelyan
temaEmelyan requested a review from gnawf July 2, 2026 05:25
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Test Results

  655 files  +  655    655 suites  +655   1m 7s ⏱️ + 1m 7s
2 187 tests +2 187  1 630 ✅ +1 630  557 💤 +557  0 ❌ ±0 
2 195 runs  +2 195  1 638 ✅ +1 638  557 💤 +557  0 ❌ ±0 

Results for commit 442d95c. ± Comparison against base commit 812c2dd.

♻️ This comment has been updated with latest results.

@gnawf gnawf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good so far.

?: return null

// Do nothing for fields with instructions (rename/hydration/stub/…)
val hasFieldInstructions = runCatching {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we rewrite this to avoid expecting an exception and using runCatching to catch it.

val isTypename = overallField.fieldName == TypeNameMetaFieldDef.name

// getFieldDefinitions can throw while planning a hydration backing query; any failure => not a candidate.
val parentOutputTypes = runCatching {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as before, shouldn't rely on try catch here. If the type doesn't exist in engineSchema then it should be handled without throwing.

val exposedOverallImplNames: Set<String>
val parentAbstractTypeName: String
when (val parentType = parentOutputTypes.singleOrNull()) {
is GraphQLInterfaceType -> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In theory if there are multiple output interfaces and the field exists on all of them that works too.

@temaEmelyan
temaEmelyan merged commit 898956d into master Jul 14, 2026
6 checks passed
@temaEmelyan
temaEmelyan deleted the abstract-types-fragments branch July 14, 2026 04:55
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.

2 participants