+1 Thanks, Marius On Thu, Sep 15, 2011 at 6:04 PM, Thomas Mortagne <[email protected]> wrote:

+1

On Thu, Sep 15, 2011 at 4:35 PM, Vincent Massol <[email protected]> wrote:

...

Hi devs,

After much brainstorming with Thomas and with the implementation of the new model I'm doing, we've come to the conclusion that it would be better to add the notions of Locale and Version in EntityReference.

The main reasons are: * otherwise we need to introduce a notion of UniqueEntityReference in the model and it makes it very awkward at an API level (user need to constuct a UniqueEntityReference from an EntityReference depending on the APIs used) * it makes APIs more complex than what they could be.

For example: - getDocument(EntityReference) - getDocument(EntityReference, Locale) - getDocument(EntityReference, Version) - getDocument(EntityReference, Locale, Version)

vs

- getDocument(EntityReference)

(subnote: this is why I introduced UniqueEntityReference in the model)

* It'll mean that anywhere we use an entity reference we'll be able to reference a specific version of that entity and/or a specific language. Some example: including a specific version of a page, referencing a specific version of an attachment in wiki syntax, etc.

In a first version I'd like to only introduce a generic get/setAttributes (to allow extensibility) in EntityReference + get/setLocale/Version (for easiness of access).

ATM I'm not planning to define a textual syntax for attributes (but it could something like: wiki:space.page[name1=value1, … nameN=valueN]). I'm not asking to vote on this.

Here's my +1

Thanks -Vincent

_______________________________________________ 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

On 10/13/2011 07:08 AM, Vincent Massol wrote:

After more brainstorming with Denis and Thomas here's some updated information about what we'd like to do:

* Make EntityReference immutable (ie remove the set* methods) - it's a best practice since it reduces potential mistakes (you pass a reference to some code and then somewhere else you call setXXX()) * Remove the clone() method (since it's less needed when the object is immutable and it's hard to implement in a generic way for generic parameters) * Have equals(), toString(), hashCode(), compareTo() all act on all EntityReference class fields (ie including the parameters) * Modify or add a new EntityReferenceResolver that will fill the parameters when they're missing. Of course the resolver will only know how to handle some parameters (Version and Locale ATM). * We thought that the safest way was to add the get/setLocale() and get/setVersion methods in DocumentReference for now (as helper methods to the generic add/getParameter methods in EntityReference. This is because the new model hasn't been voted yet and thus we're not yet sure that all Entities will support Version/Locale

Note that this will break backward compatibility in EntityReference but we think we need to do this ASAP.

Let us know if you think there's a problem.

Looks good, +1.

Thanks -Vincent

On Sep 15, 2011, at 4:35 PM, Vincent Massol wrote:

...

Hi devs,

After much brainstorming with Thomas and with the implementation of the new model I'm doing, we've come to the conclusion that it would be better to add the notions of Locale and Version in EntityReference.

The main reasons are: * otherwise we need to introduce a notion of UniqueEntityReference in the model and it makes it very awkward at an API level (user need to constuct a UniqueEntityReference from an EntityReference depending on the APIs used) * it makes APIs more complex than what they could be.

For example: - getDocument(EntityReference) - getDocument(EntityReference, Locale) - getDocument(EntityReference, Version) - getDocument(EntityReference, Locale, Version)

vs

- getDocument(EntityReference)

(subnote: this is why I introduced UniqueEntityReference in the model)

* It'll mean that anywhere we use an entity reference we'll be able to reference a specific version of that entity and/or a specific language. Some example: including a specific version of a page, referencing a specific version of an attachment in wiki syntax, etc.

In a first version I'd like to only introduce a generic get/setAttributes (to allow extensibility) in EntityReference + get/setLocale/Version (for easiness of access).

ATM I'm not planning to define a textual syntax for attributes (but it could something like: wiki:space.page[name1=value1, … nameN=valueN]). I'm not asking to vote on this.

Here's my +1

Thanks -Vincent

-- Sergiu Dumitriu http://purl.org/net/sergiu/

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/cea424914f40ce924afbc49b3159b...) :

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)

where Pair is http://commons.apache.org/lang/api-3.0-beta/org/apache/commons/lang3/Pair.ht...

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>.

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…

I had propose using overloaded constructor like

public DocumentReference(String wikiName, List<String> spaceNames, String pageName, Locale locale)

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?

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? Also the goal is to have unknown parameters so how can you do a mapping for something unknown? :) 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

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/cea424914f40ce924afbc49b3159b... )

...

:

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)

where Pair is

http://commons.apache.org/lang/api-3.0-beta/org/apache/commons/lang3/Pair.ht...

...

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)

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. 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.

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. 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) * 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

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/cea424914f40ce924afbc49b3159b... )

...

:

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.ht...

...

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. 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. 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, fooling them into thinking all versions of every document are available in the same namespace, 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.

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. 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. 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

On Tue, Nov 15, 2011 at 21:26, Vincent Massol <[email protected]> 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/cea424914f40ce924afbc49b3159b...

...

)

...

:

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.ht...

...

...

> > 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?

I am absolutely not happy with that. I had a hard time putting it to work and I do not want to wait further to merge immutable references. Since parameters cames with them, I will not loose my time removing them just because there is now a doubt on the initial vote here. I stay convince that we need to have language in references, and I have seen nobody saying it should not. The initial discussion came from the implementation of the constructor for Document Reference, which simply provide optional Locale to a document reference. So I do not understand your -1 now. What we do not agree on are arbitrary parameters, and having version one of them. So what is the problem to have my current implementation committed ? (With Serializable in place of Object, which is a good remarks since we need serializable references) Denis

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

-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO

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/cea424914f40ce924afbc49b3159b... )

...

:

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.ht... > > 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. 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

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/cea424914f40ce924afbc49b3159b... > ) >> : >> >> 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.ht... >>> >>> 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.

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

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/cea424914f40ce924afbc49b3159b... ) > : > > 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.ht... >> >> 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