On Oct 16, 2012, at 3:08 PM, Andreas Jonsson <aj@member.fsf.org> wrote:

Hi,

2012-10-16 11:03, Vincent Massol skrev:

...

Hi Andreas,

It would be great if you could explain your real use case and the reason you wish to introduce this (I guess you have a use case where you need this :)).

I want to put authorization information in the execution context, and I want to trust that this information is carried over to any new execution context that is activated in the scope of the current request.

That's always true, except where there are bugs (i.e. code that doesn't clone the context but create a new one when it shouldn't) but that would be the same even with your proposal.

It is also great if the object that holds the information cannot be replaced. So, if you look at the code you will see that I have implemented a rather aggressive enforcment of the combination of the attributes read-only and inherited.

Ok I'll need to read your code… :)

But also, the need for the inherited property already exists for managing the properties "xwikicontext" and "velocityContext". Just look at the TODO and FIXME in DefaultExecutionContextManager in the master branch.

I wrote that code and those TODO/FIXME :) There's only one issue and you'll still have it: it's about putting objects that can be cloned in the EC (that includes the velocitycontext and xwikicontext).

But the general idea is just to be able to associate any kind of metadata with each property. It can be compared to how interpreted languages handle variables. Take for instance bash, where you can just set a variable to a value, wich implicitly declares the variable. But you can also explicitly declare a variable, and in the declaration you can provide flags that sets various attributes of the variable. (e.g, declare -r variable='readonly value'.)

I'd be fine if it solved a problem but so far I fail to see how your solution will solve the real issue we have now which is that the elements put in the EC are not cloneable...

...

See below.

On Oct 12, 2012, at 9:27 AM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi everyone,

I would like to propose to introduce a possibility to associate properties in the execution context with metadata by declaring them. We introduce the class 'ExecutionContextProperty' that will store the metadata attributes, in addition to the actual property value.

/** * Declare a property in the execution context. * * @param key The key that names the property * @param property The property to declare. */ public void declareProperty(String key, ExecutionContextProperty property) Does this mean the current ExecutionContext.setProperty() method would be deprecated/removed?

Why not use instead: setProperty(String key, ECP property)?

Setting and declaring a property are completely different concepts. In a purely declarative model, properties would have to be declared before being set. But what I'm proposing is more in line with what bash does with variables. Setting a value on an undefined property will implicitly declare it with default values for all attributes. You can also provide an initial value when declaring the property (which of course is required for read-only properties).

I still think a single semantic with setProperty is simpler to understand for the user. He can either pass an untyped value or a type one. BTW what would the default property type? Cloned?

...

...

The metadata governs how the property should be treated when managing the execution contexts. For instance, these attributes could be supported:

* Read-only - The value may not be updated within the the execution context. How do you ensure this? So getProperty() would continue to return the value and not an ECP instance right?

Yes, getProperty returns the value of the property. I have not added any method to obtain the declared ECP instance as I don't see any need for it right now.

Of course, it is only the reference to the value that is guaranteed to be read-only by the execution context. Depending on the application, it may be desirable to make the object immutable, but that is not up to the execution context to enforce.

...

So when the user calls declareProperty() with the same key as an existing key it would be refused if it's been declared Readonly?

A property may only be declared once in an execution context, so it will be refused if the property is already declared. It doesn't matter if it was an explicit or implicit declaration. Attempting to declare an already existing property is an indication that something is wrong, so raising an exception seems appropriate.

I don't understand this. You mean that we don't have a single use case where we wish to update a property put in the EC? Like for example a counter that we want to increment… IMO there's a valid use case in modifying an existing property in the EC.

If the property is read-only, then also setProperty will refuse to replace the value.

That one makes sense.

...

Now imagine that I pass an ECP instance with a value of List. How do you prevent some from doing: getProperty("mylistkey").add(somenewelement)?

...

* Inherited - The property will be inherited from the current context when replacing the execution context within the scope of a request. I don't understand this one, we create a new Execution Context for each Request.

No, you are mistaken. We allow the creation and activation of several execution contexts per request. That's why we have the kludges in DefaultExecutionContextManager for copying and cloning "xwikicontext" and "velocityContext".

I'm pretty sure I'm right. We create a new EC for each Request and then there are parts of the code that require a clean context and for that they clean the current EC, push it and then pop it.

...

...

* Clone - Also clone the value when the execution context is cloned. What are the cases when we don't want to clone a property? I don't see any, for me we always want to clone them and in practice we would instead need to change our current signature to:

setProperty(String key, Cloneable value)

I do not want to clone my authorization data. It wouldn't hurt cloning it, but it would serve no purpose. But I can also imagine that some applications might want to put objects that are expensive to clone in the execution context.

