search

zf-fr / zfr-oauth2-server

PHP library for creating an OAuth 2 server (currently proof of concept)
BSD 3-Clause "New" or "Revised" License
36 stars 13 forks source link

Second level cache does not work #30

Closed bakura10 closed 9 years ago

bakura10 commented 9 years ago

Hi,

I'm testing the current master branch that features second level cache, but for no reasons, I don't manage to make this work. I suppose I do not understand completely how second level cache work.

I'm trying to exploit second level cache at token level, as token are 100% read-only, and retrieved at each request.

The configuration is here: https://github.com/zf-fr/zfr-oauth2-server/blob/master/config/doctrine/ZfrOAuth2.Server.Entity.AbstractToken.dcm.xml#L9

Notice that there is an association here to the token owner: https://github.com/zf-fr/zfr-oauth2-server/blob/master/config/doctrine/ZfrOAuth2.Server.Entity.AbstractToken.dcm.xml#L13

But I don't want to cache the association, obviously (because the user may change often). While debugging, I realized the save to cache is done here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php#L134

However, when I var_dump the cache entry, this is a plain array but with no reference to the owner id. Obviosuly, when the entity is retrieved from cache, the association is completely lost.

What did I misunderstood?

bakura10 commented 9 years ago

ping @guillhermoblanco @FabioBatSilva

As you told me on IRC, I have done more test, and I have done a var_dump on data here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Cache/DefaultEntityHydrator.php#L106

As suspected, the association id is not saved.

If I comment this line: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Cache/DefaultEntityHydrator.php#L75

then the association is saved as an AssociationCacheEntry with the id, which is what I want. I do not want to do any modification because I have no idea how it should work.

To my understanding, I SHOULD NOT have to specify the "Cache" annotation on the association because:

  1. It would be highly redundant if we had to add this annotation to each association.
  2. According to doc, if I add "Cache" annotation on association, it must also be present on related entity, and I don't want to cache at all the related entity.

I think that for OneToOne and ManyToOne association, the association must be always cached. Could you please check that? I don't want this to be a major bug of Doctrine 2.5 :p

bakura10 commented 9 years ago

ping @guilhermeblanco

FabioBatSilva commented 9 years ago

@bakura10 please see https://github.com/FabioBatSilva/doctrine2/commit/4f7e71963f5197235fce9f87a56b02bbbba46026

You need the @Cache configuration in each association property you want to cache. in your case it will cause the Token and the assoc Token#client to be cached, but not the Client it self.

bakura10 commented 9 years ago

But how does this make sense? If you don't cache ToOne association, this means you will ALWAYS stored partial objects in your cache, right? And it seems that Doctrine does not resolve the association after, it sounds like a not sane default :p.

bakura10 commented 9 years ago

@guilhermeblanco what is your position on this? I'm sorry but I still feel very bad with the current solution of Doctrine 2 and I am afraid that you will receive tons of issues of people saying why SL2 does not work whenever you have one association. As I said, for me the correct behaviour would be:

Currently, as I said, the problem is that you are FORCED to check all your associations, and manually add the cache. It is as fragile as "partial" when fetching entities from repositories. If you add one association and forget to add the @Cache, boom, it fails.

@FabioBatSilva could you please explain me the exact rationale behind this? Or at least it should be made very clearly in the doc that as soon as you have an association, you HAVE to add the @Cache association, unless you are 100% sure that you won't need to retrieve the association once you fetch it from cache again.

ping @Ocramius also

bakura10 commented 9 years ago

Never mind, I think I see the use case, in the sense that the association could be changed, obviously. Stupid me...

However, there is still one problem: in my code, if I don't set the @Cache, then accessing the association once we fetch it it fails. Actually Doctrine does not seem to generate any proxy at all.

Ocramius commented 9 years ago

Please raise these concerns on the doctrine2 issue tracker if they affect the ORM, not here. On Feb 11, 2015 8:29 AM, "Michaël Gallego" notifications@github.com wrote:

@guihermeblanco what is your position on this? I'm sorry but I still feel very bad with the current solution of Doctrine 2 and I am afraid that you will receive tons of issues of people saying why SL2 does not work whenever you have one association. As I said, for me the correct behaviour would be:

  • If no @Cache is defined on association level, Doctrine should cache identifiers to TO_ONE associations.
  • If a @Cache is defined on association level but is not defined on target entity, it does nothing.
  • If a @Cache is defined on association level and is defined on target entity, it should cache the entity with the entire association entity (not just the id).

Currently, as I said, the problem is that you are FORCED to check all your associations, and manually add the cache. It is as fragile as "partial" when fetching entities from repositories. If you add one association and forget to add the @Cache, boom, it fails.

@FabioBatSilva https://github.com/FabioBatSilva could you please explain me the exact rationale behind this? Or at least it should be made very clearly in the doc that as soon as you have an association, you HAVE to add the @Cache association, unless you are 100% sure that you won't need to retrieve the association once you fetch it from cache again.

ping @Ocramius https://github.com/Ocramius also

— Reply to this email directly or view it on GitHub https://github.com/zf-fr/zfr-oauth2-server/issues/30#issuecomment-73847537 .

bakura10 commented 9 years ago

Exactly. I'm closing, I'm going to make more tests too, as from @FabioBatSilva , it should work (but definitely does not :().