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