federation icon indicating copy to clipboard operation
federation copied to clipboard

Top-level errors `path` field has incorrect value in case of nested service call

Open mpospelov opened this issue 4 years ago • 7 comments

  • Package name: apollo-gateway occurs on master and latest versions
  • Here a commit with failing test:
    • https://github.com/mpospelov/apollo-server/commit/cd9cc2424026ab690a72d8874054747928d489b6
    • we are fetching the following query
    query {
      topReviews(first: 10) {
        author {
          name
        }
      }
    }
    
    • one of authors has error while fetching name field in accounts schema
  • The expected behavior: expect(response).toHaveProperty('errors.0.path', ['topReviews', 7, 'author', 'name'])
  • The actual behavior: ["_entities", 7, "name"]
  • Runnable reproduction:
    1. clone my branch https://github.com/mpospelov/apollo-server/tree/nested-services-path-building-issue
    2. run cd packages/apollo-gateway && npx jest src/__tests__/executeQueryPlan.test.ts
  • Reasoning behind this behaviour:
    1. In our project, there are multiple services which receive requests via apollo-gateway
    2. Each service can raise authorization field error based on current user permission
    3. In order to show errors properly on front-end, errors.*.path field should comply with graphql specs https://spec.graphql.org/June2018/#sec-Errors

mpospelov avatar Jun 09 '20 07:06 mpospelov

I believe it is important to comply with the specs here. From the perspective of the client it should be transparent that a request is going through a gateway. I hope you can consider this issue and thank you for all the hard work!

paneq avatar Jun 09 '20 15:06 paneq

Is there any update on moving this forward?

It is critical (as already mentioned above) that the response comply with the GraphQL spec, since all the tooling are built around that.

I've started digging into it, but with the current federation implementation it seem tricky to achieve it.

Some thoughts

Firstly i needed to identify where it makes more sense to fix this issue, in the federated service or the gateway?

With the current understanding that i have, it might be pretty difficult to fix it at the federated service level, since, it has no knowledge of where the federated entity query belongs in the entire selection set. Even thought it might be the ideal place

So i thought that the gateway would be an easier option since it has the knowledge of the entire selection before each node is split during the query plan.

I was trying to find a way of mapping back the errors to the right path comparing the initial selection set with the response of the PlanNode

i'm not very close with a working solution, still trying to figure out some code paths and thoughts behind the federation implementation.

If you have some advise i'd be glad to take this forward or at least help in some way 😄

Obviously a big thanks for the work you are all doing.

fenos avatar Jun 15 '20 15:06 fenos

Is there any word on this? Or a work around anyone has come up with? Cheers!

jonahmolinski avatar Feb 23 '21 01:02 jonahmolinski

My first look at the code, but it seems something could be done in this method to map the error paths: https://github.com/apollographql/federation/blob/813101200012f9268fcafb65eeaedf27ca7479b7/gateway-js/src/executeQueryPlan.ts#L192

prowe avatar Mar 13 '21 02:03 prowe

Howdy to everyone following this issue! We're planning on rolling this into the 2.2.0 release, which is still in the planning phase. We don't know the specific implementation path yet (the linked PR #988 seems like a great start), but we'll start making more progress here within the next few weeks.

benweatherman avatar Jun 16 '22 22:06 benweatherman

Hi @benweatherman !

Do you have an ETA to share when the 2.2.0 release will be made available? On the milestones page of the project, there is no due date set for that release. I see the 2.1.0 release is just around the corner, due 2 days from now on July 8th, but hoping that the 2.2.0 release won't be too far off so that we can benefit from this fix at some point in the near future.

Thank you very much!

EXPEylazzari avatar Jul 06 '22 21:07 EXPEylazzari

Howdy @EXPEylazzari! No ETA yet on when this will release, but it's the next feature we're working on. Once it's finished, it'll go into whatever the next release is.

For more context, our main focus for 2.1.0 is composing user directives #1893, which has changed several times during implementation. That's causing the 2.1.0 release to take longer than we originally anticipated. So it's possible this issue will be released in 2.1.0, depending on when #1893 wraps up.

TL;DR we're working on this next and it'll be released in whatever version follows its completion.

benweatherman avatar Jul 12 '22 21:07 benweatherman

Hi all! We recently fixed this issue in https://github.com/apollographql/federation/pull/2304, which has been delivered in Federation v2.3. With that, I'll be closing this issue, but feel free to open any new issues if your use cases are not fully met. Thanks!

korinne avatar Feb 21 '23 17:02 korinne