Re: [xwiki-devs] [VOTE] Temporary make sure a BaseObject cannot target a class coming from another wiki

On Mar 29, 2012, at 12:07 PM, Denis Gervalle wrote: 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. It's never been voted nor even proposed so no issue at all… :) 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). 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 &lt;vincent(a)massol.net&gt; >>>> 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