openfoodfacts / openfoodfacts-laravel

Open Food Facts API wrapper for Laravel
MIT License
148 stars 17 forks source link

Do not cache product not found responses when calling OpenFoodFacts::barcode() #11

Closed khalyomede closed 2 years ago

khalyomede commented 2 years ago

Epic: #

Description

I tried to pass a product bar code number that did not existed yet, then added it by contributing through the OpenFoodFact Android App, then called the API again and still no product found.

I flushed my cache, called the method again, and this time the correct response is returned.

Acceptance criteria

Only cache results for product found, do not cache product not found results (to allow subsequent retries to hopefully find the product if a contributor add it afterward).

What would a demo look like

  1. Setup a Laravel cache driver (file, database, ...)
  2. Call OpenFoodFacts::barcode($code);, where $code is a bar code of a product not in the database yet
  3. Add the product (either through the mobile app or the website)
  4. Call OpenFoodFacts::barcode($code);, where $code is a bar code of the product just added
  5. No exception "Product not found", the result is the product JSON response

Notes

I noticed the cache is set after the JSON response it fetched. I did not dived into it in details, but if it is possible to figure out if the product is not found at this point, maybe we should prevent caching at this level?

https://github.com/openfoodfacts/openfoodfacts-php/blob/develop/src/Api.php#L431-L435

Edit: In fact, I just tried to dd at this point, by testing with product number "3760314500074", and the response is the following:

[
  "code" => "3760314500074"
  "status" => 0
  "status_verbose" => "product not found"
]

So the fix for me (if you think it make sense), would be to update the code like this

class Api
{
  // ...

  private function fetch(string $url, bool $isJsonFile = true): array
  {
    // ...

    $jsonResult = json_decode($response->getBody(), true);

-   if (!empty($this->cache) && !empty($jsonResult)) {
+   if (!empty($this->cache) && !empty($jsonResult) && (($jsonResult["status"] ?? 0) !== 0) {
      $this->cache->set($cacheKey, $jsonResult);
    }
  }
}

Tasks

epalmans commented 2 years ago

/CC @Dwarfex

As this targets the upstream repo: we are open to pr’s at https://github.com/openfoodfacts/openfoodfacts-php

khalyomede commented 2 years ago

For history purpose, here is the link to the PR on the openfoodfacts-php repository:

https://github.com/openfoodfacts/openfoodfacts-php/pull/38