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:
always return a cloned version of the XWikiDocument
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.
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?
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.
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…).
What about adding a read-only flag in XWikiDocument (or use an existing flag) and let all setters and other methods that modify XWikiDocument throw an exception when the flag is set?
I agree that option 1 is going to cause too much overhead (in fact, we have exactly this logic to clone the document when a modification shall be made already in many places).
I’m also fine with option 2, the problem I see there is that it will be impossible to know which code modified the document. While it hides some bugs, it doesn’t feel like a good solution that allows us to fix these kinds of bugs, in particular as the “fix” won’t always work.
I don’t understand the relationship which this proposal. This would only make sense with a new API, we cannot make XWiki#getDocument return a readonly document as it would be a huge retro compatibility problem. As I mentioned in a previous message, the focus of this proposal is not to discuss better new APIs, but to fix common problems with existing code as much as we can.
To me, the document returned by XWiki#getDocument is not supposed to be modified, any code modifying such a document without cloning it is violating the API contract. So to me, this wouldn’t be a breakage, we would just enforce the existing API. If we don’t want to go as far as throwing an exception, we could at least start logging something if this is the case (could be a debug message with a stack trace that we explicitly enable on CI) so we can find the code that modifies the document. Otherwise, we’ll just make the issues that are caused by modifying cached document instances harder to reproduce, but they won’t go away and might still appear, e.g., in situations of high load.
It’s definitely not part of the contract of that API. All code used to directly modify the XWikiDocument returned by XWiki#getDocument and a lot of code still do (especially in contrib extensions), we just started at some point highly recommending cloning that XWikiDocument in case of modificatrion to avoid concurrently problems.
I’m not against adding some log, it’s a good idea. We could log a warning in setMetaDataDirty(true) if this.cached is true for example. But I still feel we should try to improve the situation as much as possible, option 2 won’t fix everything, but it should help a lot.
As I’ve said above, I’m fine with option 2, to make it clear: +1 for option 2.
I’m also fine with logging a warning, I was just a bit worried that it might flood the logs on production instances without admins being able to do anything. In any case, we need to also log a stack trace (like in an additional debug log), otherwise the warning is not that useful.
Actually, I was thinking about making those warnings tied to the deprecation log, so an admin could disable them using logging.deprecated.enabled=false.