[xwiki-devs] [VOTE] Property declarations for the execution context
Hi Everyone, I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2: https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c... Explanation of the feature: The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance: $ my_variable="some value" In the case of the execution context, the syntax is: context.setProperty("my_variable", "some value"); This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there: $ declare -ru my_variable="read only, forced upper case" Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes: * Final - The value may not be updated within the the execution context. Default: false. * Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. * Type - The class of the value, for typechecking when setting the value. Default: null (unchecked). * Non-null - The value may not be null, checked when setting the value. Default: false. Example declaration: ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The property value may be updated and the property may be removed and redeclared, unless declared 'final': context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.) Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following: @Inject Execution execution; @Inject ExecutionContextManager ecm; ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); } Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context: // Pushing a clone ExecutionContext context = ecm.clone(execution.getContext()); execution.pushContext(context); try { // Do work } finally { execution.popContext(); } // Pushing a clean context ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); } Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value. The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context. The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier. So to summarize, this feature: 1. is a convenient mechanism for managing properties in the execution context, 2. provides some validation of the property values, 3. improves performance slightly by avoiding unnecessary cloning. For more information, see the proposal thread: http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... Best Regards, /Andreas
Hi Andreas, On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false.
* Cloned value - Also clone the value when the execution context is cloned. Default: false.
ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default?
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property);
The API is very verbose just to declare one property… IMO it would be nice to have something more compact. As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable");
In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution;
No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext());
<brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one? Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have: ExecutionContext context = execution.getContext(); execution.pushContext(context.clone()) And to create an EC: ExecutionContext ec = execution.createContext(); Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar...
+0 from me after we agree on the items above. I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext). Thanks -Vincent
Forgot to mention that I'm leaving for holidays Monday morning for one week without computer nor internet access… :) Thanks -Vincent On Oct 27, 2012, at 7:13 PM, Vincent Massol <[email protected]> wrote:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false.
* Cloned value - Also clone the value when the execution context is cloned. Default: false.
ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default?
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property);
The API is very verbose just to declare one property… IMO it would be nice to have something more compact.
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable");
In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution;
No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext());
<brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one?
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone())
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar...
+0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext).
Thanks -Vincent
Hi Vincent, 2012-10-27 19:13, Vincent Massol skrev:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default?
No, it is not actually a compatibility issue, because the clone method in the execution context manager doesn't actually clone the execution context. This option is therefore only used when copying the properties when inheriting the parent context, which is a new feature. But having said that, it is possible that we should enable cloning by default. My thoughts on this is only that we cannot know if the the value can be cloned, which would be a slight complication.
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The API is very verbose just to declare one property… IMO it would be nice to have something more compact.
Ok. To make the API more compact, I changed it to use a builder pattern for declaring properties, for example: context.newProperty("key").type(String.class).nonNull().initial("value").declare();
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
Let's see if I understand you correctly. First, I would say that setting a property with attributes (i.e., setProperty(key, ECP)) is precisely what I mean by 'declaring' a property. You are just using a different method name. So, I'm taking the liberty to continue to use the word 'declaring' to denote 'setting a property with attributes'. With this in mind, do you suggest that we forbid updating property values altogether (i.e., the same as declaring them final by default with no option to not declare them final)? Also, do you suggest deprecating 'ExecutionContext.setValue(String, Object)' to disallow implicit declarations altogether? The ability to actually replace the value object seems very useful to me. A simple use case is that of keeping a boolean flag in the context: context.newProperty("flag").type(Boolean.class).nonNull() .initial((Boolean) false).declare(); // ... if (! ((Boolean) context.getProperty("flag"))) { context.setProperty("flag", (Boolean) true); // ... } As of implicit declarations, I have allowed them only to maintain backwards compatiblity. I would be in favour of disallowing them.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution; No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext()); <brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one?
Yes, the execution context manager seems redundant.
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone())
Given that the method ECM.clone(EC) doesn't actually clone anything, particularily not the execution context given as argument, I'd say that it should be deprecated in favor of making the execution context itself cloneable. Best Regards, /Andreas
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... +0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext).
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 10/28/2012 04:43 PM, Andreas Jonsson wrote:
Hi Vincent,
2012-10-27 19:13, Vincent Massol skrev:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default?
No, it is not actually a compatibility issue, because the clone method in the execution context manager doesn't actually clone the execution context. This option is therefore only used when copying the properties when inheriting the parent context, which is a new feature.
But having said that, it is possible that we should enable cloning by default. My thoughts on this is only that we cannot know if the the value can be cloned, which would be a slight complication.
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The API is very verbose just to declare one property… IMO it would be nice to have something more compact.
Ok. To make the API more compact, I changed it to use a builder pattern for declaring properties, for example:
context.newProperty("key").type(String.class).nonNull().initial("value").declare();
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
Let's see if I understand you correctly. First, I would say that setting a property with attributes (i.e., setProperty(key, ECP)) is precisely what I mean by 'declaring' a property. You are just using a different method name. So, I'm taking the liberty to continue to use the word 'declaring' to denote 'setting a property with attributes'.
With this in mind, do you suggest that we forbid updating property values altogether (i.e., the same as declaring them final by default with no option to not declare them final)? Also, do you suggest deprecating 'ExecutionContext.setValue(String, Object)' to disallow implicit declarations altogether?
The ability to actually replace the value object seems very useful to me. A simple use case is that of keeping a boolean flag in the context:
context.newProperty("flag").type(Boolean.class).nonNull() .initial((Boolean) false).declare();
// ...
if (! ((Boolean) context.getProperty("flag"))) { context.setProperty("flag", (Boolean) true); // ... }
As of implicit declarations, I have allowed them only to maintain backwards compatiblity. I would be in favour of disallowing them.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution; No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext()); <brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one?
Yes, the execution context manager seems redundant.
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone())
Given that the method ECM.clone(EC) doesn't actually clone anything, particularily not the execution context given as argument, I'd say that it should be deprecated in favor of making the execution context itself cloneable.
Best Regards,
/Andreas
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... +0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext).
I agree with the current state of the code, +1 for merge. -- Sergiu Dumitriu http://purl.org/net/sergiu/
Hi, Obviously, I should vote +1 myself as well. So, we have 2 +1 and 1 +0. Does anyone else have an opinion on this? Maybe Thomas? /Andreas 2012-10-29 19:31, Sergiu Dumitriu skrev:
On 10/28/2012 04:43 PM, Andreas Jonsson wrote:
Hi Vincent,
2012-10-27 19:13, Vincent Massol skrev:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default? No, it is not actually a compatibility issue, because the clone method in the execution context manager doesn't actually clone the execution context. This option is therefore only used when copying the properties when inheriting the parent context, which is a new feature.
But having said that, it is possible that we should enable cloning by default. My thoughts on this is only that we cannot know if the the value can be cloned, which would be a slight complication.
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The API is very verbose just to declare one property… IMO it would be nice to have something more compact. Ok. To make the API more compact, I changed it to use a builder pattern for declaring properties, for example:
context.newProperty("key").type(String.class).nonNull().initial("value").declare();
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
Let's see if I understand you correctly. First, I would say that setting a property with attributes (i.e., setProperty(key, ECP)) is precisely what I mean by 'declaring' a property. You are just using a different method name. So, I'm taking the liberty to continue to use the word 'declaring' to denote 'setting a property with attributes'.
With this in mind, do you suggest that we forbid updating property values altogether (i.e., the same as declaring them final by default with no option to not declare them final)? Also, do you suggest deprecating 'ExecutionContext.setValue(String, Object)' to disallow implicit declarations altogether?
The ability to actually replace the value object seems very useful to me. A simple use case is that of keeping a boolean flag in the context:
context.newProperty("flag").type(Boolean.class).nonNull() .initial((Boolean) false).declare();
// ...
if (! ((Boolean) context.getProperty("flag"))) { context.setProperty("flag", (Boolean) true); // ... }
As of implicit declarations, I have allowed them only to maintain backwards compatiblity. I would be in favour of disallowing them.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution; No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext()); <brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one? Yes, the execution context manager seems redundant.
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone()) Given that the method ECM.clone(EC) doesn't actually clone anything, particularily not the execution context given as argument, I'd say that it should be deprecated in favor of making the execution context itself cloneable.
Best Regards,
/Andreas
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... +0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext). I agree with the current state of the code, +1 for merge.
+1, sorry for the late answer, did not had much time to review your branch before On Wed, Oct 31, 2012 at 1:17 PM, Andreas Jonsson <[email protected]> wrote:
Hi,
Obviously, I should vote +1 myself as well. So, we have 2 +1 and 1 +0. Does anyone else have an opinion on this? Maybe Thomas?
/Andreas
2012-10-29 19:31, Sergiu Dumitriu skrev:
On 10/28/2012 04:43 PM, Andreas Jonsson wrote:
Hi Vincent,
2012-10-27 19:13, Vincent Massol skrev:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default? No, it is not actually a compatibility issue, because the clone method in the execution context manager doesn't actually clone the execution context. This option is therefore only used when copying the properties when inheriting the parent context, which is a new feature.
But having said that, it is possible that we should enable cloning by default. My thoughts on this is only that we cannot know if the the value can be cloned, which would be a slight complication.
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The API is very verbose just to declare one property… IMO it would be nice to have something more compact. Ok. To make the API more compact, I changed it to use a builder pattern for declaring properties, for example:
context.newProperty("key").type(String.class).nonNull().initial("value").declare();
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
Let's see if I understand you correctly. First, I would say that setting a property with attributes (i.e., setProperty(key, ECP)) is precisely what I mean by 'declaring' a property. You are just using a different method name. So, I'm taking the liberty to continue to use the word 'declaring' to denote 'setting a property with attributes'.
With this in mind, do you suggest that we forbid updating property values altogether (i.e., the same as declaring them final by default with no option to not declare them final)? Also, do you suggest deprecating 'ExecutionContext.setValue(String, Object)' to disallow implicit declarations altogether?
The ability to actually replace the value object seems very useful to me. A simple use case is that of keeping a boolean flag in the context:
context.newProperty("flag").type(Boolean.class).nonNull() .initial((Boolean) false).declare();
// ...
if (! ((Boolean) context.getProperty("flag"))) { context.setProperty("flag", (Boolean) true); // ... }
As of implicit declarations, I have allowed them only to maintain backwards compatiblity. I would be in favour of disallowing them.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution; No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext()); <brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one? Yes, the execution context manager seems redundant.
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone()) Given that the method ECM.clone(EC) doesn't actually clone anything, particularily not the execution context given as argument, I'd say that it should be deprecated in favor of making the execution context itself cloneable.
Best Regards,
/Andreas
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... +0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext). I agree with the current state of the code, +1 for merge.
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
Thank you! Now we have 3 +1 and 1 +0. Vote passed. I will merge the branch. Best Regards, /Andreas 2012-10-31 14:48, Thomas Mortagne skrev:
+1, sorry for the late answer, did not had much time to review your branch before
On Wed, Oct 31, 2012 at 1:17 PM, Andreas Jonsson <[email protected]> wrote:
Hi,
Obviously, I should vote +1 myself as well. So, we have 2 +1 and 1 +0. Does anyone else have an opinion on this? Maybe Thomas?
/Andreas
2012-10-29 19:31, Sergiu Dumitriu skrev:
On 10/28/2012 04:43 PM, Andreas Jonsson wrote:
Hi Vincent,
2012-10-27 19:13, Vincent Massol skrev:
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <[email protected]> wrote:
Hi Everyone,
I would like you to vote on merging the feature-execution-context branches of commons and platform before the release of 4.3M2:
https://github.com/xwiki/xwiki-commons/compare/master...feature-execution-co... https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-c...
Explanation of the feature:
The execution context is a simple map that binds a string to an object. This can be compared to how variables are handled by scripting languages such as bash, where an assignment brings the variable into existance:
$ my_variable="some value"
In the case of the execution context, the syntax is:
context.setProperty("my_variable", "some value");
This feature is about adding property declarations, where a property can be associated with attributes that controls how the execution context and execution context manager handles the property. The general idea can, once again, be compared to bash and how it is possible to declare variables there:
$ declare -ru my_variable="read only, forced upper case"
Of course, the set of attributes that are interesting to us is different from bash. Currently the feature branch have support for declaring properties with these attributes:
* Final - The value may not be updated within the the execution context. Default: false.
* Inherited - The property will be inherited from the current context when replacing the execution context within the current scope. Default: false. * Cloned value - Also clone the value when the execution context is cloned. Default: false. ATM I believe that all our properties are "cloned" since when we clone the context they are there. Won't this cause a backward compat issue? Shouldn't it be true by default? No, it is not actually a compatibility issue, because the clone method in the execution context manager doesn't actually clone the execution context. This option is therefore only used when copying the properties when inheriting the parent context, which is a new feature.
But having said that, it is possible that we should enable cloning by default. My thoughts on this is only that we cannot know if the the value can be cloned, which would be a slight complication.
* Type - The class of the value, for typechecking when setting the value. Default: null (unchecked).
* Non-null - The value may not be null, checked when setting the value. Default: false.
Example declaration:
ExecutionContextProperty property = new ExecutionContextProperty("my_variable"); property.setValue("some value"); property.setType(String.class); property.setInherited(true); context.declareProperty(property); The API is very verbose just to declare one property… IMO it would be nice to have something more compact. Ok. To make the API more compact, I changed it to use a builder pattern for declaring properties, for example:
context.newProperty("key").type(String.class).nonNull().initial("value").declare();
As I've already commented I'd personally have preferred that the ExecutionContextProperty and properties in general be immutable and set with one call to context.setProperty(key, ECP) (i.e. no need to declare IMO). I find it more straightforward and prevents forgetting to call declareProperty, which becomes really critical to get the right behavior you want.
Let's see if I understand you correctly. First, I would say that setting a property with attributes (i.e., setProperty(key, ECP)) is precisely what I mean by 'declaring' a property. You are just using a different method name. So, I'm taking the liberty to continue to use the word 'declaring' to denote 'setting a property with attributes'.
With this in mind, do you suggest that we forbid updating property values altogether (i.e., the same as declaring them final by default with no option to not declare them final)? Also, do you suggest deprecating 'ExecutionContext.setValue(String, Object)' to disallow implicit declarations altogether?
The ability to actually replace the value object seems very useful to me. A simple use case is that of keeping a boolean flag in the context:
context.newProperty("flag").type(Boolean.class).nonNull() .initial((Boolean) false).declare();
// ...
if (! ((Boolean) context.getProperty("flag"))) { context.setProperty("flag", (Boolean) true); // ... }
As of implicit declarations, I have allowed them only to maintain backwards compatiblity. I would be in favour of disallowing them.
The property value may be updated and the property may be removed and redeclared, unless declared 'final':
context.setProperty("my_variable", "some new value"); context.removeProperty("my_variable"); In general immutability is better in term of performance/implementation.
Additional attributes may be added later. This feature is also backwards compliant, in that implicit declaration by just setting a value is allowed. (Although we might want to make explicit declarations mandatory in the future.)
Within the scope of a request, or the life time of a thread executing some administrative task (e.g., the lucene index updater) the basic life cycle of the execution context is the following:
@Inject Execution execution; No need for this injection in your code below :)
@Inject ExecutionContextManager ecm;
ExecutionContext context = new ExecutionContext(); ecm.initialize(context); try { // Do work } finally { ecm.removeContext(); }
Within the life cycle of the "root" execution context, we may push a new execution context, which may either be a clean context, or a clone of the current context:
// Pushing a clone
ExecutionContext context = ecm.clone(execution.getContext()); <brainstorming mode> Actually I wonder why we have both Execution and ECM. Shouldn't we have just one? Yes, the execution context manager seems redundant.
Also I wonder why we have to call ecm.clone() instead of implementing clone() in ExecutionContext so that we would have:
ExecutionContext context = execution.getContext(); execution.pushContext(context.clone()) Given that the method ECM.clone(EC) doesn't actually clone anything, particularily not the execution context given as argument, I'd say that it should be deprecated in favor of making the execution context itself cloneable.
Best Regards,
/Andreas
And to create an EC: ExecutionContext ec = execution.createContext();
Ok maybe we would have a hard time with backward compat with this but for the sake of the discussion, would that be something better? </brainstorming mode>
execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
// Pushing a clean context
ExecutionContext context = new ExecutionContext(); execution.pushContext(context); try { // Do work } finally { execution.popContext(); }
Component authors that needs to place a value in the execution context provides an initializer that declares the variable and sets an initial value.
The attributes 'final', 'cloned value', and 'inherited' lets component authors control how the value is managed during the lifecycle of the root execution context.
The attributes 'type' and 'non-null' provides some runtime assertions to catch some errors earlier.
So to summarize, this feature:
1. is a convenient mechanism for managing properties in the execution context,
2. provides some validation of the property values,
3. improves performance slightly by avoiding unnecessary cloning.
For more information, see the proposal thread:
http://xwiki.475771.n2.nabble.com/PROPOSAL-Execution-context-property-declar... +0 from me after we agree on the items above.
I'd really like to get feedback from other committers before you push this since it's a really critical change (the Execution/EC is key and is going to become even more and more important as we move away from XWikiContext). I agree with the current state of the code, +1 for merge.
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
participants (4)
-
Andreas Jonsson -
Sergiu Dumitriu -
Thomas Mortagne -
Vincent Massol