Skip to content

fix: avoid duplicate joins on errors#1314

Merged
SkArchon merged 2 commits intomasterfrom
milinda/duplicate-cleanup
Oct 9, 2025
Merged

fix: avoid duplicate joins on errors#1314
SkArchon merged 2 commits intomasterfrom
milinda/duplicate-cleanup

Conversation

@SkArchon
Copy link
Copy Markdown
Contributor

@SkArchon SkArchon commented Oct 8, 2025

In case of non http errors the errors are duplicated resulting in duplicates in the error message such as

Post \"http://localhost:4001/graphql\": dial tcp [::1]:4001: connect: connection refused
Post \"http://localhost:4001/graphql\": dial tcp [::1]:4001: connect: connection refused
Failed to fetch from Subgraph 'employees'.

This is as we join the same error multiple times. Since we already join res.err and set it to l.ctx.subgraphErrors in l.mergeResult we can simply remove the join inside newResponseInfo

Summary by CodeRabbit

  • Bug Fixes
    • Improved error reporting for subgraph responses by avoiding aggregation of multiple errors. Users will now see clearer, more precise error messages that better reflect the originating issue.
    • Reduces noisy or confusing error chains, improving consistency in error handling and making failures easier to diagnose in client applications and logs.

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.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 8, 2025

Walkthrough

Replaces joined error propagation in newResponseInfo by setting Err solely to subgraphError, removing the previous goerrors.Join(res.err, subgraphError) behavior.

Changes

Cohort / File(s) Summary of Changes
Error handling update
v2/pkg/engine/resolve/loader.go
In newResponseInfo, Err is now assigned to subgraphError directly instead of goerrors.Join(res.err, subgraphError), altering error propagation to exclude the parent error.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely summarizes the primary fix of preventing duplicate error joins and clearly reflects the main behavioral change in the code.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d60605 and 042dc24.

📒 Files selected for processing (1)
  • v2/pkg/engine/resolve/loader.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build and test (go 1.25 / ubuntu-latest)
  • GitHub Check: Build and test (go 1.25 / windows-latest)
  • GitHub Check: Build and test (go 1.25 / windows-latest)
  • GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (1)
v2/pkg/engine/resolve/loader.go (1)

66-92: Approve removal of redundant error join
All newResponseInfo invocations pass l.ctx.subgraphErrors, which already includes res.err, eliminating duplicate messages without losing any errors.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@ysmolski ysmolski left a comment

Choose a reason for hiding this comment

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

Indeed, we call newResponseInfo after every mergeResult.

@SkArchon SkArchon merged commit a1f1f8c into master Oct 9, 2025
10 checks passed
@SkArchon SkArchon deleted the milinda/duplicate-cleanup branch October 9, 2025 09:44
ysmolski pushed a commit that referenced this pull request Oct 9, 2025
🤖 I have created a release *beep* *boop*
---


##
[2.0.0-rc.230](v2.0.0-rc.229...v2.0.0-rc.230)
(2025-10-09)


### Bug Fixes

* avoid duplicate joins on errors
([#1314](#1314))
([a1f1f8c](a1f1f8c))
* propagate fetch reasons for interface-related fields
([#1312](#1312))
([5ee3014](5ee3014))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
  * Prevent duplicate joins when errors occur, improving stability.
* Propagate fetch reasons for interface-related fields for more accurate
behavior and diagnostics.
* **Documentation**
  * Added changelog entry for version 2.0.0-rc.230.
* **Chores**
  * Bumped version to 2.0.0-rc.230.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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