28 Oct
2012
28 Oct
'12
9:43 p.m.
Hi Vincent,
2012-10-27 19:13, Vincent Massol skrev:
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();
Yes, the execution context manager seems redundant.
Hi Andreas,
On Oct 26, 2012, at 2:07 PM, Andreas Jonsson <aj(a)member.fsf.org> wrote:
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.
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-c…
https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-…
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.
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?
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-decla…
+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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs