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");
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
