Open nozzlegear opened 7 months ago
I've been caught by this as well.
However, if this new exception type doesn't inherit from ShopifyHttpException
, this might be an issue as well.
One might catch ShopifyHttpException
(as I do) and expect to get all shopify API errors, but would not.
I wanted to catch Shopify internal error. I solved it with a simple extension.
public static bool IsShopifyInternalError(this ShopifyHttpException shEx)
{
return shEx.HttpStatusCode == HttpStatusCode.InternalServerError ||
//graphql error returns HTTP 200 with the error desc in the response
(shEx.HttpStatusCode == HttpStatusCode.OK && shEx.RawBody?.Contains("INTERNAL_SERVER_ERROR") == true);
}
I'm curious under what scenario you need to filter for specific HttpStatus
?
My point of view:
They should all extend ShopifyException
. GraphQL errors have a different schema so we should be able to get that information if we get one.
GraphQl Error example:
{
"errors": [
{
"message": "Query cost is 2003, which exceeds the single query max cost limit (1000).See https://shopify.dev/concepts/about-apis/rate-limits for more information on how thecost of a query is calculated.To query larger amounts of data with fewer limits, bulk operations should be used instead.See https://shopify.dev/tutorials/perform-bulk-operations-with-admin-api for usage details.",
"extensions": {
"code": "MAX_COST_EXCEEDED",
"cost": 2003,
"maxCost": 1000,
"documentation": "https://shopify.dev/api/usage/rate-limits"
}
}
]
}
The proper way to handle would be to catch ShopifyException
exceptions and then filter what type you want using is
if you are interested in additional details.
try
{
...
}
catch (ShopifyException exception)
{
if (exception is ShopifyHttpException httpException)
{
//use httpException variable which will be casted to ShopifyHttpException
}
else if (exception is ShopifyGraphResultException graphqlException)
{
//use graphqlException variable which will be casted to ShopifyGraphResultException
}
}
Not sure how you both feel about this moving forward. This can be treated as a breaking change if people are just using ShopifyHttpException
for all.
Maybe the following hierarchy would be appropriate?
Hi @clement911, I have two questions:
ShopifyApiException
? There won't be other types of ShopifyException
that are not Api Exceptions, so we can reduce the hierarchy.I would try to keep the hierarchy simple, but that's just my opinion. I want to understand better your point of view in case I'm missing something.
@adearriba
ShopifyRestException
and a ShopifyGraphException
. Graph errors usually still return an HTTP 200 OK, so it feels a bit weird to have those inherit from ShopifyHttpException
.Thanks, @clement911.
ShopifySharpException
instead to avoid confusion. ShopifyGraphException
should inherit from ShopifyHttpException
. They both should inherit from ShopifyException
In summary, the hierarchy I see is:
Anyways, you guys have done an excellent job with the library, I just wanted to add my 2 cents.
Right now the GraphService will throw a
ShopifyHttpException
if a response has errors. However, this can cause confusion because all graph responses return200 OK
, but the exception type has anHttpStatus
property on it. If you're guarding for e.g. certain permission errors by checking the status code, your guard will always miss. In 7.0, the GraphService should throw a unique graph exception that does not inherit from the http exception.