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

Schema stitching hide errors on a federated schema #1641

Closed
fenos opened this issue Jun 11, 2020 · 8 comments
Closed

Schema stitching hide errors on a federated schema #1641

fenos opened this issue Jun 11, 2020 · 8 comments

Comments

@fenos
Copy link

fenos commented Jun 11, 2020

Hello,

I'm running a hybrid solution with schema stitching as the gateway and federation as one of the backends behind the stitching solution.

+---------------------------+
|          Client           |
+------------+--------------+
             |
    +--------v-------+
    |                |
    | Stitching      |
    |                |
    +-------+--------+
            |
            v
    +-------+--------+
    |                |
    | Federation     |
    |                |
    +----------------+

It works pretty well and i'm also able to run subscriptions behind the stitches (for now)

The problem

When a resolver of a federated entity throws an error, the errors are not returned to client.

Imagine this simple scenario:

Service A

type Query {
   serviceA: ServiceA
}

type ServiceA @key(fields: "id") {
  id: Int
}
const resolvers = {
  Query: {
    serviceA() {
      return { id: 1 };
    }
  }
};

Service B

extend type ServiceA @key(fields: "id") {
  id: Int @external
  serviceBField: String
}
const resolvers = {
  ServiceA: {
    serviceBField() {
      throw new Error("serviceB field just errored!");
    }
  }
};

Let's query it

query Demo {
  serviceA {
    id
    serviceBField
  }
}

# And you'll get

{
  "data": {
    "serviceA": {
      "id": 1,
      "serviceBField": null
    }
  }
}

The errors are not in the response, but the resolver in ServiceB is triggered and it clearly throws an Error.

Have a play

CodeSandbox that reproduce the issue: https://codesandbox.io/s/peaceful-dew-bfu2n?file=/src/index.js

Current investigation

I've started digging into this for a few hours and I'm wondering if the problem that i'm having is given by the https://github.com/apollographql/apollo-server/issues/4226 which Apollo-Federation is not adhering to the GraphQL spec and stitching code doesn't know how to handle this.

Any thoughts?

This is blocking us unfortunately

@yaacovCR
Copy link
Collaborator

If the Apollo Gateway is returning the wrong path for the errors, we can't stitch those errors back correctly. You might be able to create a transform that fixes the error paths for you, but the solution as you indicate seems to be to fix the errors on the Federation side.

Another option would be to check out the type merging within schema stitching instead of Federation altogether, but I am not sure how that works for your workflow.

https://www.graphql-tools.com/docs/schema-stitching#merging-types

@fenos
Copy link
Author

fenos commented Jun 12, 2020

@yaacovCR that is fair, and I agree that it should be fixed in the downstream Apollo Federation package.

But it is still strange that the errors needs to be remapped back, instead of just being returned as they are?

@yaacovCR
Copy link
Collaborator

You could throw the errors when you receive them, but then you would have to combine multiple errors if you receive them into one large error, and also would not be able to return whatever partial data returned. So the errors are passed down the resolution tree to their original location, which requires the correct path.

@yaacovCR
Copy link
Collaborator

Maybe this workaround might help you?

I think it would limit your ability to get partial responses...

#480 (comment)

@fenos
Copy link
Author

fenos commented Jun 16, 2020

@yaacovCR

I've tried to add a transformer to the schema stitching, but unfortunately the results parameters doesn't have the errors in there

const throwOnAllResultErrors = {
      transformResult(result: any) {
        // always throw if an error is returned by the underlying schema
        if (result.errors != null && result.errors.length > 0) {
          combineErrors(result.errors);
        }

        console.log(result); // no errors

        return result;
      }
    };

Do you know any other workaround i can try?

@yaacovCR
Copy link
Collaborator

It should? Consider posting minimal reproduction as github repository

@yaacovCR yaacovCR reopened this Aug 24, 2020
yaacovCR added a commit that referenced this issue 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 yaacovCR mentioned this issue Aug 24, 2020
Merged
yaacovCR added a commit that referenced this issue 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 added a commit that referenced this issue 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
Copy link
Collaborator

yaacovCR commented Aug 25, 2020

@fenos we found a way to support this in v7...

yaacovCR added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 added a commit that referenced this issue 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 yaacovCR added the waiting-for-release Fixed/resolved, and waiting for the next stable release label Oct 6, 2020
yaacovCR added a commit that referenced this issue 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 added a commit that referenced this issue 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.
@yaacovCR yaacovCR removed bug schema stitching waiting-for-release Fixed/resolved, and waiting for the next stable release labels Oct 25, 2020
@yaacovCR
Copy link
Collaborator

Released in v7

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

3 participants