router icon indicating copy to clipboard operation
router copied to clipboard

Remove `@` from error paths

Open carodewig opened this issue 5 months ago • 3 comments

When a subgraph returns an unexpected response (ie not a body with at least one of errors or data), the errors surfaced by the router include an @ in the path which indicates an error applied to all elements in the array. This is not a behavior defined in the GraphQL spec and is not easily parsed.

This PR expands the @ symbol to reflect all paths that the error should apply to.

The tests reflect the following federated graph with 4 subgraphs:

products ----> inventory
         \---> reviews
                     \---> accounts

## router response (all subgraph calls successful)
{
    "data": {
        "topProducts": [
            {
                "name": "Table",
                "inStock": true,
                "reviews": [
                    {"id": "1", "author": {"username": "@ada", "name": "Ada"}},
                    {"id": "1", "author": {"username": "@alan", "name": "Alan"}}
                ]
            },
            {
                "name": "Chair",
                "inStock": false,
                "reviews": [
                    {"id": "3", "author": {"username": "@alan", "name": "Alan"}}
                ]
            }
        ]
    }
}

Currently, if the inventory subgraph returns a malformed response, the router response will look like:

{
    "data": {"topProducts": [{"name": "Table", "inStock": Null, "reviews": [{"id": "1", "author": {"username": "@ada", "name": "Ada"}}, {"id": "1", "author": {"username": "@alan", "name": "Alan"}}]}, {"name": "Chair", "inStock": Null, "reviews": [{"id": "3", "author": {"username": "@alan", "name": "Alan"}}]}]}, 
    "errors": [
        {
            "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error", 
            "path": ["topProducts", "@"], 
            "extensions": {"service": "inventory", "reason": "graphql response without data must contain at least one error", "code": "SUBREQUEST_MALFORMED_RESPONSE"}
        }
    ]
}

With this change, the response will look like:

{
    "data":{"topProducts":[{"name":"Table","inStock":null,"reviews":[{"id":"1","author":{"username":"@ada","name":"Ada"}},{"id":"1","author":{"username":"@alan","name":"Alan"}}]},{"name":"Chair","inStock":null,"reviews":[{"id":"3","author":{"username":"@alan","name":"Alan"}}]}]},
    "errors": [
        {
            "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error",
            "path":["topProducts",0],
            "extensions":{"service":"inventory","reason":"graphql response without data must contain at least one error","code":"SUBREQUEST_MALFORMED_RESPONSE"}
        },
        {
            "message":"service 'inventory' response was malformed: graphql response without data must contain at least one error",
            "path":["topProducts",1],
            "extensions":{"service":"inventory","reason":"graphql response without data must contain at least one error","code":"SUBREQUEST_MALFORMED_RESPONSE"}
        }
    ]
}

TODO:

  • [ ] Would it be better to just specify 0 as the element rather than expanding all the paths?
  • [ ] Is inverted_paths the best way to handle this?
  • [ ] Should this be applied in the Fetch step of the query plan, or should it take place in the Flatten step? I put it in the Fetch step because that's where the inverted_paths variable is accessible, so I think that's where we know all the paths the error applies to.
  • [ ] Document the reasoning behind interim_errors inline
  • [ ] Use snapshots to capture which errors are actually present rather than just counting the number of errors (in the tests)

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • [ ] Changes are compatible[^1]
  • [ ] Documentation[^2] completed
  • [ ] Performance impact assessed and acceptable
  • [ ] Metrics and logs are added[^3] and documented
  • Tests added and passing[^4]
    • [ ] Unit Tests
    • [ ] Integration Tests
    • [ ] Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]: A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices. [^4]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

carodewig avatar Jun 13 '25 13:06 carodewig