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? >> > > http://markmail.org/message/yggcayev5hvgv342
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
