Countercache gives wrong result

[JelmerD]

This is a (multiple allowed):

• [x] bug

• [ ] enhancement

• [ ] feature-discussion (RFC)

• CakePHP Version: 2.9.4

• Platform and Target: Vagrant bento/debian-7.9

What you did

I added conditions to my counterCache

array(
    //...
    'counterCache' => array(
        'flight_count' => array(
            'Flight.status_id' => 3
        )
    ),
)

What happened

I'm getting a count result of 1000+

What you expected to happen

I expected a result of 24

Debugging

I did some debugging and it seems it has something to do with Model.php:2178

When I hard code $recursive to -1, I'm getting the expected result. This also applies when I set flight_count => true, as it was before. The array is empty, and thus $recursive will be set to -1 by the code, and therefore it works as expected.

All my models have these two settings as well:

public $actsAs = array('Containable');
public $recursive = -1;

[markstory]

What are the generated conditions for your model? The line you linked to allows conditions to be set on the HasMany association and has been in place for ~6 years. That amount of time either means conditions have effectively never worked or there is another problem.

[JelmerD]

@markstory

/Cake/Model/Model.php (line 2196)
array(
    'Flight.status_id' => (int) 3,
    '`Flight`.`user_plane_id`' => '7'
)

Which is weird as I now look at it, that is not the "user plane" I soft deleted at this current example.

Debugging gave me this:

L2175 is retrieving a value from $this->field($foreignKey), the value of this $foreignKey is extracted from $assoc at L2156. Which in turn has been defined in my own belongsTo definition. Therfore, the value of this variable is "user_plane_id".

So it will perform this find: $this->field('user_plane_id').

Quote from the docblock: Returns the content of a single field given the supplied conditions, of the first record in the supplied order.

So, at first I expected it to return the correct ID due to some "magic" Cake code, but as it doesn't give the correct value, I expected the first record from the database, but it returns the 4th. I checked $this->id and that has the value of false, on multiple places inside the updateCounterCache function.

Then at L2200 it is updating the fields where UserPlane.id = $keys[$foreignKey], which is 7, as found at L2175

So I'm trying to debug this, but I can't seem to find it.

@markstory again, the fact that I'm only noticing this just now is because I didn't do anything on this project for over a year. I think I have to debug it further and check what I'm doing wrong myself. But would anybody be so kind to exclude that it has to do something with the core?

[markstory]

If the generated conditions match 1000 records, then the code is doing all that it can.

[JelmerD]

The generated conditions are not matching the correct records. It should, but it isn't due to the $recursive value. Hard coding it to -1 fixes the count.

[markstory]

If the generated conditions are not correct, the resulting count can never be correct.

[JelmerD]

The conditions are correct, but recursive = 0, as forced in the Model, when conditions is not empty, is causing a wrong count.

Hard coding recursive to -1, is resulting in a correct count. So, because of the recursive = 0 part, the query is somehow returning too many rows.

So, why is the recursive setting messing up the find? Is this a behavior I should understand? Because I set the recursive to -1 in the AppModel, so why is it using a different recursive value, I have no idea.

Back at home I'll try to give it a test on a clean Cake project. Maybe via a test in the cakephp repo, and try to reproduce the problem.

[josegonzalez]

@JelmerD what is the entire query with and without recursive = -1?

[markstory]

So, why is the recursive setting messing up the find?

This can happen when a joinable association (HasOne, BelongsTo) modifies the result set in a way where many records are joined instead of a single record.

[JelmerD]

I found something else regarding this issue I find rather interesting, and I'm not sure if I'm doing something wrong here. Because I'm trying to debug if it lays in the core (I already found something and I will push it to my fork any moment)

But first things first.

I have a Flight, Flight belongsTo Plane. Plane hasMany Flight.

Now I'm doing an updateAll on Flight, with a list of ID's as the condition. So I'm bulk-updating several flights, I'm changing their status_id to 1 ("deleted").

Now I want to update all the associations that have a counterCache depending on Flight, so that their counterCaches get updated. In this example: the flight_count's at Plane should get updated.

So I call the following method:

$this->Flight->updateCounterCache($listOfFlightIds);
// note, I tried several things here, with a key, without a key, one key instead of an array, etc.

But I have to provide the foreignKey of the other model? So I apparantly I have to call:

$this->Flight->updateCounterCache(array('plane_id' => 1));
$this->Flight->updateCounterCache(array('plane_id' => 2));

But this seems weird to me, because the updateCounterCache method has a foreach loop that loops over all the belongsTo associations. Therefore, I have to provide ALL the foreignKey ids, for every single belongsToAssociation myself? If it depended on that particular flight.

Doesn't it make more sense if I provide the method with the ID of the record I just updated, and that the updateCounterCache method does the work for me?

So, something like:

$this->Flight->updateCounterCache([1,2,3]);

function updateCounterCache($ids) {
    // for every belongsTo $association
        // for every $ids as $id
            // $conditions = $this[$association FK] = $id AND $association[counterScope]
            // count $this where $conditions
            // update / save
}

If I look at _doSave and other places the updateCounterCache is used, it isn't going to work to just change this method. But am I right about this behavior? If so, isn't it an idea to add this feature?

[markstory]

But this seems weird to me, because the updateCounterCache method has a foreach loop that loops over all the belongsTo associations.

The loop over belongsTo associations is so that each related model has its counters updated if applicable.

When you call updateCounterCache() you have to supply data that looks like the model's natural data. While I can see how you would like to call updateCounterCache() with a list of ids to update, that is not how the method currently works. We would need to add another method to get the behavior you're looking for.

[markstory]

Any updates on this or should I close the issue?

[JelmerD]

No updates, can't seem to fix it. The only fix I got working was setting $recursive to -1 manually.

I can give you access to my code if you'd like, maybe you can see it immediately? Your call.