craftcms / shopify

Synchronize and extend product data from your Shopify storefront.
MIT License
45 stars 25 forks source link

Issue with getDefaultVariant() Returning Null After Shopify Sync #112

Closed mauricepattyn closed 5 months ago

mauricepattyn commented 6 months ago

Hi! 👋

I've encountered an issue when running the command craft shopify/sync/products. Some products that have variants in Shopify return null for getDefaultVariant(), resulting in a TypeError. After syncing, I noticed that some products show an empty array [] for variants when using {{ dump(product.variants) }}. It seems like the sync is not fully propagating the variant data.

Steps Taken to Debug and Resolve: Rate Limit Handling: The issue was partly due to hitting Shopify's API rate limit, which is 2 requests per second for the REST Admin API. This caused incomplete data syncs. By implementing retry logic with exponential backoff, this rate limit issue was mitigated.

API Request Adjustment: Modified the API request handling to properly handle retries and delays when encountering rate limits. This adjustment ensures that all product variants are properly retrieved and stored.

Could you please advise if there might be additional configurations or settings required in Shopify to ensure all variant data is properly synced? Additionally, would it be possible to modify the getDefaultVariant() method to handle null values more gracefully to prevent errors?

Thank you for your assistance with this issue.

linear[bot] commented 6 months ago

PT-1733 Issue with getDefaultVariant() Returning Null After Shopify Sync

mauricepattyn commented 5 months ago

Update: I have made adjustments to handle Shopify's rate limit of 2 requests per second, which was causing incomplete syncs. This includes implementing retry logic with exponential backoff.

lukeholder commented 5 months ago

@mauricepattyn The shopify package we are using should be doing exponential backoff for us automatically, with the API returning a RETRY_AFTER header:

https://github.com/Shopify/shopify-api-php/blob/main/src/Clients/Http.php#L219-L225

Did you write your own backoff logic?

mauricepattyn commented 5 months ago

Hi @lukeholder

Thank you for your response.

I see now that the Shopify API package you're using already handles the exponential backoff automatically, including respect for the Retry-After header. I wasn't aware of this built-in functionality initially.

However, despite this, I am still encountering rate limit errors. The error message I receive is: Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service.

It seems that the current implementation may not be handling the rate limits as expected. I'm not an expert in backend development, as I primarily work as a front-end developer, but I've implemented my own backoff logic, which includes a delay after every two requests to ensure compliance with the 2 requests per second limit. This adjustment has resolved the issue on my end.

Here's the code I've implemented: ` public function get($path, array $query = []) { $maxRetries = 5; $retryCount = 0; $requestsMade = 0; $delayMicroseconds = 500000; do { try { $response = $this->getClient()->get($path, [], $query); $responseData = $response->getDecodedBody();

        if (isset($responseData['errors'])) {
            throw new \Exception('API Error: ' . json_encode($responseData['errors']));
        }

        $requestsMade++;

        if ($requestsMade >= 2) {
            usleep($delayMicroseconds);
            $requestsMade = 0;
        }

        return $responseData;
    } catch (\Exception $e) {
        if ($retryCount >= $maxRetries) {
            throw $e;
        }

        if ($e->getCode() == 429) {
            $retryAfter = $e->getResponse()->getHeaderLine('Retry-After') ?? 1;
            sleep((int)$retryAfter);
            $retryCount++;
        } else {
            throw $e;
        }
    }
} while (true);

} `

Could you please verify if the built-in logic is correctly handling the rate limit, or if there might be additional settings or configurations needed?

Thanks again for your assistance!

nathanworking commented 5 months ago

I consistently have this same issue and am unable to find a workaround

lukeholder commented 5 months ago

I consistently have this same issue and am unable to find a workaround

We have created a PR that fixes the 50 limit as well as adds a throttle option to the cli sync command:

https://github.com/craftcms/shopify/issues/118

We will merge and release it soon. Thanks.

nfourtythree commented 5 months ago

Hi

Version 5.2.0 of the plugin has now been released with this fix/update included. If you have issues with rate limiting after this update we have added a --throttle option to the command line sync command which will make this initial sync slower but will ensure all data is sync'd into Craft.

Continual syncing via web hooks in normal operation should not be affected.

Thanks!

siebird commented 3 weeks ago

@nfourtythree are we able to get a fix for this in 4.x as well? I'm currently hitting the API to determine what the default variant is as it's throwing exceptions on some shopify products

{% set defaultVariant = craft.shopify.api.getProductByShopifyId(product.shopifyId).variants[0] %}

Which then we sometimes hit a API limit depending on traffic