Add RequestError and GraphQLError types to indicate server failure and graphql failure.

[zhenlu]

Fixes LIG-5257

[zhenlu]

• Add RequestError and GraphQLError types to indicate server failure and graphql failure. #95 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @zhenlu and the rest of your teammates on Graphite

[jklein24]

@mgorven can correct me if I'm wrong, but I'm not sure if this totally solves what they're looking for. The most important piece is being able to tell "is this my fault or lightspark's fault and should I retry". @mgorven is it true that anything with an extension is likely to be a non-retryable user error?

[zhenlu]

I think the RequestError will be the things for them to retry. GraphQLError is the error returned from graphql. All other errors are the errors with their code.

I don't think they should retry on GraphQLError here but it did indicate either side have a bug in code that needs to be fixed.

[mgorven]

@mgorven can correct me if I'm wrong, but I'm not sure if this totally solves what they're looking for. The most important piece is being able to tell "is this my fault or lightspark's fault and should I retry". @mgorven is it true that anything with an extension is likely to be a non-retryable user error?

It's mostly but not completely true. There are a few cases where we have an extension but it's not necessarily the user's fault (exception classes here which inherit directly from ExternalException, currently NoRoutes and CardPaymentError).

I don't think they should retry on GraphQLError here but it did indicate either side have a bug in code that needs to be fixed.

In principal you're right, but in practice I think customers might want to. e.g. if sparkcore fails to communicate with sparknode because it's restarting that would surface as a graphql error (with no error_name).

[zhenlu]

So basically:
RequestError: should retry
GraphQLError with empty Type: could retry
Other GraphQLError: don't retry.

@mgorven do you think they can retry on NoRoutes and CardPaymentError? If so I think we need to add a boolean field in the GraphQLError to indicate which one can be retried.

[mgorven]

@mgorven do you think they can retry on NoRoutes and CardPaymentError? If so I think we need to add a boolean field in the GraphQLError to indicate which one can be retried.

CardPaymentError is only used for billing, so not relevant for the SDK. I don't think that retrying NoRoute is going to help (if routefinder can't find a route it's not going to have one a few seconds later).

[zhenlu]

I can probably break the GraphQLError into two separate ones so one can retry while the other cannot.

[zhenlu]

RequestError: should retry
GraphQLInternalError: could retry
GraphQLError: don't retry

[jklein24]

RequestError: should retry GraphQLInternalError: could retry GraphQLError: don't retry

Can we document (in code) the error structs to this effect so that callers can see what to do with each type?

[jklein24]

@mgorven Do we ever return a 401 or 403 or are all http error codes deferred to graphql errors?

[zhenlu]

I don't think so. This error is early returned before parsing graphql response.

[jklein24]

Right, but I'm suggesting that really only 5XX errors should be retries, likely not all 4XX errors if our backend is returning those (which it seems to do)

[zhenlu]

Yeah that makes sense to me.

[mgorven]

We return 401 if an API token is provided but is invalid. It is also possible (but rare) for requests to fail with HTTP 500.

[zhenlu]

Merge activity

• Apr 11, 8:01 PM EDT: @zhenlu merged this pull request with Graphite.