Skip to content

feat: subgraph error propagation improvements #883

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

Merged

Conversation

StarpTech
Copy link
Collaborator

@StarpTech StarpTech commented Sep 10, 2024

This PR adds the following improvements:

  • Ensure that no extension fields are propagated by default.
  • Use the dataSource name (Subgraph name) for error messages.
  • Allow defining granularly which extension fields should be propagated.
  • Allow defining a fallback value for the extension code field.
  • Allow attaching the dataSource name as the serviceName field to the extension object.
  • data: null is never returned for errors that are happening before query execution. This compliant with the spec.

Copy link
Contributor

@df-wg df-wg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@jensneuse jensneuse left a comment

Choose a reason for hiding this comment

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

Overall it looks very good and is a significant improvement! I'd like to see if we can move some "workarounds" to the astjson package.

@StarpTech
Copy link
Collaborator Author

@jensneuse astjson has already a concept of Arenas which allows for easy value creation including memory reuse. Please check again.

@jensneuse
Copy link
Member

@jensneuse astjson has already a concept of Arenas which allows for easy value creation including memory reuse. Please check again.

The idea is good. Great find!

I'd slightly change the implementation though.
Instead of having an arena pool, you can add one arena to each Resolvable.
The Resolvable has the lifecycle of the Request, which means that it can be invalidated once the Request handling is done.
This is already implemented and we're using it to free resources live throughout the lifecycle of a Request.
So I think the simple optimal solution would be to add one arena to each resolvable and create JSON AST nodes from it.

WDYT?

@StarpTech StarpTech merged commit 13cb695 into master Sep 12, 2024
8 checks passed
@StarpTech StarpTech deleted the dustin/eng-5648-subgraph-error-propagation-improvements branch September 12, 2024 13:06
devsergiy pushed a commit that referenced this pull request Nov 15, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.1.0](execution/v1.0.9...execution/v1.1.0)
(2024-11-14)


### Features

* add data source ID to trace
([#870](#870))
([beb8720](beb8720))
* add further apollo-compatible error support
([#939](#939))
([2d08eb6](2d08eb6))
* add query plans
([#871](#871))
([da79d7e](da79d7e))
* expose acquire resolver wait time in loader hooks
([#854](#854))
([b85148d](b85148d))
* expose compose method of engine federation config factory
([#878](#878))
([95e943e](95e943e))
* improve performance and memory usage of loader & resolbable
([#851](#851))
([27670b7](27670b7))
* improve resolve performance by solving merge abstract nodes in
postprocessing
([#826](#826))
([6566e02](6566e02))
* include subgraph name in ART
([#929](#929))
([fc0993d](fc0993d))
* rewrite variable renderer to use astjson
([#946](#946))
([0d2d642](0d2d642))
* subgraph error propagation improvements
([#883](#883))
([13cb695](13cb695))
* support multiple pubsub providers
([#788](#788))
([ea8b3d3](ea8b3d3))
* validate returned enum values
([#936](#936))
([7aa4add](7aa4add))


### Bug Fixes

* argument and variable validation during execution
([#902](#902))
([895e332](895e332))
* correctly render trace and query plan together
([#874](#874))
([2fc364f](2fc364f))
* execution validation order, do not reuse planner
([#925](#925))
([3ffce8b](3ffce8b))
* ignore empty errors
([#890](#890))
([4c5556f](4c5556f))
* improve ws subprotocol selection
([1fc0dd9](1fc0dd9))
* improve ws subprotocol selection
([#795](#795))
([ad67dbb](ad67dbb))
* keep scalar order when merging fields in post processing
([#835](#835))
([d27fb6e](d27fb6e))
* keep unused variables during normalization
([#802](#802))
([15ae7b3](15ae7b3))
* level of null data propagation
([#810](#810))
([537f4d6](537f4d6))
* merging fields correctly
([0dfb6a2](0dfb6a2))
* merging fields correctly
([#836](#836))
([3c4cb17](3c4cb17))
* merging response nodes
([#772](#772))
([5e89693](5e89693))
* planning of provides, parent entity jump, conditional implicit keys,
handling of external fields
([#818](#818))
([fe6ffd6](fe6ffd6)),
closes
[#830](#830)
[#847](#847)
* return empty data when all root fields was skipped
([#910](#910))
([4607dc0](4607dc0))
* variables normalization for the anonymous operations
([#965](#965))
([267aef8](267aef8))

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

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants