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