Skip to content
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

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

Closed
mpospelov opened this issue Jun 9, 2020 · 8 comments

Comments

@mpospelov
Copy link
Contributor

  • Package name: apollo-gateway occurs on master and latest versions
  • Here a commit with failing test:
    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
@paneq
Copy link

paneq commented Jun 9, 2020

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!

@fenos
Copy link

fenos commented Jun 15, 2020

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.

yaacovCR referenced this issue in ardatan/graphql-tools Aug 24, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Aug 24, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Aug 25, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Aug 31, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Aug 31, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 2, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 4, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 13, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 21, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 21, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 21, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 21, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Sep 30, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Oct 1, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Oct 6, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
yaacovCR referenced this issue in ardatan/graphql-tools Oct 23, 2020
Previously, errors with invalid or missing paths were lost.
See:
#1641
https://github.com/apollographql/apollo-server/issues/4226

They are now saved!

All correctly pathed errors are now inlined into the returned data by the CheckResultAndHandleErrors transform.

The data is now annotated only with the remaining "unpathed" errors. These are then returned when possible if a null is encountered including the missing or potentially invalid path.

Changes to error handling obviate some existing functions including getErrorsByPathSegment, getErrors in favor of getUnpathedErrors.

Utility functions including slicedError and unreleased functions extendedError and unextendedError are no longer necessary.
@abernix abernix transferred this issue from apollographql/apollo-server Jan 15, 2021
@jonahmolinski
Copy link

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

@prowe
Copy link

prowe commented Mar 13, 2021

My first look at the code, but it seems something could be done in this method to map the error paths:

async function executeFetch<TContext>(

@benweatherman
Copy link
Contributor

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.

@EXPEylazzari
Copy link

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!

@benweatherman
Copy link
Contributor

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.

@jeffjakub jeffjakub removed this from the 2.2.0 milestone Jan 6, 2023
@korinne
Copy link
Contributor

korinne commented Feb 21, 2023

Hi all! We recently fixed this issue in #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 korinne closed this as completed Feb 21, 2023
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

No branches or pull requests

9 participants