fix: check interface implementation in potentiallySameObject for nullability relaxation

[davidomid]

Fixes #1455

Summary

When RelaxSubgraphOperationFieldSelectionMergingNullability is enabled, field selection merging should allow nullability differences between types that cannot overlap at runtime. The potentiallySameObject function previously treated any interface+object pair as potentially overlapping, which prevented nullability relaxation even when the object type does not implement the interface.

Example

Using the existing boxDefinition test schema, consider these types:

• NonNullStringBox1 — an interface with scalar: String!
• StringBox — an object type that implements SomeBox but does not implement NonNullStringBox1, with scalar: String

The following query:

{
  someBox {
    ... on NonNullStringBox1 { scalar }
    ... on StringBox         { scalar }
  }
}

produces this validation error even with RelaxSubgraphOperationFieldSelectionMergingNullability enabled:

fields 'scalar' conflict because they return conflicting types 'String!' and 'String'

Since StringBox does not implement NonNullStringBox1, no runtime object can ever satisfy both type conditions simultaneously, so the nullability difference is safe to relax. After this fix, the query validates successfully when the relaxation flag is enabled.

Fix

Updated potentiallySameObject to split the single interface case into three:

• both interfaces → conservatively assume overlap (true)
• interface + object → overlap only if the object implements the interface (check via NodeImplementsInterfaceNode)
• two objects → overlap only if same type name

Testing

Two new test cases have been added alongside the existing "disallows differing return type nullability when interface could overlap" test, reusing the same boxDefinition schema:

1. "allows differing return type nullability on interface vs non implementing type with relaxation" — same query as the existing test (NonNullStringBox1 vs StringBox) but with FieldSelectionMerging(true) → Valid. Directly demonstrates the fix: StringBox does not implement NonNullStringBox1, so relaxation applies.

2. "rejects differing return type nullability on interface vs implementing type even with relaxation" — SomeBox (interface, scalar: String) vs NonNullStringBox1Impl (implements SomeBox, scalar: String!), FieldSelectionMerging(true) → Invalid. Confirms that overlap detection still blocks relaxation when types actually overlap.

Additionally, some manual tests have been done by:

1. Pulling this branch into a branch of wundergraph/cosmo.
2. Pulling that wundergraph/cosmo branch into a federation router service.
3. Running some queries to verify that the conflict errors have been removed for non-overlapping types.

Checklist

• I have discussed my proposed changes in an issue and have received approval to proceed.
• I have followed the coding standards of the project.
• Tests or benchmarks have been added or updated.

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

Summary by CodeRabbit

• Bug Fixes

  • Improved validation of overlapping interface and object types during field selection merging.
  • Correctly allows compatible nullability differences when interface fragments overlap.
  • Continues to reject invalid nullability conflicts between unrelated object types.

• Tests

  • Added coverage for valid and invalid field selection merging scenarios involving nullable and non-null fields.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bbd70639-82df-4880-bdbe-318b2d6e71f0

📥 Commits

Reviewing files that changed from the base of the PR and between d277d2f and 1faf730.

📒 Files selected for processing (2)

• v2/pkg/astvalidation/operation_rule_field_selection_merging.go
• v2/pkg/astvalidation/operation_validation_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• v2/pkg/astvalidation/operation_rule_field_selection_merging.go
• v2/pkg/astvalidation/operation_validation_test.go

---

📝 Walkthrough

Walkthrough

The change updates field-selection-merging overlap logic. Interface/object pairs overlap only when the object implements the interface. New tests cover allowed and rejected nullability differences.

Changes

Field selection merging validation