Ah ok i think I understand what you mean now. You want to create a relationship between EC instances in the EC stack so that when reading a property value from the current EC, if the property is inherited it's taken from the next EC in the stack… So basically you wish to remove the need for cloning and replace it with inheritance. I get it now! :) It's interesting since it could make creating a new EC faster… I now understand the need for readonly. It's a bit dangerous though since it means there's no way of ensuring a clean EC and there's always the risk that you'd modify a value located in the previous EC in the stack. Unless we don't allow this, which would make sense (that's probably what you're doing I guess… :))...

...

...

* Type - The class of the value, for typechecking when replacing the value. Can you explain more? Does it mean you wish that getProperty() returns a typed value? We can already do this by using generics.

No, it just means that we can perform run-time type checking when setting the value of a declared property. It would catch some errors earlier, but not as early as compile time.

...

...

* Non-null - The value may not be null

So, the arguments for this proposal are at least two:

1. Catch errors early and simplify debugging. Can you explain more this point, I don't see the benefit over what we can already do.

You can let the execution context provide some basic validation when setting it. If you mark a value read-only, you will not need to worry about if any component suddenly replaces or remove it in the middle of the request.

...

...

2. Convenience of having properties automatically maintained across execution contexts within the same request cycle. This is probably related to the inherit item which I don't understand.

Yes it is. ;)

...

...

What do you think? Honestly I don't see the need for this.

What we lack IMO is proper Execution Context cloning and for this to happen we need to ensure we only cloneable items in the EC. As for the other "types" I don't really see a strong need but maybe I've misunderstood a few things (hence my questions above).

...

I would like to implement and merge this ASAP. We need to be very careful because the EC is a key class and introducing this means we're changing an important public API and we cannot go back easily (except through some deprecation cycles). IMO, seen the importance this should be a VOTE instead of a PROPOSAL.

The proposal adds two methods and one class, while beeing backwards compliant with current code. To me it seems that we have plenty of time to change our minds before releasing 4.3-rc1 if I were to merge this with master right now. Or is our policy that we have to commit to any API that we have added in a milestone release?

Well IMO it's much better to get an agreement before committing but you run the high risk to be asked to remove it in hurry at the last moment before the release and fix everything that's beed modified because of it + the stabilization risk when we're getting close to the release due to the rollback. We have a lazy commit rule but this rule means that any committer can ask for a rollback at any time. I'd have personally asked for one because I think this is a critical API and I would have liked to understand it before we put it in. Now, I've just understood your proposal and for me the winning argument is that your proposal makes it faster to create a new sub-EC for the same request and since we create an insane number of EC in the course of a request it can only be a good thing. Note: we need to check the place where we create those EC and fix them since I believe creation of a new EC is not always required. One such place for example is $doc.display() calls. I think each one create a new EC. So while your proposal adds a lot of complexity it's also interesting. I'd like to have other's opinion on this too. One thing that I'd like to see deprecated is the default setProperty() without Typing information. IMO we need to explicitly control each property with this proposal and not use a default. It becomes very important that each property's type is controlled since it can lead to important bugs. Thanks -Vincent

I will propose a vote for it later in any event.

Best Regards,

/Andreas

...

We would really need some feedback from several devs before going on with this IMO. ATM I'd like to understand it more before voting.

Thanks -Vincent

PS: Didn't get the time to read your implementation code yet

...

Best Regards,

/Andreas

On Wed, Oct 17, 2012 at 10:29 AM, Thomas Mortagne <thomas.mortagne@xwiki.com> wrote:

Sounds very interesting and I'm +1 in general for the idea.

Some remarks:

* is it possible to decide that you don't want to clone some property that has been declared as cloneable in the parent Context ? For example in your isolated code you have a well know property that could have been set in the parent context by any extension and which is completely different from yours

Asking this because in your proposal you can't redifine a property. Maybe the answer is that we want to allow redefining a property.

* it does not seems to be possible to have a use case where you make a property read-during for some time and then back to normal. Can you make a property in a new EContext both inherit the value of it's parent context and make it readonly even if the parent one is not ?

On Wed, Oct 17, 2012 at 9:20 AM, Vincent Massol <vincent@massol.net> wrote:

...

On Oct 16, 2012, at 3:08 PM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi,

2012-10-16 11:03, Vincent Massol skrev:

...

Hi Andreas,

It would be great if you could explain your real use case and the reason you wish to introduce this (I guess you have a use case where you need this :)).

I want to put authorization information in the execution context, and I want to trust that this information is carried over to any new execution context that is activated in the scope of the current request.

That's always true, except where there are bugs (i.e. code that doesn't clone the context but create a new one when it shouldn't) but that would be the same even with your proposal.

...

It is also great if the object that holds the information cannot be replaced. So, if you look at the code you will see that I have implemented a rather aggressive enforcment of the combination of the attributes read-only and inherited.

Ok I'll need to read your code… :)

...

But also, the need for the inherited property already exists for managing the properties "xwikicontext" and "velocityContext". Just look at the TODO and FIXME in DefaultExecutionContextManager in the master branch.

I wrote that code and those TODO/FIXME :)

There's only one issue and you'll still have it: it's about putting objects that can be cloned in the EC (that includes the velocitycontext and xwikicontext).

...

But the general idea is just to be able to associate any kind of metadata with each property. It can be compared to how interpreted languages handle variables. Take for instance bash, where you can just set a variable to a value, wich implicitly declares the variable. But you can also explicitly declare a variable, and in the declaration you can provide flags that sets various attributes of the variable. (e.g, declare -r variable='readonly value'.)

I'd be fine if it solved a problem but so far I fail to see how your solution will solve the real issue we have now which is that the elements put in the EC are not cloneable...

...

...

See below.

On Oct 12, 2012, at 9:27 AM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi everyone,

I would like to propose to introduce a possibility to associate properties in the execution context with metadata by declaring them. We introduce the class 'ExecutionContextProperty' that will store the metadata attributes, in addition to the actual property value.

/** * Declare a property in the execution context. * * @param key The key that names the property * @param property The property to declare. */ public void declareProperty(String key, ExecutionContextProperty property) Does this mean the current ExecutionContext.setProperty() method would be deprecated/removed?

Why not use instead: setProperty(String key, ECP property)?

Setting and declaring a property are completely different concepts. In a purely declarative model, properties would have to be declared before being set. But what I'm proposing is more in line with what bash does with variables. Setting a value on an undefined property will implicitly declare it with default values for all attributes. You can also provide an initial value when declaring the property (which of course is required for read-only properties).

I still think a single semantic with setProperty is simpler to understand for the user. He can either pass an untyped value or a type one.

BTW what would the default property type? Cloned?

...

...

...

The metadata governs how the property should be treated when managing the execution contexts. For instance, these attributes could be supported:

* Read-only - The value may not be updated within the the execution context. How do you ensure this? So getProperty() would continue to return the value and not an ECP instance right?

Yes, getProperty returns the value of the property. I have not added any method to obtain the declared ECP instance as I don't see any need for it right now.

Of course, it is only the reference to the value that is guaranteed to be read-only by the execution context. Depending on the application, it may be desirable to make the object immutable, but that is not up to the execution context to enforce.

...

So when the user calls declareProperty() with the same key as an existing key it would be refused if it's been declared Readonly?

A property may only be declared once in an execution context, so it will be refused if the property is already declared. It doesn't matter if it was an explicit or implicit declaration. Attempting to declare an already existing property is an indication that something is wrong, so raising an exception seems appropriate.

I don't understand this. You mean that we don't have a single use case where we wish to update a property put in the EC? Like for example a counter that we want to increment… IMO there's a valid use case in modifying an existing property in the EC.

You don't declare a new property in the case of a counter to use setProperty.

...

...

If the property is read-only, then also setProperty will refuse to replace the value.

That one makes sense.

...

...

Now imagine that I pass an ECP instance with a value of List. How do you prevent some from doing: getProperty("mylistkey").add(somenewelement)?

...

* Inherited - The property will be inherited from the current context when replacing the execution context within the scope of a request. I don't understand this one, we create a new Execution Context for each Request.

No, you are mistaken. We allow the creation and activation of several execution contexts per request. That's why we have the kludges in DefaultExecutionContextManager for copying and cloning "xwikicontext" and "velocityContext".

I'm pretty sure I'm right. We create a new EC for each Request and then there are parts of the code that require a clean context and for that they clean the current EC, push it and then pop it.

...

...

...

* Clone - Also clone the value when the execution context is cloned. What are the cases when we don't want to clone a property? I don't see any, for me we always want to clone them and in practice we would instead need to change our current signature to:

setProperty(String key, Cloneable value)

I do not want to clone my authorization data. It wouldn't hurt cloning it, but it would serve no purpose. But I can also imagine that some applications might want to put objects that are expensive to clone in the execution context.

Ah ok i think I understand what you mean now. You want to create a relationship between EC instances in the EC stack so that when reading a property value from the current EC, if the property is inherited it's taken from the next EC in the stack…

So basically you wish to remove the need for cloning and replace it with inheritance. I get it now! :)

It's interesting since it could make creating a new EC faster… I now understand the need for readonly. It's a bit dangerous though since it means there's no way of ensuring a clean EC and there's always the risk that you'd modify a value located in the previous EC in the stack. Unless we don't allow this, which would make sense (that's probably what you're doing I guess… :))...

...

...

...

