On Wed, Nov 16, 2011 at 6:51 AM, Caleb James DeLisle <[email protected]> wrote: > > > On 11/15/2011 03:26 PM, Vincent Massol wrote: >> >> On Nov 15, 2011, at 9:04 PM, Caleb James DeLisle wrote: >> >> >>> On 11/15/2011 12:25 PM, Vincent Massol wrote: >>>> >>>> On Nov 15, 2011, at 5:29 PM, Denis Gervalle wrote: >>>> >>>>> On Tue, Nov 15, 2011 at 17:11, Vincent Massol <[email protected]> wrote: >>>>> >>>>>> >>>>>> On Nov 15, 2011, at 5:03 PM, Denis Gervalle wrote: >>>>>> >>>>>>> Hi Devs, >>>>>>> >>>>>>> The implementation of the immutable version of reference is almost ready >>>>>>> now. It introduce the parameters on reference as suggested, but we now >>>>>> have >>>>>>> a discussion on how the constructors of "typed" entity reference should >>>>>> be. >>>>>>> >>>>>>> My initial dev was to provide constructor like: >>>>>> >>>>>> you're missing something here :) >>>>>> >>>>>>> But Vincent have different vision of this, here its comment extracted >>>>>> from >>>>>> >>>>>> I don't have a different vision. It's just that you limited your proposal >>>>>> to just Locale which clearly isn't good enough. >>>>>> >>>>>>> GitHub ( >>>>>>> >>>>>> https://github.com/xwiki/xwiki-platform/commit/cea424914f40ce924afbc49b3159bc8934251aac#commitcomment-721603 >>>>>> ) >>>>>>> : >>>>>>> >>>>>>> My proposal was: >>>>>>>> - generic params in EntityReference >>>>>>>> - helpers methods for get/setLocale and get/setVersion in >>>>>> DocumentReference >>>>>>>> >>>>>>>> Now the fact that we're making the refs immutable changed this since >>>>>> it's >>>>>>>> no longer possible. >>>>>>>> >>>>>>>> I don't think multiplying the constructor signatures is a good idea. >>>>>>>> >>>>>>>> We could either have: >>>>>>>> >>>>>>>> public DocumentReference(String wikiName, List spaceNames, String >>>>>>>> pageName, Map<String, Object> parameters) >>>>>>>> >>>>>>>> or >>>>>>>> >>>>>>>> public DocumentReference(String wikiName, List spaceNames, String >>>>>>>> pageName, Pair<String, Object>... parameters) >>> >>> Neither of these signatures will work, If you want storage to have any hope >>> of being able to get the same object for the same reference, it must be at >>> least >>> a Map or Pair of <String, Serializable>, if you allow people to pass >>> transient objects such as TCP sockets, they will. >>> >>> More importantly, if you introduce another parameter, there will be no way >>> to search for all documents where param X = value Y. To do it with SQL >>> would require >>> an evil query which did text searching on the parameter field something >>> like: WHERE doc.reference LIKE '%param=value%' >>> To add parameters to a reference and retain the ability to get all >>> documents where "param=value" you would have to modify the database schema >>> unless the information >>> already exists in the XWikiDocument mapping as with language. >>> >>>>>>>> >>>>>>>> where Pair is >>>>>>>> >>>>>> http://commons.apache.org/lang/api-3.0-beta/org/apache/commons/lang3/Pair.html >>>>>>>> >>>>>>>> Maybe this needs some discussion on the devs list rather than here to >>>>>> make >>>>>>>> sure everyone sees it? >>>>>>>> >>>>>>> >>>>>>> I am myself not really happy with that since I dislike the idea that >>>>>>> parameter are generic on "typed" references. >>>>>>> Do not like either the idea to provide keys for creating a Map or Pairs, >>>>>>> since the implementation details that use Map should not be so exposed >>>>>> IMO. >>>>>>> >>>>>>> There should not be so much parameter on a single "typed" reference, >>>>>> >>>>>> I don't understand this sentence. >>>>>> >>>>>> The Map is <String, Object>. >>>>>> >>>>> >>>>> I would means that this should not cause so many additional constructors, >>>>> if we list them individually. >>>>> So your have not the same vision then I have :) >>>> >>>> See below. >>>> >>>>>>> and >>>>>>> these should be easy to provide. Creating Maps in java is not fun in >>>>>> syntax >>>>>>> for that IMO, and is far too open. >>>>>> >>>>>> Sure but the goal here is not to redo java… >>>>>> >>>>> >>>>> Not my goals, just want to be explicit and easy to use. >>>>> >>>>> >>>>>> >>>>>>> I had propose using overloaded constructor like >>>>>>> >>>>>>> public DocumentReference(String wikiName, List<String> spaceNames, >>>>>>> String >>>>>>> pageName, Locale locale) >>> >>> +1 >>> >>>>>> >>>>>> Again, this doesn't work. It only works for a **single** parameter. It >>>>>> doesn't work for multiple parameters. How do you specify the Locale or >>>>>> some >>>>>> unknown String param? >>>>>> >>>>> >>>>> I simply provide the need constructor, no more. >>>> >>>> If you say this then you say that we don't need generic parameters for >>>> References which is what started the discussion… >>>> >>>> Also what you say is not correct at all since we already know we need >>>> Version which you didn't put. >>>> >>>> Last, it means passing null to the constuctors when you don't use the >>>> parameters which is really really bad IMO or you'll need to define lots of >>>> various constructors which increase exponentially with the number of >>>> parameters we support. >>> >>> A constant need for new parameters indicated design problems, including >>> locale information is fixing an error which limited the document reference >>> from the beginning. >>> Building this infrastructure for additional parameters will foster bad >>> practice of using parameters where spaces should be used instead and will >>> allow the creation >>> of arbitrary dimensional data trees where users are only familiar with the >>> standard 1 dimension of filesystems and URLs. >>> >>>> >>>> However this won't even work since you won't be able to support adding new >>>> parameters. Users of the Reference system will need to modify its source >>>> if they wish to add a new parameter which again goes against the initial >>>> proposal. >>>> >>>>>>> or if something more flexible should be used >>>>>>> >>>>>>> public DocumentReference(String wikiName, List<String> spaceNames, >>>>>>> String >>>>>>> pageName, Object... parameters) >>>>>>> >>>>>>> where parameters is later interpreted based on object type and limited >>>>>>> to >>>>>>> those used for a given typed reference. >>>>>> >>>>>> This doesn't work. If I have 2 parameters of type String, how do you map >>>>>> them automatically? >>>>>> >>>>> >>>>> I suggest not to have loosely typed parameters, but strong one, like >>>>> Locale >>>>> and Version. >>> >>> +1 >>> >>> I am -1 on version, I don't like it since a document history should be a >>> separate entity which yields documents, as it is now. >> >> How do you reference a given version of, say, a document? And how do you >> pass this along the chain of calls? >> >> We'll need to introduce a new Reference (which is the initial reason I made >> this proposal - I had to introduce a UniqueReference class in the new Model >> before of this). > > If you are needing to reference old versions of something on a regular basis > then you are going to run into performance issues. > We only access old versions either via a look at them or to rollback, both > operations which we accept will be slow. > >> >>> If we include version in the reference, storage will be forced to remove it >>> on save because we want users asking for a document >>> with no knowledge of the current version to get the current version. This >>> means we already have a magic attribute which is treated >>> differently by storage. >> >> All params are treated specially by the code, I don't understand what's the >> problem. The exact same thing will happen for the Locale. > > On a storage level, you can take the reference, serialize it into a UUID then > lookup the document with that UUID. > If a version parameter is added, the storage has to remove that parameter > first. > >> >>> In addition, the introduction of a version parameter will allow a user to >>> load a document of a given version >>> with a simple getDocument() call, >> >> Yes that's the idea! :) >> >>> fooling them into thinking all versions of every document are available in >>> the same namespace, >> >> Not sure what namespace you're referring to. > > Namespace is the wrong word, you tell the user that it's coming from the same > place but it's not. > > Consider this: > thirdVersion = xwiki.getDocument(referenceToThirdVersionOfMainWebHome); > thirdVersion.setContent("new content"); > thirdVersion.save(); > println(xwiki.getDocument(referenceToThirdVersionOfMainWebHome).getContent()); > > This will not yield the new content, if it did then the versioning system > wouldn't really be versioning. > But by treating a version as a distinct document with a distinct reference, > you are telling the user that it should. > >> >>> if getDocument(referenceToThirdVersionOfMainWebHome) works then it stands >>> to reason that search("WHERE doc.version=3 AND doc.name='Main.WebHome'") >>> would as well but it won't because that's not how storage works. >> >> Maybe you meant that it's not how storage currently work? >> >> Remember that this proposal has nothing to do with either the current model >> or the current storage. It's about the new model and the new storage that we >> want to have. > > I don't understand what you mean by this. > Inserting a new row into the document table every time a user edits a > document is not feasible. > >> >>>> I'm a big big big -1 on this. >>>> >>>> There's no reason to create a fake String when all you need is a string. >>>> Same for int, long, Number, etc. >>>> Same for also if you want to have several Locales or several Versions. >>>> >>>>>> Also the goal is to have unknown parameters so how can you do a mapping >>>>>> for something unknown? :) >>>>>> >>>>> >>>>> That is clearly not my goal, why do you want unknown parameters on >>>>> Document >>>>> Reference ? >>>> >>>> Maybe this is where we don't agree. This is the original idea: to have a >>>> set of "unknown" parameters for extensibility (from the POV of the >>>> Reference of course, from the POV of XWiki they are not unknown obviously >>>> ;)). This means that it's up to the users of the References to decide what >>>> parameters to put and it's up to the consumers to decide what parameters >>>> to support. >>> >>> The introduction of k-d trees for storing documents is not something that >>> anyone has ever used to my knowledge, it may be something that people think >>> they need >>> but like the goto command, something that starts out nice ends up creating >>> more problems than it's worth. >>> >>>> >>>> It seems to me your vision is to: >>>> * Not support arbitrary parameters >>>> * Only support Version and Locale >>>> * Provide 3 constructors, one where there's no Locale no Version, one with >>>> Locale no version and one with Version on Locale >>>> >>>> Whereas my vision is: >>>> * Support arbitrary parameters for extensibility (like a URL if you prefer >>>> which support arbitrary parameters) >>> >>> This is not correct, the parameters in a URL are never to my knowledge used >>> as a discriminator. >>> Any webserver I can think of uses the parameters as arguments which are >>> passed to the page, not discriminators to select the page. >> >> Fun, you forgot XWiki itself! :) >> We use version and language as URL parameters…. ?rev=X&language=Y >> >> Params reference are optional and are meant to be qualifier for the >> reference. > > IMO this is a bad design since it means that certain parameters such as > language have special meanings, all others are used as a means of > communicating with the scripting in the wiki page. > >> >>> for example: >>> path/to/some/page.php?var=val&varb=42 >>> will load the page "path/to/some/page.php" then execute it, passing var=val >>> and varb=42 as arguments. >> >> Seen all this I propose that stop this discussion and don't commit any of >> this on master for the moment since we have too many diverging opinions on >> the new model and we need to agree about the new model first. >> >> So I change my vote about the proposal I initially sent to -1 for now till >> this is sorted out because I don't want to rush into something for which we >> don't agree and have diverging ideas. >> >> I've been planning to put the work I started in the contrib sandbox into a >> branch on master in order to start using it ASAP (Thomas and I had a >> discussion and we think we could already start to slow use portions of it to >> validate it). >> >> So here's what I propose: >> >> * I do this ASAP >> * I write an email explaining the work done so far on the new model >> * We discuss it and come to an agreement >> * Whether we extend our current Entity References with params or not will be >> a direct consequence of the model agreement >> >> I know Denis wanted to use the Locale param for his work. FTM Denis will >> have to pass the Locale as a parameter alongside the Entity Reference as >> it's done every where ATM. >> >> Is that ok? > > I disagree with slamming on the brakes on Denis's branch, the Denis wrote a > lot of code with the understanding that everyone agreed on it so I want to do > that justice. > I have reviewed Denis's branch and it all looks good to me, it makes 2 major > changes: > 1. Immutability which I have wanted since early on, I think this will make > the code a lot more stable. > 2. Introduction of a locale field which is necessary for the reference to be > able to map to a document id, this means we need it now in order to make new > references compatible with the existing model. > > The other proposals such as version and arbitrary parameters broaden the new > model over the existing one and they should not be undertaken without > research into how the new model will interact with users and new storage, as > well as how it will interact with old storage and the old model during the > transition. > > In my review of the branch, I found a few things which could be changed: > In EntityReference.java: > > private static final long serialVersionUID = 1L; > that should become 2L because removing child is a breaking change from a > serialization standpoint. > > public EntityReference(String name, EntityType type, EntityReference parent, > Map<String,Object> parameters) > I think this should be protected at least for now. > > public final Object getParameter(String name) > I think this should be protected at least for now. > > > +1 for merging the branch with these changes.
+1 too, I don't agree that locale without version is useless and if parameters are made internal for now everyone should be happy until we agree on something about them. > > Caleb > > >> >> Thanks >> -Vincent >> >>> Caleb >>> >>> >>>> * Have some "well-known" parameters so that xwiki code that users >>>> References know what they can put as reference parameters and what they >>>> can extract from XWiki parameters. >>>> >>>> Thanks >>>> -Vincent >>>> >>>>>> Thanks >>>>>> -Vincent >>>>>> >>>>>>> Here Vincent comments on this: >>>>>>> >>>>>>>> The automatic mapping idea seems a bad idea to me (too magic and >>>>>>>> doesn't >>>>>>>> work in a lot of cases). >>>>>>> >>>>>>> >>>>>>> Maybe some of you have an even better idea ? >>>>>>> >>>>>>> Denis >> _______________________________________________ >> 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
