On Nov 16, 2011, at 11:34 AM, Caleb James DeLisle wrote: > > > On 11/16/2011 03:14 AM, Vincent Massol wrote: >> >> On Nov 16, 2011, at 6:51 AM, Caleb James DeLisle 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. >> >> I don't understand what this changes. If you're able to "look at them" then >> it means you have the logic to … look at them. The fact that you can have a >> reference to that version doesn't change anything. >> >> Also I don't agree about performance. That only depends on how versions are >> stored in the storage. > > My apologies again for being vague, we generally only access old versions of > through the viewrev action and the rollback action. > This is for a good reason, accessing old versions of documents is hard > because we don't store entire versions of old documents, we store differences > between versions. > All versioned data stores function this way for space efficiency but it > incurs a performance penalty on loading old versions. > IMO it's important to help the user understand this performance penalty by > making them use a special call to get old versions. > >> >> >> >>> >>>> >>>>> 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. >> >> No that's not true. Right now for example we remove the wiki part of the >> reference since we have one DB per wiki. > > Indeed, how to map wikis to databases in the future is another topic for a > different discussion, FWIW I don't think the current system is optimal. > As a side note, the wiki reference is a very central part of the > EntityReference, far from an arbitrary parameter which happens to be under > the key "VERSION". > >> >> Saying that a reference is stored as is in a "Document row" (assuming RDBMS >> for the sake of the discussion) is not correct IMO. That's not the role of a >> reference. A reference is computed based on various parts (wiki it's in, >> space it's in, locale, revision, etc). >> >>> If a version parameter is added, the storage has to remove that parameter >>> first. >> >> The reference is never stored in the storage as is! When an Entity is loaded >> from the storage the reference is computed based on various things. For >> example as I explained above, based on the DB name for the wiki part. > > I think we should be moving toward a new storage system which does simply > store the entire reference for performance and code cleanliness in the store. > >> >>>>> 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. >> >> errr? This is a completely different problem. It's about immutability or not >> of past revisions. >> >> Consider what we do now: >> >> currentVersion = xwiki.getDocument(refereneToCurrentVersionOfMainWebHome) >> thirdVersion = currentVersion.getVersion(thirdversion) >> thirdVersion.setContent("new content") >> thirdVersion.save() >> >> currentVersion = xwiki.getDocument(refereneToCurrentVersionOfMainWebHome) >> thirdVersion = currentVersion.getVersion(thirdversion) >> println(thirdVersoin.getContent()); >> >> How is that different? >> >> What do you propose? > > I'm not making a proposal right now but I do believe in weighing different > options. > User interface is everything and this is user interface, even if it is > interface for advanced users. > > What we currently do gives the user a small heads up that something is > different because the old version "comes from" the new version but I agree it > is inadequate. > Off the top of my head I can imagine something like: > > currentVersion = xwiki.getDocument(refereneToCurrentVersionOfMainWebHome) > currentVersion.asVersion(3.0); > println(currentVersion.getContent()); > currentVersion.save(); > > With asVersion() not returning a distinct document but switching the document > underneath the wrapper which the user sees, the user never thinks he is > modifying an old version. > If we are going to make old versions mutable after the fact, this will > require a separate proposal as it raises another set of issues. > > >> >>>>> 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. >> >> I'm also ok to just add support for Locale right now but I really hope we'll >> agree in the future because there's no reason to consider locale differently >> than the version for example IMO. >> >> Also I'd like that Denis remove the Map of parameters in EntityReference >> since this is not going to be used and we need an agreement first about them >> which we don't have ATM. >> >> This means moving the getLocale() to EntityReference and having a >> specialized constructor for taking a Locale in EntityReference. > > IMO locale is an attribute of a DocumentReference, not an EntityReference. It > makes no sense to have a Spanish object in an English document, > objects/attachments could indicate their locality by which document they are > attached to. > My opinion aside, I think we should stick to what is needed to make the new > reference model able to represent the old data model and right now, we only > translate documents. > I don't have a strong position on how the internals work, and I will defer to > Denis's judgement on that matter.
Yes right I agree. I was already thinking about the new model. Let's put it only in DocumentReference for now then and not put anything in EntityReference. Thanks -Vincent >> Of course we all realize that this means that if we agree in the future >> about adding the Version or other params we're going to break APIs once more >> because we'll need to add new constructors, possibly deprecate old ones and >> add new methods, but I don't see any other way if Denis really wants to have >> this now. > > These APIs are still young, to really get this right we'll will need a few > feedback cycles before we really know what we need. > > Caleb > >> >> Thanks >> -Vincent >> >>> 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 _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