* Type - The class of the value, for typechecking when replacing the value. Can you explain more? Does it mean you wish that getProperty() returns a typed value? We can already do this by using generics.

No, it just means that we can perform run-time type checking when setting the value of a declared property. It would catch some errors earlier, but not as early as compile time.

...

...

* Non-null - The value may not be null

So, the arguments for this proposal are at least two:

1. Catch errors early and simplify debugging. Can you explain more this point, I don't see the benefit over what we can already do.

You can let the execution context provide some basic validation when setting it. If you mark a value read-only, you will not need to worry about if any component suddenly replaces or remove it in the middle of the request.

...

...

2. Convenience of having properties automatically maintained across execution contexts within the same request cycle. This is probably related to the inherit item which I don't understand.

Yes it is. ;)

...

...

What do you think? Honestly I don't see the need for this.

What we lack IMO is proper Execution Context cloning and for this to happen we need to ensure we only cloneable items in the EC. As for the other "types" I don't really see a strong need but maybe I've misunderstood a few things (hence my questions above).

...

I would like to implement and merge this ASAP. We need to be very careful because the EC is a key class and introducing this means we're changing an important public API and we cannot go back easily (except through some deprecation cycles). IMO, seen the importance this should be a VOTE instead of a PROPOSAL.

The proposal adds two methods and one class, while beeing backwards compliant with current code. To me it seems that we have plenty of time to change our minds before releasing 4.3-rc1 if I were to merge this with master right now. Or is our policy that we have to commit to any API that we have added in a milestone release?

Well IMO it's much better to get an agreement before committing but you run the high risk to be asked to remove it in hurry at the last moment before the release and fix everything that's beed modified because of it + the stabilization risk when we're getting close to the release due to the rollback.

We have a lazy commit rule but this rule means that any committer can ask for a rollback at any time. I'd have personally asked for one because I think this is a critical API and I would have liked to understand it before we put it in.

Now, I've just understood your proposal and for me the winning argument is that your proposal makes it faster to create a new sub-EC for the same request and since we create an insane number of EC in the course of a request it can only be a good thing. Note: we need to check the place where we create those EC and fix them since I believe creation of a new EC is not always required. One such place for example is $doc.display() calls. I think each one create a new EC.

So while your proposal adds a lot of complexity it's also interesting. I'd like to have other's opinion on this too.

One thing that I'd like to see deprecated is the default setProperty() without Typing information. IMO we need to explicitly control each property with this proposal and not use a default. It becomes very important that each property's type is controlled since it can lead to important bugs.

Thanks -Vincent

...

I will propose a vote for it later in any event.

Best Regards,

/Andreas

...

We would really need some feedback from several devs before going on with this IMO. ATM I'd like to understand it more before voting.

Thanks -Vincent

PS: Didn't get the time to read your implementation code yet

...

Best Regards,

/Andreas

_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

-- Thomas Mortagne

2012-10-17 11:42, Andreas Jonsson skrev:

2012-10-17 10:30, Thomas Mortagne skrev:

...

On Wed, Oct 17, 2012 at 10:29 AM, Thomas Mortagne <thomas.mortagne@xwiki.com> wrote:

...

Sounds very interesting and I'm +1 in general for the idea.

Some remarks:

* is it possible to decide that you don't want to clone some property that has been declared as cloneable in the parent Context ? For example in your isolated code you have a well know property that could have been set in the parent context by any extension and which is completely different from yours Asking this because in your proposal you can't redifine a property. Maybe the answer is that we want to allow redefining a property. As I see it, we can support three use patterns for the execution context manager:

@Inject Execution execution;

@Inject ExecutionContextManager ecm;

// Pattern 1:

ExecutionContext context = new ExecutionContext(); ecm.initialize(context);

// Pattern 2:

// The default execution context manager ignores the parameter! (Or // actually, it picks the xwikicontext from the passed execution // context, but otherwise ignores it. But I've removed that in my // branch.) ExecutionContext context = ecm.clone(null); execution.pushContext(context); try { // ... } finally { execution.popContext(); }

// Pattern 3:

ExecutionContext context = new ExecutionContext();

// Manual initialization of the context.

