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-context-metadata >>> https://github.com/xwiki/xwiki-platform/compare/master...feature-execution-context-metadata >>> >>> 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-declarations-and-property-metadata-attributes-td7581766.html >> +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/ _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
