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 _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