execution.pushContext(context); try { // ... } finally { execution.popContext(); }

In pattern 3, you can re-declare the property however you like, unless it was marked final (I've changed 'read-only' to 'final', because it seems more accurate) and inherited in the parent context.

...

...

* it does not seems to be possible to have a use case where you make a property read-during for some time and then back to normal. Can you make a property in a new EContext both inherit the value of it's parent context and make it readonly even if the parent one is not ? Yes, that is possible. You can do this (assuming it wasn't already final and inherited):

ExecutionContext context = new ExecutionContext();

ExecutionContextProperty property = new ExecutionContextProperty("foo"); property.setValue(execution.getContext().getProperty("foo")); property.setFinal(true); property.setInherited(true); context.declareProperty(property);

execution.pushContext(context); try { // ... } finally { execution.popContext(); }

Also, I didn't think of the fact that it is always possible to explicitly redeclare any non-final property by simply removing the old one first: context.removeProperty("foo"); ExecutionContextProperty property = new ExecutionContextProperty("foo"); property.setValue(execution.getContext().getProperty("foo")); property.setFinal(true); property.setInherited(true); context.declareProperty(property); Best Regards, /Andreas

...

...

On Wed, Oct 17, 2012 at 9:20 AM, Vincent Massol <vincent@massol.net> wrote:

...

On Oct 16, 2012, at 3:08 PM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi,

2012-10-16 11:03, Vincent Massol skrev:

...

Hi Andreas,

It would be great if you could explain your real use case and the reason you wish to introduce this (I guess you have a use case where you need this :)). I want to put authorization information in the execution context, and I want to trust that this information is carried over to any new execution context that is activated in the scope of the current request. That's always true, except where there are bugs (i.e. code that doesn't clone the context but create a new one when it shouldn't) but that would be the same even with your proposal.

...

It is also great if the object that holds the information cannot be replaced. So, if you look at the code you will see that I have implemented a rather aggressive enforcment of the combination of the attributes read-only and inherited. Ok I'll need to read your code… :)

...

But also, the need for the inherited property already exists for managing the properties "xwikicontext" and "velocityContext". Just look at the TODO and FIXME in DefaultExecutionContextManager in the master branch. I wrote that code and those TODO/FIXME :)

There's only one issue and you'll still have it: it's about putting objects that can be cloned in the EC (that includes the velocitycontext and xwikicontext).

...

But the general idea is just to be able to associate any kind of metadata with each property. It can be compared to how interpreted languages handle variables. Take for instance bash, where you can just set a variable to a value, wich implicitly declares the variable. But you can also explicitly declare a variable, and in the declaration you can provide flags that sets various attributes of the variable. (e.g, declare -r variable='readonly value'.) I'd be fine if it solved a problem but so far I fail to see how your solution will solve the real issue we have now which is that the elements put in the EC are not cloneable...

...

...

See below.

On Oct 12, 2012, at 9:27 AM, Andreas Jonsson <aj@member.fsf.org> wrote:

> Hi everyone, > > I would like to propose to introduce a possibility to associate > properties in the execution context with metadata by declaring them. We > introduce the class 'ExecutionContextProperty' that will store the > metadata attributes, in addition to the actual property value. > > /** > * Declare a property in the execution context. > * > * @param key The key that names the property > * @param property The property to declare. > */ > public void declareProperty(String key, ExecutionContextProperty property) Does this mean the current ExecutionContext.setProperty() method would be deprecated/removed?

Why not use instead: setProperty(String key, ECP property)? Setting and declaring a property are completely different concepts. In a purely declarative model, properties would have to be declared before being set. But what I'm proposing is more in line with what bash does with variables. Setting a value on an undefined property will implicitly declare it with default values for all attributes. You can also provide an initial value when declaring the property (which of course is required for read-only properties). I still think a single semantic with setProperty is simpler to understand for the user. He can either pass an untyped value or a type one.

BTW what would the default property type? Cloned?

...

...

> The metadata governs how the property should be treated when managing > the execution contexts. For instance, these attributes could be supported: > > * Read-only - The value may not be updated within the the execution > context. How do you ensure this? So getProperty() would continue to return the value and not an ECP instance right? Yes, getProperty returns the value of the property. I have not added any method to obtain the declared ECP instance as I don't see any need for it right now.

Of course, it is only the reference to the value that is guaranteed to be read-only by the execution context. Depending on the application, it may be desirable to make the object immutable, but that is not up to the execution context to enforce.

...

So when the user calls declareProperty() with the same key as an existing key it would be refused if it's been declared Readonly? A property may only be declared once in an execution context, so it will be refused if the property is already declared. It doesn't matter if it was an explicit or implicit declaration. Attempting to declare an already existing property is an indication that something is wrong, so raising an exception seems appropriate. I don't understand this. You mean that we don't have a single use case where we wish to update a property put in the EC? Like for example a counter that we want to increment… IMO there's a valid use case in modifying an existing property in the EC. You don't declare a new property in the case of a counter to use setProperty.

...

...

If the property is read-only, then also setProperty will refuse to replace the value. That one makes sense.

...

...

Now imagine that I pass an ECP instance with a value of List. How do you prevent some from doing: getProperty("mylistkey").add(somenewelement)?

> * Inherited - The property will be inherited from the current context > when replacing the execution context within the scope of a request. I don't understand this one, we create a new Execution Context for each Request. No, you are mistaken. We allow the creation and activation of several execution contexts per request. That's why we have the kludges in DefaultExecutionContextManager for copying and cloning "xwikicontext" and "velocityContext". I'm pretty sure I'm right. We create a new EC for each Request and then there are parts of the code that require a clean context and for that they clean the current EC, push it and then pop it.

...

...

> * Clone - Also clone the value when the execution context is cloned. What are the cases when we don't want to clone a property? I don't see any, for me we always want to clone them and in practice we would instead need to change our current signature to:

setProperty(String key, Cloneable value) I do not want to clone my authorization data. It wouldn't hurt cloning it, but it would serve no purpose. But I can also imagine that some applications might want to put objects that are expensive to clone in the execution context. Ah ok i think I understand what you mean now. You want to create a relationship between EC instances in the EC stack so that when reading a property value from the current EC, if the property is inherited it's taken from the next EC in the stack…

So basically you wish to remove the need for cloning and replace it with inheritance. I get it now! :)

It's interesting since it could make creating a new EC faster… I now understand the need for readonly. It's a bit dangerous though since it means there's no way of ensuring a clean EC and there's always the risk that you'd modify a value located in the previous EC in the stack. Unless we don't allow this, which would make sense (that's probably what you're doing I guess… :))...

...

...

> * Type - The class of the value, for typechecking when replacing > the value. Can you explain more? Does it mean you wish that getProperty() returns a typed value? We can already do this by using generics. No, it just means that we can perform run-time type checking when setting the value of a declared property. It would catch some errors earlier, but not as early as compile time.

...

> * Non-null - The value may not be null > > So, the arguments for this proposal are at least two: > > 1. Catch errors early and simplify debugging. Can you explain more this point, I don't see the benefit over what we can already do. You can let the execution context provide some basic validation when setting it. If you mark a value read-only, you will not need to worry about if any component suddenly replaces or remove it in the middle of the request.

...

> 2. Convenience of having properties automatically maintained across > execution contexts within the same request cycle. This is probably related to the inherit item which I don't understand. Yes it is. ;)

...

> What do you think? Honestly I don't see the need for this.

What we lack IMO is proper Execution Context cloning and for this to happen we need to ensure we only cloneable items in the EC. As for the other "types" I don't really see a strong need but maybe I've misunderstood a few things (hence my questions above).

> I would like to implement and merge this ASAP. We need to be very careful because the EC is a key class and introducing this means we're changing an important public API and we cannot go back easily (except through some deprecation cycles). IMO, seen the importance this should be a VOTE instead of a PROPOSAL. The proposal adds two methods and one class, while beeing backwards compliant with current code. To me it seems that we have plenty of time to change our minds before releasing 4.3-rc1 if I were to merge this with master right now. Or is our policy that we have to commit to any API that we have added in a milestone release? Well IMO it's much better to get an agreement before committing but you run the high risk to be asked to remove it in hurry at the last moment before the release and fix everything that's beed modified because of it + the stabilization risk when we're getting close to the release due to the rollback.

We have a lazy commit rule but this rule means that any committer can ask for a rollback at any time. I'd have personally asked for one because I think this is a critical API and I would have liked to understand it before we put it in.

Now, I've just understood your proposal and for me the winning argument is that your proposal makes it faster to create a new sub-EC for the same request and since we create an insane number of EC in the course of a request it can only be a good thing. Note: we need to check the place where we create those EC and fix them since I believe creation of a new EC is not always required. One such place for example is $doc.display() calls. I think each one create a new EC.

So while your proposal adds a lot of complexity it's also interesting. I'd like to have other's opinion on this too.

One thing that I'd like to see deprecated is the default setProperty() without Typing information. IMO we need to explicitly control each property with this proposal and not use a default. It becomes very important that each property's type is controlled since it can lead to important bugs.

Thanks -Vincent

...

I will propose a vote for it later in any event.

Best Regards,

/Andreas

...

We would really need some feedback from several devs before going on with this IMO. ATM I'd like to understand it more before voting.

Thanks -Vincent

PS: Didn't get the time to read your implementation code yet

> Best Regards, > > /Andreas

---

devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

Hi, I've made some changes to the implementation. I changed the name of the "read-only" attribute to "final", because it seems more approprate. https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... Also I have integrated it in core and the functional tests seem to work. (I had some errors running them, but the errors didn't seem to be related to this feature.) https://github.com/xwiki/xwiki-platform/commit/4ceb225fe31846d4f9080eb8351f3... It would be much nicer if we had an execution context initializer for the "real" xwikicontext. Maybe we need the ability to order the initializers for that to work? More comments below... 2012-10-17 09:20, Vincent Massol skrev:

