[xwiki-devs] [VOTE] Clirr difference for fixing content dirty flag.
Hi, I am requesting a vote for adding the following clirr difference: <difference> <className>com/xpn/xwiki/objects/ListProperty</className> <field>list</field> <differenceType>6006</differenceType> <justification>Elemenents must be added to the wrapping NotifyOnUpdateList to ensure that the property is marked 'dirty' when updated. To avoid that this mechanism is circumvented, the field is made final.</justification> </difference> The flag isContentDirty in XWikiDocument is currently always true when a document is loaded from the database. This means that a script that just load and save a document without modifying it will generate a new version, and the point of the flag is completely lost. It is also a blocker for my work in the feature-authorization-context branch. Fixing this was non-trivial. It is assumed everywhere that the content dirty will be set, not only when the document content changes, but also when a property, the xclass or any attachment changes. But there were no code that actually set the flag on updates (because it was always true anyway). What's worse is that we have API methods that returns lists that are live updateable. I have added a wrapper list class to detect the updates in these lists. Hibernate refuse to store elements from my wrapper list for some reason which forced me to add a workaround for hibernate. Surprisingly, Clirr doesn't require any exception for the below methods which I have added, but I will list them here for your information: In BaseClass: public void setOwnerDocument(XWikiDocument ownerDocument) public void setDirty(boolean isDirty) In BaseProperty: public boolean isValueDirty() protected void setValueDirty(Object newValue) public void setValueDirty(boolean valueDirty) public void setOwnerDocument(XWikiDocument ownerDocument) In ListProperty: public void setUseHibernateWorkaround(boolean useHibernateWorkaround) In XWikiAttachmentContent: public void setOwnerDocument(XWikiDocument ownerDocument) New class: AbstractNotifyOnUpdateList<E> Best Regards, /Andreas
On Mon, Nov 5, 2012 at 8:25 AM, Andreas Jonsson <[email protected]> wrote:
Hi,
I am requesting a vote for adding the following clirr difference:
<difference> <className>com/xpn/xwiki/objects/ListProperty</className> <field>list</field> <differenceType>6006</differenceType> <justification>Elemenents must be added to the wrapping NotifyOnUpdateList to ensure that the property is marked 'dirty' when updated. To avoid that this mechanism is circumvented, the field is made final.</justification> </difference>
+1
The flag isContentDirty in XWikiDocument is currently always true when a document is loaded from the database. This means that a script that just load and save a document without modifying it will generate a new version, and the point of the flag is completely lost. It is also a blocker for my work in the feature-authorization-context branch.
Fixing this was non-trivial. It is assumed everywhere that the content dirty will be set, not only when the document content changes, but also when a property, the xclass or any attachment changes. But there were no code that actually set the flag on updates (because it was always true anyway). What's worse is that we have API methods that returns lists that are live updateable. I have added a wrapper list class to detect the updates in these lists.
Hibernate refuse to store elements from my wrapper list for some reason which forced me to add a workaround for hibernate.
Surprisingly, Clirr doesn't require any exception for the below methods which I have added, but I will list them here for your information:
This is normal, because adding new methods to a class (not interface) doesn't break (binary) compatibility.
In BaseClass:
public void setOwnerDocument(XWikiDocument ownerDocument)
BaseClass is extended by PropertyMetaClass which defines the meta properties of an XClass property, like dateFormat, multiSelect, disabled, etc. Owner document doesn't make sense for a meta class, but I guess this is not a problem.
public void setDirty(boolean isDirty)
You need to fix the Javadoc for both methods. @param doesn't match the actual parameter name.. Best is to activate Checkstyle from your IDE to see these errors right away.
In BaseProperty:
public boolean isValueDirty() protected void setValueDirty(Object newValue) public void setValueDirty(boolean valueDirty)
public void setOwnerDocument(XWikiDocument ownerDocument)
You need to fix the Javadoc here too. Also, BaseProperty is the base class for all raw types (StringProperty, IntegerProperty, etc.) which are not bound to any document so owner document doesn't make sense for them. It makes sense only for XClass properties (which extend PropertyClass, which is derived from BaseClass). Sergiu or Ludovic, who know this part of the code better, should tell us if adding this method here is fine.
In ListProperty:
public void setUseHibernateWorkaround(boolean useHibernateWorkaround)
Could you improve the Javadoc? It doesn't explain anything, it doesn't say which workaround. The old core is already very cryptic.. Thanks, Marius
In XWikiAttachmentContent:
public void setOwnerDocument(XWikiDocument ownerDocument)
New class: AbstractNotifyOnUpdateList<E>
Best Regards,
/Andreas
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
Hi, Marius! 2012-11-07 09:39, Marius Dumitru Florea skrev:
On Mon, Nov 5, 2012 at 8:25 AM, Andreas Jonsson <[email protected]> wrote:
Hi,
I am requesting a vote for adding the following clirr difference:
<difference> <className>com/xpn/xwiki/objects/ListProperty</className> <field>list</field> <differenceType>6006</differenceType> <justification>Elemenents must be added to the wrapping NotifyOnUpdateList to ensure that the property is marked 'dirty' when updated. To avoid that this mechanism is circumvented, the field is made final.</justification> </difference> +1
The flag isContentDirty in XWikiDocument is currently always true when a document is loaded from the database. This means that a script that just load and save a document without modifying it will generate a new version, and the point of the flag is completely lost. It is also a blocker for my work in the feature-authorization-context branch.
Fixing this was non-trivial. It is assumed everywhere that the content dirty will be set, not only when the document content changes, but also when a property, the xclass or any attachment changes. But there were no code that actually set the flag on updates (because it was always true anyway). What's worse is that we have API methods that returns lists that are live updateable. I have added a wrapper list class to detect the updates in these lists.
Hibernate refuse to store elements from my wrapper list for some reason which forced me to add a workaround for hibernate.
Surprisingly, Clirr doesn't require any exception for the below methods which I have added, but I will list them here for your information: This is normal, because adding new methods to a class (not interface) doesn't break (binary) compatibility.
Thanks for clarifying!
In BaseClass:
public void setOwnerDocument(XWikiDocument ownerDocument) BaseClass is extended by PropertyMetaClass which defines the meta properties of an XClass property, like dateFormat, multiSelect, disabled, etc. Owner document doesn't make sense for a meta class, but I guess this is not a problem.
public void setDirty(boolean isDirty)
You need to fix the Javadoc for both methods. @param doesn't match the actual parameter name.. Best is to activate Checkstyle from your IDE to see these errors right away.
In BaseProperty:
public boolean isValueDirty() protected void setValueDirty(Object newValue) public void setValueDirty(boolean valueDirty) public void setOwnerDocument(XWikiDocument ownerDocument) You need to fix the Javadoc here too. Also, BaseProperty is the base class for all raw types (StringProperty, IntegerProperty, etc.) which are not bound to any document so owner document doesn't make sense for them. It makes sense only for XClass properties (which extend PropertyClass, which is derived from BaseClass).
Sergiu or Ludovic, who know this part of the code better, should tell us if adding this method here is fine.
The "owner document" is optional and may be set to null wherever it doesn't make sense to have it. This is also true for BaseClass above. The important thing is that it is set correctly when a property is associated with an xobject that is associated with a document. Best Regards, /Andreas
In ListProperty:
public void setUseHibernateWorkaround(boolean useHibernateWorkaround) Could you improve the Javadoc? It doesn't explain anything, it doesn't say which workaround. The old core is already very cryptic..
Thanks, Marius
In XWikiAttachmentContent:
public void setOwnerDocument(XWikiDocument ownerDocument)
New class: AbstractNotifyOnUpdateList<E>
Best Regards,
/Andreas
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
participants (2)
-
Andreas Jonsson -
Marius Dumitru Florea