[xwiki-devs] [VOTE] Temporary make sure a BaseObject cannot target a class coming from another wiki
Hi devs, we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628. The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references. Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference. So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName. Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki ** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference WDYT ? Here is my +1 -- Thomas Mortagne
On Mon, Mar 26, 2012 at 15:43, Thomas Mortagne <[email protected]>wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki ** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
+1, astonishing no :)
Here is my +1
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
Hi, This looks like a very complex topic. Trying to answer below (or rather ask questions)… On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part. Are we talking about the same thing?
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there. Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
On Mon, Mar 26, 2012 at 18:21, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference.
There is nothing wrong on the long run, when we get a store that support it. But for now, this is always the sign of a mistake. And this mistake has been hidden by the current implementation, and is now coming to surface.
This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
This is nothing new or recent, it has always been that way. XClass reference may be relative to the document containing the XObjects, which allows cloning document between wikis without necessarily creating XObjects that reference classes from the source wiki (Self containing document is also workable, and space relative as well). Using absolute reference is not wrong, but create absolute reference to the XClass even when the XObject is cloned. This has been voted when the API has been defined. This works in the same way parent reference works.
Are we talking about the same thing?
Probably not? We are not changing the API in fact. But since our current store only support local XClass, and that referencing external XClass get finally converted to local XClass by the implementation at the store level, users (developers) of the API has not noticed they are incorrectly using absolute references. So, what we want to do temporarily is keep this wrong behavior doing it right with a warning in the logs, to allow gently adapting existing unnoticed wrong usage. Not only ours, but also those of users accessing this API. All it breaks is a feature that do not work anyway with the current store. Moreover the current store create absolute references while it store relative one. This is obviously a mistake that stay unnoticed. The same is true for the add XObject action. Said another way, currently you have potentially different absolute references that finally look at the same XClass, but potentially this XClass is loaded several time with different references, since the wiki part is not taken into account by the store. This unexpected feature does not work anymore in 4.x since we use Object reference to compute IDs.
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This is precisely not what we want at the end. This is currently a limitation of the store, not a limitation of the object API. Since the object API has been written to support this, it would be a pity to abandon it because the store currently does not allow it.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document. Here is the javadoc you wrote in BaseCollection: /** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference; It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended. So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used Here we are talking proposing 3)
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :) However I'd like to suggest a different solution. Premises: * 99% of the use cases are for relative references * We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right * We have never supported absolute references to XClasses (since we've never supported this in the DB) Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference()) * Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute). This will ensure that users of the api can't use it wrongly. I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it. Thanks -Vincent On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Wed, Mar 28, 2012 at 9:42 AM, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references * We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right * We have never supported absolute references to XClasses (since we've never supported this in the DB)
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference()) * Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
So basically you vote for 2)
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject. This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ? On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
On Mar 28, 2012, at 10:49 AM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
It is! This is even the reason of the vote: because some places of the code use wrongly an absolute ref instead of a relative ref...
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject.
I don't recall this. Can you give me the link? Also if it his is true I'm suggesting to vote again on this.
This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ?
Nothing should be reverted IMO, it's ok to pass a full reference even if only the relative part will be saved in the wiki. In the future, **if** we ever want to add support for absolute xclass ref in the DB then we'll have another api and we will then be able to modify the places that want to support absolute refs to use the new api.
On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
It's simple IMO. Most of our APIs return DocumentReference, ie absolute refs. So for example imagine I want to use a XClass defined in a given doc, I'll use: doc.getDocumentReference() as the XClass reference. We don't have a ClassReference object and so far we use DocumentReference == ClassReference. Thanks -Vincent
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
On Wed, Mar 28, 2012 at 11:00, Vincent Massol <[email protected]> wrote:
On Mar 28, 2012, at 10:49 AM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
It is! This is even the reason of the vote: because some places of the code use wrongly an absolute ref instead of a relative ref...
Not the code of the API, but recent code using the API like the sheet binding mechanism. There was a bug in the store, but this is really not the story.
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject.
I don't recall this. Can you give me the link?
http://markmail.org/message/yggcayev5hvgv342
Also if it his is true I'm suggesting to vote again on this.
Going back after a vote, is really not good IMO, unless we are really sure, so not in hurry.
This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ?
Nothing should be reverted IMO, it's ok to pass a full reference even if only the relative part will be saved in the wiki. In the future, **if** we ever want to add support for absolute xclass ref in the DB then we'll have another api and we will then be able to modify the places that want to support absolute refs to use the new api.
You are wrong, we take care of passing relative reference in many places, and later we need to go back to an absolute one, since relative reference are not workable with. So leaving all this code if we are sure it is useless is not appropriate. But what I means mainly, is that we have written these line of codes (and test) just because of the vote above, and now you just propose to throw it away.
On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
It's simple IMO. Most of our APIs return DocumentReference, ie absolute refs. So for example imagine I want to use a XClass defined in a given doc, I'll use: doc.getDocumentReference() as the XClass reference.
Yes, but maybe we just need a doc.getDocumentReference().toRelative(EntityType.WIKI) for the purpose.
We don't have a ClassReference object and so far we use DocumentReference == ClassReference.
Thanks -Vincent
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the uniquetemporary identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
On Mar 29, 2012, at 12:07 PM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 11:00, Vincent Massol <[email protected]> wrote:
On Mar 28, 2012, at 10:49 AM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
It is! This is even the reason of the vote: because some places of the code use wrongly an absolute ref instead of a relative ref...
Not the code of the API, but recent code using the API like the sheet binding mechanism. There was a bug in the store, but this is really not the story.
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject.
I don't recall this. Can you give me the link?
Two points about this: 1) This was not a vote 2) I wrote this proposal and I remember it very well and I've never proposed to allow having an XClass in one wiki and an XObject in another wiki.
Also if it his is true I'm suggesting to vote again on this.
Going back after a vote, is really not good IMO, unless we are really sure, so not in hurry.
It's never been voted nor even proposed so no issue at all… :)
This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ?
Nothing should be reverted IMO, it's ok to pass a full reference even if only the relative part will be saved in the wiki. In the future, **if** we ever want to add support for absolute xclass ref in the DB then we'll have another api and we will then be able to modify the places that want to support absolute refs to use the new api.
You are wrong, we take care of passing relative reference in many places, and later we need to go back to an absolute one, since relative reference are not workable with. So leaving all this code if we are sure it is useless is not appropriate. But what I means mainly, is that we have written these line of codes (and test) just because of the vote above, and now you just propose to throw it away.
I 've never written any line of code because I wanted to support having an XClass in one wiki and an XObject in another one. That's not true. It would have been stupid to propose something that cannot be implemented. I would not have done this. My strategy is to not propose something for the future because of YAGNI unless in some special case where I'm 100% sure it'll be needed and not implementing it would cause issues in the future. Now it's possible that this proposal had a side effect that lead to allowing this but it doesn't mean we wanted to support it (and we were not supporting it since we cannot support it currently).
On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
It's simple IMO. Most of our APIs return DocumentReference, ie absolute refs. So for example imagine I want to use a XClass defined in a given doc, I'll use: doc.getDocumentReference() as the XClass reference.
Yes, but maybe we just need a doc.getDocumentReference().toRelative(EntityType.WIKI) for the purpose.
We can already do this so that's not the issue IMO. My preference still goes to 2) ATM but if the majority prefers 3) I'll vote +0 for 3) :) Thanks -Vincent
We don't have a ClassReference object and so far we use DocumentReference == ClassReference.
Thanks -Vincent
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote:
Hi,
This looks like a very complex topic. Trying to answer below (or rather ask questions)…
On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote:
> Hi devs, > > we have an important inconsistency that we ignored since a long time > and that is producing http://jira.xwiki.org/browse/XWIKI-7628. > > The main issue is that the storage save document containing object > linked explicitly to another wiki without complaining which mean that > user continue using API wrongly without really knowing it's wrong. And > now we have lots of code badly using object class related API and > contently creating object with absolute class references.
I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part.
Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
> Until now it was OK even if a mess and it's still not visible. But > that's mostly luck... until we start copying documents from one wiki > to another starting with 4.0 branch. What changed is that the uniquetemporary > identifier of the object in the database is "properly" generated by > using it's reference and since a class reference in a BaseObject > contain a wiki it's now taken into account. The old id generator used > to use the deprecated BaseCollection#getClassName which is forced > absolute for retro-compatibility reason even if it was actually > possible before to put absolutely anything as class reference. > > So right now we have two issues: > * store save invalid document without complaining > * many code is wrongly using BaseObject and XWikiDocument API > regarding class references mostly because you don't really see it > right away when you don't use it properly. Even ObjectAddAction is > wrong, a lot of code that was using setClassName has been badly > converted to fix deprecated method code by resolving what was passed > to setClassName into an absolute reference which is very wrong and not > the behavior of setClassName. > > Here is what we propose with Denis: > * 4.x (or until class coming from another wiki is supported but I > doubt this is going to happen anytime soon with this storage): > ** throw an exception in the storage when saving a document containing > objects coming from another wiki
I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
Thanks -Vincent
> ** to temporary limit breakage while we fix wrong code and help us > find this wrong code, modify BaseCollection#setXClassReference to > force it to remove the wiki from the passed reference and log a > warning about a bad usage > * 5.0: remove the bandage from BaseCollection#setXClassReference > > WDYT ? > > Here is my +1
Hi Denis, ok I've discussed with Thomas and here's my current position: * +1 to add a warning log in BaseObject to find out all the places in code that use an absolute ref instead of a relative ref * +1 to remove this warning in 4.0 final * +1 to throw an exception in the store implementation if it finds an absolute ref * However I'd like that we keep removing the wiki part in setXClassReference() *forever* i.e. decide that we don't want to support this *ever* in the current old Model. We could add this feature but *only* in the new Model which will have completely new code and will not use the old APIs anyway so this won't cause any API breakage for anyone. Denis, does that some acceptable to you (I know Thomas is ok with this since it's what he'd like to do too)? Thanks -Vincent On Mar 29, 2012, at 12:51 PM, Vincent Massol wrote:
On Mar 29, 2012, at 12:07 PM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 11:00, Vincent Massol <[email protected]> wrote:
On Mar 28, 2012, at 10:49 AM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
It is! This is even the reason of the vote: because some places of the code use wrongly an absolute ref instead of a relative ref...
Not the code of the API, but recent code using the API like the sheet binding mechanism. There was a bug in the store, but this is really not the story.
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject.
I don't recall this. Can you give me the link?
Two points about this: 1) This was not a vote 2) I wrote this proposal and I remember it very well and I've never proposed to allow having an XClass in one wiki and an XObject in another wiki.
Also if it his is true I'm suggesting to vote again on this.
Going back after a vote, is really not good IMO, unless we are really sure, so not in hurry.
It's never been voted nor even proposed so no issue at all… :)
This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ?
Nothing should be reverted IMO, it's ok to pass a full reference even if only the relative part will be saved in the wiki. In the future, **if** we ever want to add support for absolute xclass ref in the DB then we'll have another api and we will then be able to modify the places that want to support absolute refs to use the new api.
You are wrong, we take care of passing relative reference in many places, and later we need to go back to an absolute one, since relative reference are not workable with. So leaving all this code if we are sure it is useless is not appropriate. But what I means mainly, is that we have written these line of codes (and test) just because of the vote above, and now you just propose to throw it away.
I 've never written any line of code because I wanted to support having an XClass in one wiki and an XObject in another one. That's not true.
It would have been stupid to propose something that cannot be implemented. I would not have done this. My strategy is to not propose something for the future because of YAGNI unless in some special case where I'm 100% sure it'll be needed and not implementing it would cause issues in the future.
Now it's possible that this proposal had a side effect that lead to allowing this but it doesn't mean we wanted to support it (and we were not supporting it since we cannot support it currently).
On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
It's simple IMO. Most of our APIs return DocumentReference, ie absolute refs. So for example imagine I want to use a XClass defined in a given doc, I'll use: doc.getDocumentReference() as the XClass reference.
Yes, but maybe we just need a doc.getDocumentReference().toRelative(EntityType.WIKI) for the purpose.
We can already do this so that's not the issue IMO.
My preference still goes to 2) ATM but if the majority prefers 3) I'll vote +0 for 3) :)
Thanks -Vincent
We don't have a ClassReference object and so far we use DocumentReference == ClassReference.
Thanks -Vincent
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote: > Hi, > > This looks like a very complex topic. Trying to answer below (or rather ask questions)… > > On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote: > >> Hi devs, >> >> we have an important inconsistency that we ignored since a long time >> and that is producing http://jira.xwiki.org/browse/XWIKI-7628. >> >> The main issue is that the storage save document containing object >> linked explicitly to another wiki without complaining which mean that >> user continue using API wrongly without really knowing it's wrong. And >> now we have lots of code badly using object class related API and >> contently creating object with absolute class references. > > I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part. > > Are we talking about the same thing?
It's not that simple, xclass reference in xobject are working like parents reference in document: they are relative to keep what the user explicitly asked even when the document is copied in another context. So which reference is set to the object is very important because the reference will be copied as it is when copying a document.
Here is the javadoc you wrote in BaseCollection:
/** * The meaning of this reference fields depends on the element represented. Examples: * <ul> * <li>If this BaseCollection instance represents an XObject then refers to the document where the XObject's XClass * is defined.</li> * <li>If this BaseCollection instance represents an XClass then it's not used.</li> * </ul> * Note that we're saving the XClass reference as a relative reference instead of an absolute one. This is because * We want the ability (for example) to create an XObject relative to the current space or wiki so that a copy of * that XObject would retain that relativity. This is for example useful when copying a Wiki into another Wiki so * that the copied XObject have a XClassReference pointing in the new wiki. */ private EntityReference xClassReference;
It's not only a storage issue, if you have an document object with reference wiki1:Space:MyClass and copy that document to wiki2 then you will have an object in wiki2 with class wiki1:Space:MyClass. The storage does not have a word to said in this process it just get the resulting new XWikiDocument to save and in this case we are asking the store to store an object defined by a class which is in another wiki, something that it does not support and more importantly something that was not intended.
So we have several choices here: 1) continue hiding under the carpet that we are asking to the store something that it can't do and have inconsistency between object and storage and crappy hack here and there in the storage (in the case of XWIKI-7628 it means not trusting what's returned by BaseObject#getReference) 2) totally forget about relative xclass reference chosen outside of BaseObject and always remove the wiki from the reference stored in BaseCollection#xClassReference which mean that this API will officially never support having an object with a class from another wiki 3) keep the current API as it has been defined and voted and try to make a bit more sure people are using it the way it's supposed to be used
Here we are talking proposing 3)
Got it!
>> Until now it was OK even if a mess and it's still not visible. But >> that's mostly luck... until we start copying documents from one wiki >> to another starting with 4.0 branch. What changed is that the uniquetemporary >> identifier of the object in the database is "properly" generated by >> using it's reference and since a class reference in a BaseObject >> contain a wiki it's now taken into account. The old id generator used >> to use the deprecated BaseCollection#getClassName which is forced >> absolute for retro-compatibility reason even if it was actually >> possible before to put absolutely anything as class reference. >> >> So right now we have two issues: >> * store save invalid document without complaining >> * many code is wrongly using BaseObject and XWikiDocument API >> regarding class references mostly because you don't really see it >> right away when you don't use it properly. Even ObjectAddAction is >> wrong, a lot of code that was using setClassName has been badly >> converted to fix deprecated method code by resolving what was passed >> to setClassName into an absolute reference which is very wrong and not >> the behavior of setClassName. >> >> Here is what we propose with Denis: >> * 4.x (or until class coming from another wiki is supported but I >> doubt this is going to happen anytime soon with this storage): >> ** throw an exception in the storage when saving a document containing >> objects coming from another wiki > > I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there.
This has nothing to do with what we propose, the goal here is to find code which is setting absolute reference for the class to that it's fixed to not have issue when copying. If we do what you say then the document copy will fail like it does now but simply for another reason and it's not going to help fixing anything because the issue is not at this level but in the code that put an absolute reference in the original document.
> > Thanks > -Vincent > >> ** to temporary limit breakage while we fix wrong code and help us >> find this wrong code, modify BaseCollection#setXClassReference to >> force it to remove the wiki from the passed reference and log a >> warning about a bad usage >> * 5.0: remove the bandage from BaseCollection#setXClassReference >> >> WDYT ? >> >> Here is my +1
On Thu, Mar 29, 2012 at 2:44 PM, Vincent Massol <[email protected]> wrote:
Hi Denis,
ok I've discussed with Thomas and here's my current position:
* +1 to add a warning log in BaseObject to find out all the places in code that use an absolute ref instead of a relative ref * +1 to remove this warning in 4.0 final * +1 to throw an exception in the store implementation if it finds an absolute ref * However I'd like that we keep removing the wiki part in setXClassReference() *forever* i.e. decide that we don't want to support this *ever* in the current old Model. We could add this feature but *only* in the new Model which will have completely new code and will not use the old APIs anyway so this won't cause any API breakage for anyone.
I insist again on the fact that *forever* allows to optimize all class reference related code accordingly and stop contently resolve those references as absolute references. It also mean that BaseCollection#getRelativeXClassReference() will be made public (and probably deprecated getXClassReference())
Denis, does that some acceptable to you (I know Thomas is ok with this since it's what he'd like to do too)?
Thanks -Vincent
On Mar 29, 2012, at 12:51 PM, Vincent Massol wrote:
On Mar 29, 2012, at 12:07 PM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 11:00, Vincent Massol <[email protected]> wrote:
On Mar 28, 2012, at 10:49 AM, Denis Gervalle wrote:
On Wed, Mar 28, 2012 at 09:42, Vincent Massol <[email protected]> wrote:
Thanks to Thomas and Denis for the explanations. I understand now, it's very clear :)
However I'd like to suggest a different solution.
Premises: * 99% of the use cases are for relative references
Right, since nothing else would work today.
* We need to make sure devs don't use the API wrongly and thus the API has to be easy to use right
This is true for most API, maybe we could add more helpers, but this is not the subject of this vote.
It is! This is even the reason of the vote: because some places of the code use wrongly an absolute ref instead of a relative ref...
Not the code of the API, but recent code using the API like the sheet binding mechanism. There was a bug in the store, but this is really not the story.
* We have never supported absolute references to XClasses (since we've never supported this in the DB)
The database have never support it, but the whole API have been written to support it. This has been the result of a previous vote on this subject.
I don't recall this. Can you give me the link?
Two points about this: 1) This was not a vote 2) I wrote this proposal and I remember it very well and I've never proposed to allow having an XClass in one wiki and an XObject in another wiki.
Also if it his is true I'm suggesting to vote again on this.
Going back after a vote, is really not good IMO, unless we are really sure, so not in hurry.
It's never been voted nor even proposed so no issue at all… :)
This is why we have not proposed to change the behavior of the API, but a temporary solution to help those that have use it badly and was never able to notice it due to some bug at store level, that this usage is currently wrong, and fix it on the fly for now (quick fix), since there is no other valid usage for our current storage.
Based on these premises I'd like to suggest that: * We automatically remove the wiki part when adding an XObject (specifically in resolveClassReference())
We effectively propose to do so with a stack trace warning if the wiki is ok, and with a stack trace error if you try to really link to an external class. This would not break abruptly existing wrong usage, and allow us to extends later.
* Later on when we implement a new storage that supports multiwiki for xclasses then we add another method so that users who want to use an absolute reference do it voluntarily and not by mistake. It could be: newAbsoluteXObject(…) for example. Alternatively we could add: newXObject(EntityReference ref, boolean isAbsolute).
This is not that simple. We take care of relativizing the reference in most places already, before calling object creation. But, for sure this would simplify a lot. This means removing what we have done up to now. However, we support external classes in all places from the action, and only the storage does not really allow them. Should we revert that ?
Nothing should be reverted IMO, it's ok to pass a full reference even if only the relative part will be saved in the wiki. In the future, **if** we ever want to add support for absolute xclass ref in the DB then we'll have another api and we will then be able to modify the places that want to support absolute refs to use the new api.
You are wrong, we take care of passing relative reference in many places, and later we need to go back to an absolute one, since relative reference are not workable with. So leaving all this code if we are sure it is useless is not appropriate. But what I means mainly, is that we have written these line of codes (and test) just because of the vote above, and now you just propose to throw it away.
I 've never written any line of code because I wanted to support having an XClass in one wiki and an XObject in another one. That's not true.
It would have been stupid to propose something that cannot be implemented. I would not have done this. My strategy is to not propose something for the future because of YAGNI unless in some special case where I'm 100% sure it'll be needed and not implementing it would cause issues in the future.
Now it's possible that this proposal had a side effect that lead to allowing this but it doesn't mean we wanted to support it (and we were not supporting it since we cannot support it currently).
On the other hand, this seems to me curious to pass over an absolute reference to just ignore a part of it. Maybe we are simply missing something in references to helps these usages ?
It's simple IMO. Most of our APIs return DocumentReference, ie absolute refs. So for example imagine I want to use a XClass defined in a given doc, I'll use: doc.getDocumentReference() as the XClass reference.
Yes, but maybe we just need a doc.getDocumentReference().toRelative(EntityType.WIKI) for the purpose.
We can already do this so that's not the issue IMO.
My preference still goes to 2) ATM but if the majority prefers 3) I'll vote +0 for 3) :)
Thanks -Vincent
We don't have a ClassReference object and so far we use DocumentReference == ClassReference.
Thanks -Vincent
This will ensure that users of the api can't use it wrongly.
I really don't see any point in continuing to try to support a feature that doesn't exist and that we can't support… It's better to add support for it when we implement a storage that supports it.
Thanks -Vincent
On Mar 26, 2012, at 8:07 PM, Thomas Mortagne wrote:
> On Mon, Mar 26, 2012 at 6:21 PM, Vincent Massol <[email protected]> wrote: >> Hi, >> >> This looks like a very complex topic. Trying to answer below (or rather ask questions)… >> >> On Mar 26, 2012, at 3:43 PM, Thomas Mortagne wrote: >> >>> Hi devs, >>> >>> we have an important inconsistency that we ignored since a long time >>> and that is producing http://jira.xwiki.org/browse/XWIKI-7628. >>> >>> The main issue is that the storage save document containing object >>> linked explicitly to another wiki without complaining which mean that >>> user continue using API wrongly without really knowing it's wrong. And >>> now we have lots of code badly using object class related API and >>> contently creating object with absolute class references. >> >> I don't see anything wrong with adding an XObject to a Document using an absolute XClass reference. This is actually the API we've added and we've only added relative XObject API more recently which IMO is the wrong one since we always said we wanted to use absolute references everywhere in all APIs and it's up to the storage to remove/add the wiki part. >> >> Are we talking about the same thing? > > It's not that simple, xclass reference in xobject are working like > parents reference in document: they are relative to keep what the user > explicitly asked even when the document is copied in another context. > So which reference is set to the object is very important because the > reference will be copied as it is when copying a document. > > Here is the javadoc you wrote in BaseCollection: > > /** > * The meaning of this reference fields depends on the element > represented. Examples: > * <ul> > * <li>If this BaseCollection instance represents an XObject then > refers to the document where the XObject's XClass > * is defined.</li> > * <li>If this BaseCollection instance represents an XClass then > it's not used.</li> > * </ul> > * Note that we're saving the XClass reference as a relative > reference instead of an absolute one. This is because > * We want the ability (for example) to create an XObject relative > to the current space or wiki so that a copy of > * that XObject would retain that relativity. This is for example > useful when copying a Wiki into another Wiki so > * that the copied XObject have a XClassReference pointing in the new wiki. > */ > private EntityReference xClassReference; > > It's not only a storage issue, if you have an document object with > reference wiki1:Space:MyClass and copy that document to wiki2 then you > will have an object in wiki2 with class wiki1:Space:MyClass. The > storage does not have a word to said in this process it just get the > resulting new XWikiDocument to save and in this case we are asking the > store to store an object defined by a class which is in another wiki, > something that it does not support and more importantly something that > was not intended. > > So we have several choices here: > 1) continue hiding under the carpet that we are asking to the store > something that it can't do and have inconsistency between object and > storage and crappy hack here and there in the storage (in the case of > XWIKI-7628 it means not trusting what's returned by > BaseObject#getReference) > 2) totally forget about relative xclass reference chosen outside of > BaseObject and always remove the wiki from the reference stored in > BaseCollection#xClassReference which mean that this API will > officially never support having an object with a class from another > wiki > 3) keep the current API as it has been defined and voted and try to > make a bit more sure people are using it the way it's supposed to be > used > > Here we are talking proposing 3)
Got it!
>>> Until now it was OK even if a mess and it's still not visible. But >>> that's mostly luck... until we start copying documents from one wiki >>> to another starting with 4.0 branch. What changed is that the uniquetemporary >>> identifier of the object in the database is "properly" generated by >>> using it's reference and since a class reference in a BaseObject >>> contain a wiki it's now taken into account. The old id generator used >>> to use the deprecated BaseCollection#getClassName which is forced >>> absolute for retro-compatibility reason even if it was actually >>> possible before to put absolutely anything as class reference. >>> >>> So right now we have two issues: >>> * store save invalid document without complaining >>> * many code is wrongly using BaseObject and XWikiDocument API >>> regarding class references mostly because you don't really see it >>> right away when you don't use it properly. Even ObjectAddAction is >>> wrong, a lot of code that was using setClassName has been badly >>> converted to fix deprecated method code by resolving what was passed >>> to setClassName into an absolute reference which is very wrong and not >>> the behavior of setClassName. >>> >>> Here is what we propose with Denis: >>> * 4.x (or until class coming from another wiki is supported but I >>> doubt this is going to happen anytime soon with this storage): >>> ** throw an exception in the storage when saving a document containing >>> objects coming from another wiki >> >> I don't like this. If the problem is that we don't want to allow an XObject to be added when the XClass is coming from another wiki then this should be checked at the level of the AddXObject APIs and throw an exception there. > > This has nothing to do with what we propose, the goal here is to find > code which is setting absolute reference for the class to that it's > fixed to not have issue when copying. If we do what you say then the > document copy will fail like it does now but simply for another reason > and it's not going to help fixing anything because the issue is not at > this level but in the code that put an absolute reference in the > original document. > >> >> Thanks >> -Vincent >> >>> ** to temporary limit breakage while we fix wrong code and help us >>> find this wrong code, modify BaseCollection#setXClassReference to >>> force it to remove the wiki from the passed reference and log a >>> warning about a bad usage >>> * 5.0: remove the bandage from BaseCollection#setXClassReference >>> >>> WDYT ? >>> >>> Here is my +1
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
+0 Thanks, Marius On Mon, Mar 26, 2012 at 4:43 PM, Thomas Mortagne <[email protected]> wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki ** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
Basically the code for this is written and I am waiting some more vote to commit it , I would like to have this in M2, since it will show where we use the API badly. On Tue, Mar 27, 2012 at 09:32, Marius Dumitru Florea < [email protected]> wrote:
+0
Thanks, Marius
On Mon, Mar 26, 2012 at 4:43 PM, Thomas Mortagne <[email protected]> wrote:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki ** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
+1 Best Regards, /Andreas 2012-03-26 15:43, Thomas Mortagne skrev:
Hi devs,
we have an important inconsistency that we ignored since a long time and that is producing http://jira.xwiki.org/browse/XWIKI-7628.
The main issue is that the storage save document containing object linked explicitly to another wiki without complaining which mean that user continue using API wrongly without really knowing it's wrong. And now we have lots of code badly using object class related API and contently creating object with absolute class references.
Until now it was OK even if a mess and it's still not visible. But that's mostly luck... until we start copying documents from one wiki to another starting with 4.0 branch. What changed is that the unique identifier of the object in the database is "properly" generated by using it's reference and since a class reference in a BaseObject contain a wiki it's now taken into account. The old id generator used to use the deprecated BaseCollection#getClassName which is forced absolute for retro-compatibility reason even if it was actually possible before to put absolutely anything as class reference.
So right now we have two issues: * store save invalid document without complaining * many code is wrongly using BaseObject and XWikiDocument API regarding class references mostly because you don't really see it right away when you don't use it properly. Even ObjectAddAction is wrong, a lot of code that was using setClassName has been badly converted to fix deprecated method code by resolving what was passed to setClassName into an absolute reference which is very wrong and not the behavior of setClassName.
Here is what we propose with Denis: * 4.x (or until class coming from another wiki is supported but I doubt this is going to happen anytime soon with this storage): ** throw an exception in the storage when saving a document containing objects coming from another wiki ** to temporary limit breakage while we fix wrong code and help us find this wrong code, modify BaseCollection#setXClassReference to force it to remove the wiki from the passed reference and log a warning about a bad usage * 5.0: remove the bandage from BaseCollection#setXClassReference
WDYT ?
Here is my +1
participants (5)
-
Andreas Jonsson -
Denis Gervalle -
Marius Dumitru Florea -
Thomas Mortagne -
Vincent Massol