On Oct 16, 2012, at 3:08 PM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi,

2012-10-16 11:03, Vincent Massol skrev:

...

Hi Andreas,

It would be great if you could explain your real use case and the reason you wish to introduce this (I guess you have a use case where you need this :)). I want to put authorization information in the execution context, and I want to trust that this information is carried over to any new execution context that is activated in the scope of the current request. That's always true, except where there are bugs (i.e. code that doesn't clone the context but create a new one when it shouldn't) but that would be the same even with your proposal.

...

It is also great if the object that holds the information cannot be replaced. So, if you look at the code you will see that I have implemented a rather aggressive enforcment of the combination of the attributes read-only and inherited. Ok I'll need to read your code… :)

...

But also, the need for the inherited property already exists for managing the properties "xwikicontext" and "velocityContext". Just look at the TODO and FIXME in DefaultExecutionContextManager in the master branch. I wrote that code and those TODO/FIXME :)

There's only one issue and you'll still have it: it's about putting objects that can be cloned in the EC (that includes the velocitycontext and xwikicontext).

I'm not sure I understand what you mean. In my solution you can explicitly declare that you want the values cloned.

...

But the general idea is just to be able to associate any kind of metadata with each property. It can be compared to how interpreted languages handle variables. Take for instance bash, where you can just set a variable to a value, wich implicitly declares the variable. But you can also explicitly declare a variable, and in the declaration you can provide flags that sets various attributes of the variable. (e.g, declare -r variable='readonly value'.) I'd be fine if it solved a problem but so far I fail to see how your solution will solve the real issue we have now which is that the elements put in the EC are not cloneable...

...

...

See below.

On Oct 12, 2012, at 9:27 AM, Andreas Jonsson <aj@member.fsf.org> wrote:

...

Hi everyone,

I would like to propose to introduce a possibility to associate properties in the execution context with metadata by declaring them. We introduce the class 'ExecutionContextProperty' that will store the metadata attributes, in addition to the actual property value.

/** * Declare a property in the execution context. * * @param key The key that names the property * @param property The property to declare. */ public void declareProperty(String key, ExecutionContextProperty property) Does this mean the current ExecutionContext.setProperty() method would be deprecated/removed?

Why not use instead: setProperty(String key, ECP property)? Setting and declaring a property are completely different concepts. In a purely declarative model, properties would have to be declared before being set. But what I'm proposing is more in line with what bash does with variables. Setting a value on an undefined property will implicitly declare it with default values for all attributes. You can also provide an initial value when declaring the property (which of course is required for read-only properties). I still think a single semantic with setProperty is simpler to understand for the user. He can either pass an untyped value or a type one.

I would prefer different method names to clearly separate declaration from setting the value.

BTW what would the default property type? Cloned?

I have set the default to not cloning, but I'm open to changing that. Maybe the default should be 'clone if Cloneable'?

...

...

...

The metadata governs how the property should be treated when managing the execution contexts. For instance, these attributes could be supported:

* Read-only - The value may not be updated within the the execution context. How do you ensure this? So getProperty() would continue to return the value and not an ECP instance right? Yes, getProperty returns the value of the property. I have not added any method to obtain the declared ECP instance as I don't see any need for it right now.

Of course, it is only the reference to the value that is guaranteed to be read-only by the execution context. Depending on the application, it may be desirable to make the object immutable, but that is not up to the execution context to enforce.

...

So when the user calls declareProperty() with the same key as an existing key it would be refused if it's been declared Readonly? A property may only be declared once in an execution context, so it will be refused if the property is already declared. It doesn't matter if it was an explicit or implicit declaration. Attempting to declare an already existing property is an indication that something is wrong, so raising an exception seems appropriate. I don't understand this. You mean that we don't have a single use case where we wish to update a property put in the EC? Like for example a counter that we want to increment… IMO there's a valid use case in modifying an existing property in the EC.

But the property values can still be updated, unless they have been declared 'final' (what I previously called 'read-only').

...

If the property is read-only, then also setProperty will refuse to replace the value. That one makes sense.

...

...

Now imagine that I pass an ECP instance with a value of List. How do you prevent some from doing: getProperty("mylistkey").add(somenewelement)?

...

* Inherited - The property will be inherited from the current context when replacing the execution context within the scope of a request. I don't understand this one, we create a new Execution Context for each Request. No, you are mistaken. We allow the creation and activation of several execution contexts per request. That's why we have the kludges in DefaultExecutionContextManager for copying and cloning "xwikicontext" and "velocityContext". I'm pretty sure I'm right. We create a new EC for each Request and then there are parts of the code that require a clean context and for that they clean the current EC, push it and then pop it.

But it is this pushing and popping (and sometimes just replacing) of new contexts that I am worried about. I want to be able to ensure that a property value is propagated to all execution contexts that are activated during a request cycle.

