Make sure to always return a clean document from the cache

Development
1

Hi devs,

Directly modifying the XWikiDocument instance coming from the cache always end up causing some problems, we know that and that’s why we generally try to clone it before modifying it.

The problem is that it’s just generally, and we keep finding this mistake, in extensions or even in XWiki Standard (just identified and fixed a very old one) so I’m trying to find of a good way to make sure this does not happen again.

I can think of two possibilities, in XWikiCacheStore#loadXWikiDoc:

  1. always return a cloned version of the XWikiDocument
  2. before returning the XWikiDocument, check if it was modified (XWikiDocument#isMetadataDirty) and reload it from the database if that’s the case

WDYT ?

While 1. is obviously much simpler and safer, I’m afraid it might be too expensive in terms of performances, and it also might cause more retro compatibility problems (all the reasons why we never did it so far), so right now I would be pushing for 2. (but it’s not a -1 for 1. if the majority think it’s better).

Of course, it feels way too dangerous to apply that to any other branch than master.

2

Option 1 is probably overkill in lots of cases where we only access doc in readonly.
Option 2 sounds good, at least I don’t see what problem it could cause, and it might certainly fix some bugs indeed. Now maybe on the long run we could deprecate the standard API to get a doc and provide 2 distinct APIs for getting a doc in readOnly and getting a doc to write on it: first one wouldn’t perform a clone and be fast, and second one would always perform a clone. I don’t know at all if it’s a good idea, but I feel like it might be cleaner on the long run?

3

option 3: Return a ReadOnlyXWikiDocument instance which would extend XWikiDocument but reimplement all setters so that they return an exception, making it a read only instance.

This would negate the cost of the clone and the cost/complexity of reloading from the DB.

WDYT?

4

I don’t really understand your proposal, that’s a huge API breakage.

5

Maybe, but my main concern right now is actually to fix, I assume, quite a few bugs we just did not notice yet. Also, I would prefer this kind of new concept to go in a real revamp of the XWiki entity store interface (the long awaited new store…).