On Wed, Oct 17, 2012 at 2:06 PM, Andreas Jonsson <[email protected]> wrote:
> 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
>>> <[email protected]> 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");
Indeed.
>
> 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 <[email protected]> wrote:
>>>>> On Oct 16, 2012, at 3:08 PM, Andreas Jonsson <[email protected]> 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 <[email protected]>
>>>>>>> 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
>>>>> [email protected]
>>>>> http://lists.xwiki.org/mailman/listinfo/devs
>>>>
>>>> --
>>>> Thomas Mortagne
>>>
>
> _______________________________________________
> devs mailing list
> [email protected]
> http://lists.xwiki.org/mailman/listinfo/devs
--
Thomas Mortagne
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