...

...

...

* Clone - Also clone the value when the execution context is cloned. What are the cases when we don't want to clone a property? I don't see any, for me we always want to clone them and in practice we would instead need to change our current signature to:

setProperty(String key, Cloneable value) I do not want to clone my authorization data. It wouldn't hurt cloning it, but it would serve no purpose. But I can also imagine that some applications might want to put objects that are expensive to clone in the execution context. Ah ok i think I understand what you mean now. You want to create a relationship between EC instances in the EC stack so that when reading a property value from the current EC, if the property is inherited it's taken from the next EC in the stack…

So basically you wish to remove the need for cloning and replace it with inheritance. I get it now! :)

You are getting close. ;) The properties are copied or cloned (depending on the clonedValue attribute) to the new context, if they are marked as 'inherited'.

It's interesting since it could make creating a new EC faster… I now understand the need for readonly. It's a bit dangerous though since it means there's no way of ensuring a clean EC and there's always the risk that you'd modify a value located in the previous EC in the stack. Unless we don't allow this, which would make sense (that's probably what you're doing I guess… :))...

When putting a value into the execution context, you will have four options: 1) don't make it "inherited", 2) make it "inherited" and "cloned", 3) make it "inherited" and "final" 4) just make it "inherited". For options 3) and 4), the value should preferrably be immutable, but it is still up to the developer to decide. If you are making an application for tracing something, you will want a mutable instance to be propagated to new contexts.

...

...

...

* Type - The class of the value, for typechecking when replacing the value. Can you explain more? Does it mean you wish that getProperty() returns a typed value? We can already do this by using generics. No, it just means that we can perform run-time type checking when setting the value of a declared property. It would catch some errors earlier, but not as early as compile time.

...

...

* Non-null - The value may not be null

So, the arguments for this proposal are at least two:

1. Catch errors early and simplify debugging. Can you explain more this point, I don't see the benefit over what we can already do. You can let the execution context provide some basic validation when setting it. If you mark a value read-only, you will not need to worry about if any component suddenly replaces or remove it in the middle of the request.

...

...

2. Convenience of having properties automatically maintained across execution contexts within the same request cycle. This is probably related to the inherit item which I don't understand. Yes it is. ;)

...

...

What do you think? Honestly I don't see the need for this.

What we lack IMO is proper Execution Context cloning and for this to happen we need to ensure we only cloneable items in the EC. As for the other "types" I don't really see a strong need but maybe I've misunderstood a few things (hence my questions above).

...

I would like to implement and merge this ASAP. We need to be very careful because the EC is a key class and introducing this means we're changing an important public API and we cannot go back easily (except through some deprecation cycles). IMO, seen the importance this should be a VOTE instead of a PROPOSAL. The proposal adds two methods and one class, while beeing backwards compliant with current code. To me it seems that we have plenty of time to change our minds before releasing 4.3-rc1 if I were to merge this with master right now. Or is our policy that we have to commit to any API that we have added in a milestone release? Well IMO it's much better to get an agreement before committing but you run the high risk to be asked to remove it in hurry at the last moment before the release and fix everything that's beed modified because of it + the stabilization risk when we're getting close to the release due to the rollback.

We have a lazy commit rule but this rule means that any committer can ask for a rollback at any time. I'd have personally asked for one because I think this is a critical API and I would have liked to understand it before we put it in.

Now, I've just understood your proposal and for me the winning argument is that your proposal makes it faster to create a new sub-EC for the same request and since we create an insane number of EC in the course of a request it can only be a good thing. Note: we need to check the place where we create those EC and fix them since I believe creation of a new EC is not always required. One such place for example is $doc.display() calls. I think each one create a new EC.

Yes, that is another advantage of this proposal. But my main motivation, for the time beeing, is having the ability to enforce the combination 'inherited' and 'final' for the duration of a request.

So while your proposal adds a lot of complexity it's also interesting. I'd like to have other's opinion on this too.

I want to object to that. I do not think that it adds complexity, on the contrary it simplifies some use cases.

One thing that I'd like to see deprecated is the default setProperty() without Typing information.

I disagree, setting and declaring are different things. But we could disallow implicit declarations (i.e., setting the value of an undeclared property.) /Best Regards, Andreas

IMO we need to explicitly control each property with this proposal and not use a default. It becomes very important that each property's type is controlled since it can lead to important bugs.

Thanks -Vincent

...

I will propose a vote for it later in any event.

Best Regards,

/Andreas

...

We would really need some feedback from several devs before going on with this IMO. ATM I'd like to understand it more before voting.

Thanks -Vincent

PS: Didn't get the time to read your implementation code yet

...

Best Regards,

/Andreas

---

devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs