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 <vincent(a)massol.net> 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/cea424914f40ce924afbc49b3159…
>> )
>>> :
>>>
>>> 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.h…
>>
>> 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 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.
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.
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'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.
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?
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