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();
}
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
