GraphService errors should throw a ShopifyGraphResultException

[nozzlegear]

Right now the GraphService will throw a ShopifyHttpException if a response has errors. However, this can cause confusion because all graph responses return 200 OK, but the exception type has an HttpStatus 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.

[clement911]

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?

[adearriba]

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.

[clement911]

Maybe the following hierarchy would be appropriate?

• ShopifyException
  • ShopifyApiException
    • ShopifyAdminApiException
      • ShopifyAdminApiRestException
      • ShopifyAdminApiGraphException
    • ShopifyPartnerApiException
    • ShopifyStorefrontApiException

[adearriba]

Hi @clement911, I have two questions:

1. Why would we need ShopifyApiException? There won't be other types of ShopifyException that are not Api Exceptions, so we can reduce the hierarchy.
2. What is the value of separating by AdminApiGraph, Partner and StoreFront if they are all graphql APIs?

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.

[clement911]

@adearriba

1. Yes there can be non-api exceptions such as serialization exceptions, null reference exception and any kind of exceptions related to other bugs. While ShopifySharp is a fairly thin wrapper around the APIs, there is still some code which can have bugs and exceptions.
2. I'm not sure if there is any value. I'm not sure what the best design but that I was just offering a potential design. Maybe there should be just be a 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.

[adearriba]

Thanks, @clement911.

1. For me, an exception that doesn't come from Shopify API should be treated as a library exception ShopifySharpException instead to avoid confusion.
2. I also don't think ShopifyGraphException should inherit from ShopifyHttpException. They both should inherit from ShopifyException

In summary, the hierarchy I see is:

• ShopifySharpException (treat library exceptions with the inner exception of what happened)
• ShopifyException (To group all Shopify API exceptions regardless of type)
  • ShopifyGraphException (Specific to Graph error results)
  • ShopifyRestException (Specific to REST errors / Http Status codes)

Anyways, you guys have done an excellent job with the library, I just wanted to add my 2 cents.