Make query cache rely on entity timestamp region

[lcobucci]

As far as I saw that's the default Hibernate behavior (https://vladmihalcea.com/2015/06/08/how-does-hibernate-query-cache-work/) to keep the query cache consistent.

I'm just not sure if that is the way to go, basically because of the change on the CachedEntityPersister interface (I didn't want to duplicate the logic that generates the timestamp key).

[lcobucci]

I had an idea to improve this PR a bit by moving the timestamp region check to the query cache validator but this also means moving the timestampKey to the QueryCacheKey.

I think it's a better solution, what's your opinion?

[guilhermeblanco]

@lcobucci indeed that's a MUCH BETTER solution. If I understood your idea correctly, you'd eliminate the BC break by adding a new interface method.
This means it could potentially be part of 2.X series instead of 3.X only. =)

If you're willing to work on that, I'd appreciate.

[lcobucci]

@guilhermeblanco I really need this to be part of 2.X so I'm glad to help 😉

If I understood your idea correctly, you'd eliminate the BC break by adding a new interface method.

My idea basically is:

1. Add timestampRegion to the TimestampQueryCacheValidator constructor
2. Add timestampKey to the QueryCacheKey constructor
3. Make TimestampQueryCacheValidator#isValid() also validate the timestamp using the region and the key

I couldn't see the BC break you said, can you please elaborate a bit more?

The biggest question here is: what'd be the best place to retrieve the timestampKey?
Passing it to the QueryCacheKey constructor means that we need to have it available also on the AbstractQuery#executeUsingQueryCache().

Moving those things to que validator also simplifies the AbstractEntityPersister#getHash() logic, since we don't need concatenate the $timestamp anymore (and we could also remove the unused timestampRegion from the persister).

[Ocramius]

bc break, sadly

[Ocramius]

Also, I don't know how to regenerate a timestamp key, if not retrieved from here.

[lcobucci]

Ow ofc. Do you think that the solution I explained is applicable?

[Ocramius]

@lcobucci moving more info to the QueryCacheKey seems good to me, since it's information that sticks together anyway. I don't have the full picture though, sorry.

[lcobucci]

@guilhermeblanco @Ocramius introduced a tiny duplication in order to remove the BC break. I think it should be ok to be merged on v2.X now.

Anything else?

[Ocramius]

The docblock states that it is never null: can you check that?

[lcobucci]

The docblock is a liar! Will fix that, thanks

[lcobucci]

@guilhermeblanco @Ocramius comments processed let me know if there's anything else preventing this to be merged and let's

[Ocramius]

Since there was a change here, did you check all usages of this property?

[lcobucci]

I did but I definitely forgot to change ($entry->time + $key->lifetime) > time() to ((int) $entry->time + $key->lifetime) > time() on the validator

[Ocramius]

This goes into 2.5.5! Thanks @lcobucci

master: 009e947
2.5: e16de70