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. > * 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 ? > > 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 -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