Layer / File(s)	Summary
Type overlap calculation v2/pkg/astvalidation/operation_rule_field_selection_merging.go	potentiallySameObject treats interface pairs as overlapping and checks NodeImplementsInterfaceNode for interface/object pairs. Object/object behavior remains name equality.
Nullability validation coverage v2/pkg/astvalidation/operation_validation_test.go	Tests allow differing nullability for non-overlapping fragments and reject it for potentially overlapping fragments when FieldSelectionMerging(true) is enabled.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the nullability-relaxation fix in potentiallySameObject.
Linked Issues check	✅ Passed	The implementation satisfies [#1455] by checking interface implementation and preserving interface/interface and object/object behavior; focused tests cover both branches.
Out of Scope Changes check	✅ Passed	The changes are limited to potentiallySameObject and focused validation tests related to [#1455].

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

[coderabbitai]

🧹 Nitpick comments (1)

v2/pkg/astvalidation/operation_validation_test.go (1)

1137-1144: This test doesn’t prove the implementing-object overlap branch.

Printable.title and Book.title are both String!, so this stays valid even if potentiallySameObject wrongly treats Printable + Book as non-overlapping. To lock down the new branch, make the interface/object pair differ in nullability and assert that FieldSelectionMerging(true) still rejects them.

🧪 Example adjustment

- t.Run("allows matching types on interface vs implementing object type with relaxation", func(t *testing.T) {
+ t.Run("rejects differing nullability on interface vs implementing object type with relaxation", func(t *testing.T) {
   runWithDefinition(t, interfaceNonImplementorDefinition, `
     {
       item {
-        ... on Printable { title }
-        ... on Book      { title }
+        ... on Printable { summary }
+        ... on Book      { summary }
       }
-    }`, FieldSelectionMerging(true), Valid)
+    }`, FieldSelectionMerging(true), Invalid,
+      withValidationErrors(`fields 'summary' conflict because they return conflicting types 'String' and 'String!'`))
 })

And in interfaceNonImplementorDefinition, add a field like:

 interface Printable {
 	title: String!
 	name: String!
+	summary: String
 }
 
 type Book implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }
 
 type Magazine implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation_test.go` around lines 1137 - 1144,
The test "allows matching types on interface vs implementing object type with
relaxation" currently uses Printable.title and Book.title both as String!, so it
doesn't exercise the implementing-object overlap branch; change the schema in
interfaceNonImplementorDefinition so the interface and the implementing object
have the same field name with different nullability (e.g., Printable.title as
String and Book.title as String! or vice versa) and update the assertion to
expect rejection when FieldSelectionMerging(true) is applied, ensuring
runWithDefinition(t, interfaceNonImplementorDefinition, ...) fails validation
for that mismatch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 1137-1144: The test "allows matching types on interface vs
implementing object type with relaxation" currently uses Printable.title and
Book.title both as String!, so it doesn't exercise the implementing-object
overlap branch; change the schema in interfaceNonImplementorDefinition so the
interface and the implementing object have the same field name with different
nullability (e.g., Printable.title as String and Book.title as String! or vice
versa) and update the assertion to expect rejection when
FieldSelectionMerging(true) is applied, ensuring runWithDefinition(t,
interfaceNonImplementorDefinition, ...) fails validation for that mismatch.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: edecbe3f-1522-4aec-975b-2a06dceab6ca

📥 Commits

Reviewing files that changed from the base of the PR and between 18b292e and ccc1b35.

📒 Files selected for processing (2)

• v2/pkg/astvalidation/operation_rule_field_selection_merging.go
• v2/pkg/astvalidation/operation_validation_test.go

[davidomid]

🧹 Nitpick comments (1)

v2/pkg/astvalidation/operation_validation_test.go (1)> 1137-1144: This test doesn’t prove the implementing-object overlap branch.

Printable.title and Book.title are both String!, so this stays valid even if potentiallySameObject wrongly treats Printable + Book as non-overlapping. To lock down the new branch, make the interface/object pair differ in nullability and assert that FieldSelectionMerging(true) still rejects them.

🧪 Example adjustment

- t.Run("allows matching types on interface vs implementing object type with relaxation", func(t *testing.T) {
+ t.Run("rejects differing nullability on interface vs implementing object type with relaxation", func(t *testing.T) {
   runWithDefinition(t, interfaceNonImplementorDefinition, `
     {
       item {
-        ... on Printable { title }
-        ... on Book      { title }
+        ... on Printable { summary }
+        ... on Book      { summary }
       }
-    }`, FieldSelectionMerging(true), Valid)
+    }`, FieldSelectionMerging(true), Invalid,
+      withValidationErrors(`fields 'summary' conflict because they return conflicting types 'String' and 'String!'`))
 })

And in interfaceNonImplementorDefinition, add a field like:

 interface Printable {
 	title: String!
 	name: String!
+	summary: String
 }
 
 type Book implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }
 
 type Magazine implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation_test.go` around lines 1137 - 1144,
The test "allows matching types on interface vs implementing object type with
relaxation" currently uses Printable.title and Book.title both as String!, so it
doesn't exercise the implementing-object overlap branch; change the schema in
interfaceNonImplementorDefinition so the interface and the implementing object
have the same field name with different nullability (e.g., Printable.title as
String and Book.title as String! or vice versa) and update the assertion to
expect rejection when FieldSelectionMerging(true) is applied, ensuring
runWithDefinition(t, interfaceNonImplementorDefinition, ...) fails validation
for that mismatch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 1137-1144: The test "allows matching types on interface vs
implementing object type with relaxation" currently uses Printable.title and
Book.title both as String!, so it doesn't exercise the implementing-object
overlap branch; change the schema in interfaceNonImplementorDefinition so the
interface and the implementing object have the same field name with different
nullability (e.g., Printable.title as String and Book.title as String! or vice
versa) and update the assertion to expect rejection when
FieldSelectionMerging(true) is applied, ensuring runWithDefinition(t,
interfaceNonImplementorDefinition, ...) fails validation for that mismatch.

ℹ️ Review info

🧹 Nitpick comments (1)

v2/pkg/astvalidation/operation_validation_test.go (1)> 1137-1144: This test doesn’t prove the implementing-object overlap branch.

Printable.title and Book.title are both String!, so this stays valid even if potentiallySameObject wrongly treats Printable + Book as non-overlapping. To lock down the new branch, make the interface/object pair differ in nullability and assert that FieldSelectionMerging(true) still rejects them.

🧪 Example adjustment

- t.Run("allows matching types on interface vs implementing object type with relaxation", func(t *testing.T) {
+ t.Run("rejects differing nullability on interface vs implementing object type with relaxation", func(t *testing.T) {
   runWithDefinition(t, interfaceNonImplementorDefinition, `
     {
       item {
-        ... on Printable { title }
-        ... on Book      { title }
+        ... on Printable { summary }
+        ... on Book      { summary }
       }
-    }`, FieldSelectionMerging(true), Valid)
+    }`, FieldSelectionMerging(true), Invalid,
+      withValidationErrors(`fields 'summary' conflict because they return conflicting types 'String' and 'String!'`))
 })

And in interfaceNonImplementorDefinition, add a field like:

 interface Printable {
 	title: String!
 	name: String!
+	summary: String
 }
 
 type Book implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }
 
 type Magazine implements Printable {
 	title: String!
 	name: String!
+	summary: String!
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@v2/pkg/astvalidation/operation_validation_test.go` around lines 1137 - 1144,
The test "allows matching types on interface vs implementing object type with
relaxation" currently uses Printable.title and Book.title both as String!, so it
doesn't exercise the implementing-object overlap branch; change the schema in
interfaceNonImplementorDefinition so the interface and the implementing object
have the same field name with different nullability (e.g., Printable.title as
String and Book.title as String! or vice versa) and update the assertion to
expect rejection when FieldSelectionMerging(true) is applied, ensuring
runWithDefinition(t, interfaceNonImplementorDefinition, ...) fails validation
for that mismatch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@v2/pkg/astvalidation/operation_validation_test.go`:
- Around line 1137-1144: The test "allows matching types on interface vs
implementing object type with relaxation" currently uses Printable.title and
Book.title both as String!, so it doesn't exercise the implementing-object
overlap branch; change the schema in interfaceNonImplementorDefinition so the
interface and the implementing object have the same field name with different
nullability (e.g., Printable.title as String and Book.title as String! or vice
versa) and update the assertion to expect rejection when
FieldSelectionMerging(true) is applied, ensuring runWithDefinition(t,
interfaceNonImplementorDefinition, ...) fails validation for that mismatch.

ℹ️ Review info

I'll resolve this

[ysmolski]

Hi @davidomid. Is this PR ready for review again?

[ysmolski]

LGTM, but need another vote from @devsergiy too

[tylerscoville]

@davidomid @ysmolski have there been any updates? I'm running into the same issue.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.