wetcat-studios / fortie

Laravel 5 package for Fortnox API
Apache License 2.0
19 stars 20 forks source link

Question: count() only returns @TotalResources from previous query #69

Closed Darrrren closed 3 years ago

Darrrren commented 3 years ago

A quick question for everyone... I have 10 customer records in my database.

When I run this command:

dump($fortie->customers()->timespan('2020-09-21 14:00:00')->unlimited()->all());

3 records are returned which is correct. Then, if I run:

dump($fortie->customers()->count());

this count() also returns 3, it's not querying the API for a fresh count, I was expecting 10.

How do I run a new count() on the Customers endpoint again on the same instance of Fortie? I would like these results:

// returns 3
dump($fortie->customers()->timespan('2020-09-21 14:00:00')->unlimited()->all()); 

// queries the API for a fresh count and returns 10
dump($fortie->customers()->count()); 

// gets all 10 records
dump($fortie->customers()->unlimited()->all()); 

As always, thanks for the help!

agoransson commented 3 years ago

That is interesting behavior.

I think the endpoint is simply returning an array though, so the count() is actually not an action on the API. At least I can't find any API method called 'count' in the docs.

The result must somehow be cached, and the count() method is simply acting on the cached array.

I guess what you want to do is:

$fortie->customers()->all()->count()

Darrrren commented 3 years ago

I've tried $fortie->customers()->all()->count() but with no luck, it returns the following error:

Call to undefined method Wetcat\Fortie\Providers\Customers\Resource::count()

I've done some more investigation on this today. It's definitely caching somewhere and you can replicate this with the following code, remember I have 10 records in my database:

// This is will return 10 records (correct)
$first_all = $fortie->customers()->unlimited()->all();

// Will return 3 as they have been updated since the timespan UTC date (correct)
$first_filtered = $fortie->customers()->timespan('2020-09-21 14:00:00')->unlimited()->all();

// Will return the same 3 as the line above despite requesting all records (wrong)
$second_all = $fortie->customers()->unlimited()->all();

//  Will return all 10 records as we've stipulated an impossibly early timespan to replicate fresh query (correct)
$second_filtered = $fortie->customers()->timespan('1900-01-01 00:00:00')->unlimited()->all();

// Returns 10 records (correct because of the previous query)
$third_all = $fortie->customers()->unlimited()->all();

All on the same instance of $fortie. It looks like the timespan() method possibly forces a fresh API query. My next job is to try and understand if it's something in my system that's caching, or it's something in this package and how to stop it.

Can anyone replicate this?

agoransson commented 3 years ago

Excellent plan!

Have you tried the same on another provider? Perhaps users, or invoices or something.

Darrrren commented 3 years ago

Yes actually I have, I've tried it on accounts() of which there are over 1000 records and I'm seeing exactly the same problem. Is this normal behaviour?

agoransson commented 3 years ago

I had a quick look in the source and found a trait I had missed - so it's actually not arrays as my initial thought was. It's a request.

/src/Traits/CountTrait.php

@viktormaar perhaps you have an idea about this behavior.

Darrren, perhaps you can paste the GET request that is beeing built?

See line 30 in CountTrait.php, print out the request that is beeing built there.

agoransson commented 3 years ago

@Darrrren I think I realized now what the issue is, I'm looking in FortieRequest.php and we're actually storing a bunch of stuff in private variables - those fields are not cleared on each new request (unless we explicitly set them).

This is a major bug I think and it should be dealt with post-haste!

  /**
   * Build the Guzzle request from the FortieRequest object.
   */
  public function build ()
  {
    // Start building the URL
    $this->URL = 'https://api.fortnox.se/3/';

    // Add the extra paths, if there are any
    if (!is_null($this->paths)) {
      // If array, add all paths
      if (is_array($this->paths)) {
        foreach ($this->paths as $path) {
          $this->URL .= $path . '/';
        }
      }
      // Otherwise, add just the first
      else {
        $this->URL .= $this->paths . '/';
      }
    }

    // Add the optional filter to params
    if (!is_null($this->optionalFilter)) {
      if (is_array($this->optionalFilter)) {
        $this->params = $this->optionalFilter;
      } else {
        $this->params['filter'] = $this->optionalFilter;
      }
    }

    // Apply the URL parameters, this must be an associative array
    if (!is_null($this->params) && is_array($this->params)) {
      $i = 0;
      foreach ($this->params as $key => $param) {
        // ?
        if ($i == 0) {
          $this->URL .= '?' . $key . '=' . $param;
        }
        // &
        else {
          $this->URL .= '&' . $key . '=' . $param;
        }
        $i++;
      }
    }

    return $this;
  }

So, if you can print out the two requests then I think we'll see that the unwanted filters are set on the second request as well, even if you didn't explicitly set them.

viktormaar commented 3 years ago

As I see after a quick debug, the 'lastmodified' parameter remains after a filtered query. I´m working on it.

viktormaar commented 3 years ago

My previous comment was not complete. Not only lastmodified, but some other parameters like filter can remain. It´s because these are stored as provider properties, but the providers are usually constructed once.

I believe that the most fail-safe fix is if we change the Fortie.php as the following: 1) Provider properties will be removed 2) The constructor will store only the Guzzle client

// Set up Guzzle client
$this->client = new \GuzzleHttp\Client(array_merge([...], $config));

3) The methods will construct the provider class every time they are called

public function customers ()
{
  return new CustomersProvider($this->client);
}

What do you think @agoransson?

Darrrren commented 3 years ago

Nice catch @viktormaar and @agoransson thanks. Looking at the PR I think that will fix it for sure, looking forward to testing it ;-)

agoransson commented 3 years ago

I'm all for removing private fields.