Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java

[Adam Heath-2]

[hidden email] wrote:
> Author: adrianc
> Date: Sun Jan 31 00:08:14 2010
> New Revision: 904921
>
> URL: http://svn.apache.org/viewvc?rev=904921&view=rev
> Log:
> Some changes to the UEL integration. While attempting to upgrade the JUEL library, some flaws in my original code were exposed. This commit fixes them. Also, I added some more unit tests.

I would have split this commit into several.

> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java?rev=904921&r1=904920&r2=904921&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java Sun Jan 31 00:08:14 2010
> @@ -120,9 +120,11 @@
>      }
>  
>      protected static class BasicContext extends ELContext {
> +        protected final Map<String, Object> variables;
>          protected final VariableMapper variableMapper;
>          public BasicContext(Map<String, ? extends Object> context) {
> -            this.variableMapper = new BasicVariableMapper(context, this);
> +            this.variableMapper = new BasicVariableMapper(this);
> +            this.variables = UtilGenerics.cast(context);

This is wrong.  You've converted a read only map to a writable one.
The syntax on context means that BasicContext doesn't insert new
items, nor replace any items in context.

If you really need to modify variable, then change the generics markup
on context.

[Adrian Crum-2]

--- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:

> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 4:36 PM
> [hidden email]
> wrote:
> > Author: adrianc
> > Date: Sun Jan 31 00:08:14 2010
> > New Revision: 904921
> >
> > URL: http://svn.apache.org/viewvc?rev=904921&view=rev
> > Log:
> > Some changes to the UEL integration. While attempting
> to upgrade the JUEL library, some flaws in my original code
> were exposed. This commit fixes them. Also, I added some
> more unit tests.
>
> I would have split this commit into several.
>
> > Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java?rev=904921&r1=904920&r2=904921&view=diff
> >
> ==============================================================================
> > ---
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> (original)
> > +++
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> Sun Jan 31 00:08:14 2010
> > @@ -120,9 +120,11 @@
> >      }
> > 
> >      protected static class
> BasicContext extends ELContext {
> > +        protected final
> Map<String, Object> variables;
> >          protected final
> VariableMapper variableMapper;
> >          public
> BasicContext(Map<String, ? extends Object> context) {
> > -           
> this.variableMapper = new BasicVariableMapper(context,
> this);
> > +           
> this.variableMapper = new BasicVariableMapper(this);
> > +           
> this.variables = UtilGenerics.cast(context);
>
> This is wrong.  You've converted a read only map to a
> writable one.
> The syntax on context means that BasicContext doesn't
> insert new
> items, nor replace any items in context.
>
> If you really need to modify variable, then change the
> generics markup
> on context.

I appreciate the review and comments, but I think you're not understanding the integration. I can't change the method signature because it is part of the JSR 245 specification, and yes, I really need to write to the context because that is the whole point of the integration.

[Adrian Crum-2]

--- On Sat, 1/30/10, Adrian Crum <[hidden email]> wrote:

> From: Adrian Crum <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 4:45 PM
> --- On Sat, 1/30/10, Adam Heath
> <[hidden email]>
> wrote:
> > Subject: Re: svn commit: r904921 - in
> /ofbiz/trunk/framework/base/src/org/ofbiz/base:
> test/BaseUnitTests.java util/string/UelUtil.java
> > To: [hidden email]
> > Date: Saturday, January 30, 2010, 4:36 PM
> > [hidden email]
> > wrote:
> > > Author: adrianc
> > > Date: Sun Jan 31 00:08:14 2010
> > > New Revision: 904921
> > >
> > > URL: http://svn.apache.org/viewvc?rev=904921&view=rev
> > > Log:
> > > Some changes to the UEL integration. While
> attempting
> > to upgrade the JUEL library, some flaws in my original
> code
> > were exposed. This commit fixes them. Also, I added
> some
> > more unit tests.
> >
> > I would have split this commit into several.
> >
> > > Modified:
> >
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> > > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java?rev=904921&r1=904920&r2=904921&view=diff
> > >
> >
> ==============================================================================
> > > ---
> >
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> > (original)
> > > +++
> >
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/UelUtil.java
> > Sun Jan 31 00:08:14 2010
> > > @@ -120,9 +120,11 @@
> > >      }
> > > 
> > >      protected static class
> > BasicContext extends ELContext {
> > > +        protected final
> > Map<String, Object> variables;
> > >          protected final
> > VariableMapper variableMapper;
> > >          public
> > BasicContext(Map<String, ? extends Object>
> context) {
> > > -           
> > this.variableMapper = new
> BasicVariableMapper(context,
> > this);
> > > +           
> > this.variableMapper = new BasicVariableMapper(this);
> > > +           
> > this.variables = UtilGenerics.cast(context);
> >
> > This is wrong.  You've converted a read only map to
> a
> > writable one.
> > The syntax on context means that BasicContext doesn't
> > insert new
> > items, nor replace any items in context.
> >
> > If you really need to modify variable, then change
> the
> > generics markup
> > on context.
>
> I appreciate the review and comments, but I think you're
> not understanding the integration. I can't change the method
> signature because it is part of the JSR 245 specification,
> and yes, I really need to write to the context because that
> is the whole point of the integration.

Umm.. err.. what I meant to say was, I have to accept the read-only Map provided by the framework and write to it. The method signatures are actually my own, and I was just passing the Map along the same way I got it.

[Adam Heath-2]

Adrian Crum wrote:
>> If you really need to modify variable, then change the
>> generics markup
>> on context.
>
> I appreciate the review and comments, but I think you're not understanding the integration. I can't change the method signature because it is part of the JSR 245 specification, and yes, I really need to write to the context because that is the whole point of the integration.

Well, then, the integration is wrong.  If that method signature is
part of the spec, then you can't just go and do an end run around it
because you feel like it.

[Adrian Crum-2]

--- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 4:57 PM
> Adrian Crum wrote:
> >> If you really need to modify variable, then change
> the
> >> generics markup
> >> on context.
> >
> > I appreciate the review and comments, but I think
> you're not understanding the integration. I can't change the
> method signature because it is part of the JSR 245
> specification, and yes, I really need to write to the
> context because that is the whole point of the integration.
>
> Well, then, the integration is wrong.  If that method
> signature is
> part of the spec, then you can't just go and do an end run
> around it
> because you feel like it.

The context Map that is passed around the framework (in services, mini-lang, and screen widgets) is referred to as Map<String, ? extends Object>. Yet some portions of the framework need to write to that Map - it's their job to do so. So, what do you suggest?

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:
>> From: Adam Heath <[hidden email]>
>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>> To: [hidden email]
>> Date: Saturday, January 30, 2010, 4:57 PM
>> Adrian Crum wrote:
>>>> If you really need to modify variable, then change
>> the
>>>> generics markup
>>>> on context.
>>> I appreciate the review and comments, but I think
>> you're not understanding the integration. I can't change the
>> method signature because it is part of the JSR 245
>> specification, and yes, I really need to write to the
>> context because that is the whole point of the integration.
>>
>> Well, then, the integration is wrong.  If that method
>> signature is
>> part of the spec, then you can't just go and do an end run
>> around it
>> because you feel like it.
>
> The context Map that is passed around the framework (in services, mini-lang, and screen widgets) is referred to as Map<String, ? extends Object>. Yet some portions of the framework need to write to that Map - it's their job to do so. So, what do you suggest?

Dig further.  You'll find that this supposed read-only map is actually
already a copy in places, so if you really do write to it, the changes
will just be thrown away anyways.

In other cases, the original map that enters the system is already a
throw-away.

When I originally came up with the read-only generics for service
engine calls, I tried to make the maps writable.  But upon modifying
the entire stack, I discovered it wasn't possible.  The service engine
made copies in some cases, so the underlying service implementations
weren't able to send data back at all to the original caller.  To
encapsulate this, I made the map read only, by using the ? extends syntax.

What kind of modifications do you need to do?  Auto-vivification kinda
stuff?

[David E. Jones-2]

On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:

> Adrian Crum wrote:
>> --- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:
>>> From: Adam Heath <[hidden email]>
>>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>>> To: [hidden email]
>>> Date: Saturday, January 30, 2010, 4:57 PM
>>> Adrian Crum wrote:
>>>>> If you really need to modify variable, then change
>>> the
>>>>> generics markup
>>>>> on context.
>>>> I appreciate the review and comments, but I think
>>> you're not understanding the integration. I can't change the
>>> method signature because it is part of the JSR 245
>>> specification, and yes, I really need to write to the
>>> context because that is the whole point of the integration.
>>>
>>> Well, then, the integration is wrong.  If that method
>>> signature is
>>> part of the spec, then you can't just go and do an end run
>>> around it
>>> because you feel like it.
>>
>> The context Map that is passed around the framework (in services, mini-lang, and screen widgets) is referred to as Map<String, ? extends Object>. Yet some portions of the framework need to write to that Map - it's their job to do so. So, what do you suggest?
>
> Dig further.  You'll find that this supposed read-only map is actually
> already a copy in places, so if you really do write to it, the changes
> will just be thrown away anyways.
>
> In other cases, the original map that enters the system is already a
> throw-away.
>
> When I originally came up with the read-only generics for service
> engine calls, I tried to make the maps writable.  But upon modifying
> the entire stack, I discovered it wasn't possible.  The service engine
> made copies in some cases, so the underlying service implementations
> weren't able to send data back at all to the original caller.  To
> encapsulate this, I made the map read only, by using the ? extends syntax.

Maybe this is not the best way to look at it. The point is not that you can't change the context, it is just that any changes to the context are local to the service itself.

If I had the service engine to design again I'd consider having it be more like the screen widget with the hierarchical context, and with the context as the local variable space (for all services except those written in plain Java where you can't inject into the variable space on the stack (not that I know of anyway... maybe there is a way and that would be cool).

-David


> What kind of modifications do you need to do?  Auto-vivification kinda
> stuff?

[Adrian Crum-2]

--- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 5:28 PM
> Adrian Crum wrote:
> > --- On Sat, 1/30/10, Adam Heath <[hidden email]>
> wrote:
> >> From: Adam Heath <[hidden email]>
> >> Subject: Re: svn commit: r904921 - in
> /ofbiz/trunk/framework/base/src/org/ofbiz/base:
> test/BaseUnitTests.java util/string/UelUtil.java
> >> To: [hidden email]
> >> Date: Saturday, January 30, 2010, 4:57 PM
> >> Adrian Crum wrote:
> >>>> If you really need to modify variable,
> then change
> >> the
> >>>> generics markup
> >>>> on context.
> >>> I appreciate the review and comments, but I
> think
> >> you're not understanding the integration. I can't
> change the
> >> method signature because it is part of the JSR
> 245
> >> specification, and yes, I really need to write to
> the
> >> context because that is the whole point of the
> integration.
> >>
> >> Well, then, the integration is wrong.  If
> that method
> >> signature is
> >> part of the spec, then you can't just go and do an
> end run
> >> around it
> >> because you feel like it.
> >
> > The context Map that is passed around the framework
> (in services, mini-lang, and screen widgets) is referred to
> as Map<String, ? extends Object>. Yet some portions of
> the framework need to write to that Map - it's their job to
> do so. So, what do you suggest?
>
> Dig further.  You'll find that this supposed read-only
> map is actually
> already a copy in places, so if you really do write to it,
> the changes
> will just be thrown away anyways.
>
> In other cases, the original map that enters the system is
> already a
> throw-away.
>
> When I originally came up with the read-only generics for
> service
> engine calls, I tried to make the maps writable.  But
> upon modifying
> the entire stack, I discovered it wasn't possible. 
> The service engine
> made copies in some cases, so the underlying service
> implementations
> weren't able to send data back at all to the original
> caller.  To
> encapsulate this, I made the map read only, by using the ?
> extends syntax.
>
> What kind of modifications do you need to do? 
> Auto-vivification kinda
> stuff?

No, basic OFBiz kinda stuff.

<set field="var" value="Hello World!"/>

I need to set the var Map element to "Hello World!" The Map I'm being handed is read-only.

The nexus is FlexibleMapAccessor. Most of the framework uses it, and its purpose is to update a Map or a List. But it gets handed a read-only Map. I just pass the read-only Map on to the UEL stuff, and then convert it when I actually need to change something in it.

[Adam Heath-2]

David E Jones wrote:
> On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:
>> When I originally came up with the read-only generics for service
>> engine calls, I tried to make the maps writable.  But upon modifying
>> the entire stack, I discovered it wasn't possible.  The service engine
>> made copies in some cases, so the underlying service implementations
>> weren't able to send data back at all to the original caller.  To
>> encapsulate this, I made the map read only, by using the ? extends syntax.
>
> Maybe this is not the best way to look at it. The point is not that you can't change the context, it is just that any changes to the context are local to the service itself.

Well, if people start modifying the passed in context, and they assume
that those changes will cascade out, then they will become very
confused when that's not the case.

You'll also find that most services implementations don't need to
modify the context.

You are correct for those that do, they consider it as a local
variable store.

> If I had the service engine to design again I'd consider having it be more like the screen widget with the hierarchical context, and with the context as the local variable space (for all services except those written in plain Java where you can't inject into the variable space on the stack (not that I know of anyway... maybe there is a way and that would be cool).

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:
>> From: Adam Heath <[hidden email]>
>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>> To: [hidden email]
>> Date: Saturday, January 30, 2010, 5:28 PM
>> Adrian Crum wrote:
>>> --- On Sat, 1/30/10, Adam Heath <[hidden email]>
>> wrote:
>>>> From: Adam Heath <[hidden email]>
>>>> Subject: Re: svn commit: r904921 - in
>> /ofbiz/trunk/framework/base/src/org/ofbiz/base:
>> test/BaseUnitTests.java util/string/UelUtil.java
>>>> To: [hidden email]
>>>> Date: Saturday, January 30, 2010, 4:57 PM
>>>> Adrian Crum wrote:
>>>>>> If you really need to modify variable,
>> then change
>>>> the
>>>>>> generics markup
>>>>>> on context.
>>>>> I appreciate the review and comments, but I
>> think
>>>> you're not understanding the integration. I can't
>> change the
>>>> method signature because it is part of the JSR
>> 245
>>>> specification, and yes, I really need to write to
>> the
>>>> context because that is the whole point of the
>> integration.
>>>> Well, then, the integration is wrong.  If
>> that method
>>>> signature is
>>>> part of the spec, then you can't just go and do an
>> end run
>>>> around it
>>>> because you feel like it.
>>> The context Map that is passed around the framework
>> (in services, mini-lang, and screen widgets) is referred to
>> as Map<String, ? extends Object>. Yet some portions of
>> the framework need to write to that Map - it's their job to
>> do so. So, what do you suggest?
>>
>> Dig further.  You'll find that this supposed read-only
>> map is actually
>> already a copy in places, so if you really do write to it,
>> the changes
>> will just be thrown away anyways.
>>
>> In other cases, the original map that enters the system is
>> already a
>> throw-away.
>>
>> When I originally came up with the read-only generics for
>> service
>> engine calls, I tried to make the maps writable.  But
>> upon modifying
>> the entire stack, I discovered it wasn't possible.
>> The service engine
>> made copies in some cases, so the underlying service
>> implementations
>> weren't able to send data back at all to the original
>> caller.  To
>> encapsulate this, I made the map read only, by using the ?
>> extends syntax.
>>
>> What kind of modifications do you need to do?
>> Auto-vivification kinda
>> stuff?
>
> No, basic OFBiz kinda stuff.
>
> <set field="var" value="Hello World!"/>
>
> I need to set the var Map element to "Hello World!" The Map I'm being handed is read-only.
>
> The nexus is FlexibleMapAccessor. Most of the framework uses it, and its purpose is to update a Map or a List. But it gets handed a read-only Map. I just pass the read-only Map on to the UEL stuff, and then convert it when I actually need to change something in it.

So that needs to be writable, then the call sites need to be updated.

Do multiple(many) java-based services call FMA?  Or are there just a
few places that do a mass proxy around the other systems?  I'm
thinking that maybe we should make this more explicit, documentation
that service implementations can modify their context, but any such
changes will not be propagated back to callers.


>
>
>
>      

[Adrian Crum-2]

--- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:

> >> What kind of modifications do you need to do?
> >> Auto-vivification kinda
> >> stuff?
> >
> > No, basic OFBiz kinda stuff.
> >
> > <set field="var" value="Hello World!"/>
> >
> > I need to set the var Map element to "Hello World!"
> The Map I'm being handed is read-only.
> >
> > The nexus is FlexibleMapAccessor. Most of the
> framework uses it, and its purpose is to update a Map or a
> List. But it gets handed a read-only Map. I just pass the
> read-only Map on to the UEL stuff, and then convert it when
> I actually need to change something in it.
>
> So that needs to be writable, then the call sites need to
> be updated.
>
> Do multiple(many) java-based services call FMA?  Or
> are there just a
> few places that do a mass proxy around the other
> systems?  I'm
> thinking that maybe we should make this more explicit,
> documentation
> that service implementations can modify their context, but
> any such
> changes will not be propagated back to callers.

I will think about it some more. FMA accepts a read-only Map to get values, and it accepts a writable Map to put values. The problem is in UelUtil - it wants a writable Map in either case. I will try to think of a solution.

Thanks for the review and advice.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:
>>>> What kind of modifications do you need to do?
>>>> Auto-vivification kinda
>>>> stuff?
>>> No, basic OFBiz kinda stuff.
>>>
>>> <set field="var" value="Hello World!"/>
>>>
>>> I need to set the var Map element to "Hello World!"
>> The Map I'm being handed is read-only.
>>> The nexus is FlexibleMapAccessor. Most of the
>> framework uses it, and its purpose is to update a Map or a
>> List. But it gets handed a read-only Map. I just pass the
>> read-only Map on to the UEL stuff, and then convert it when
>> I actually need to change something in it.
>>
>> So that needs to be writable, then the call sites need to
>> be updated.
>>
>> Do multiple(many) java-based services call FMA?  Or
>> are there just a
>> few places that do a mass proxy around the other
>> systems?  I'm
>> thinking that maybe we should make this more explicit,
>> documentation
>> that service implementations can modify their context, but
>> any such
>> changes will not be propagated back to callers.
>
> I will think about it some more. FMA accepts a read-only Map to get values, and it accepts a writable Map to put values. The problem is in UelUtil - it wants a writable Map in either case. I will try to think of a solution.

Actually, that's a good point, and something I wouldn't want to lose.

[Adrian Crum-2]

--- On Sat, 1/30/10, David E Jones <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 5:37 PM
>
> On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:
>
> > Adrian Crum wrote:
> >> --- On Sat, 1/30/10, Adam Heath <[hidden email]>
> wrote:
> >>> From: Adam Heath <[hidden email]>
> >>> Subject: Re: svn commit: r904921 - in
> /ofbiz/trunk/framework/base/src/org/ofbiz/base:
> test/BaseUnitTests.java util/string/UelUtil.java
> >>> To: [hidden email]
> >>> Date: Saturday, January 30, 2010, 4:57 PM
> >>> Adrian Crum wrote:
> >>>>> If you really need to modify variable,
> then change
> >>> the
> >>>>> generics markup
> >>>>> on context.
> >>>> I appreciate the review and comments, but
> I think
> >>> you're not understanding the integration. I
> can't change the
> >>> method signature because it is part of the JSR
> 245
> >>> specification, and yes, I really need to write
> to the
> >>> context because that is the whole point of the
> integration.
> >>>
> >>> Well, then, the integration is wrong.  If
> that method
> >>> signature is
> >>> part of the spec, then you can't just go and
> do an end run
> >>> around it
> >>> because you feel like it.
> >>
> >> The context Map that is passed around the
> framework (in services, mini-lang, and screen widgets) is
> referred to as Map<String, ? extends Object>. Yet some
> portions of the framework need to write to that Map - it's
> their job to do so. So, what do you suggest?
> >
> > Dig further.  You'll find that this supposed
> read-only map is actually
> > already a copy in places, so if you really do write to
> it, the changes
> > will just be thrown away anyways.
> >
> > In other cases, the original map that enters the
> system is already a
> > throw-away.
> >
> > When I originally came up with the read-only generics
> for service
> > engine calls, I tried to make the maps writable. 
> But upon modifying
> > the entire stack, I discovered it wasn't
> possible.  The service engine
> > made copies in some cases, so the underlying service
> implementations
> > weren't able to send data back at all to the original
> caller.  To
> > encapsulate this, I made the map read only, by using
> the ? extends syntax.
>
> Maybe this is not the best way to look at it. The point is
> not that you can't change the context, it is just that any
> changes to the context are local to the service itself.
>
> If I had the service engine to design again I'd consider
> having it be more like the screen widget with the
> hierarchical context, and with the context as the local
> variable space (for all services except those written in
> plain Java where you can't inject into the variable space on
> the stack (not that I know of anyway... maybe there is a way
> and that would be cool).

When I first got involved with OFBiz, I would write to the service context Map because I didn't know any better. Then when I got to know OFBiz better, I still wrote to the service context Map because I was lazy. Now I always copy the service context Map into a local Map if I want to change things.

The moral of the story is: developers shouldn't be allowed to write to the service context Map. If a service needs a Map for local storage, then it should create one.

[Adam Heath-2]

Adrian Crum wrote:
> The moral of the story is: developers shouldn't be allowed to write to the service context Map. If a service needs a Map for local storage, then it should create one.

Maybe the service engine should make the map readonly, with
Collections.unmodifiableMap?

[David E. Jones-2]

On Jan 30, 2010, at 8:30 PM, Adrian Crum wrote:

> --- On Sat, 1/30/10, David E Jones <[hidden email]> wrote:
>> From: David E Jones <[hidden email]>
>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>> To: [hidden email]
>> Date: Saturday, January 30, 2010, 5:37 PM
>>
>> On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:
>>>
>>> When I originally came up with the read-only generics
>> for service
>>> engine calls, I tried to make the maps writable.
>> But upon modifying
>>> the entire stack, I discovered it wasn't
>> possible.  The service engine
>>> made copies in some cases, so the underlying service
>> implementations
>>> weren't able to send data back at all to the original
>> caller.  To
>>> encapsulate this, I made the map read only, by using
>> the ? extends syntax.
>>
>> Maybe this is not the best way to look at it. The point is
>> not that you can't change the context, it is just that any
>> changes to the context are local to the service itself.
>>
>> If I had the service engine to design again I'd consider
>> having it be more like the screen widget with the
>> hierarchical context, and with the context as the local
>> variable space (for all services except those written in
>> plain Java where you can't inject into the variable space on
>> the stack (not that I know of anyway... maybe there is a way
>> and that would be cool).
>
> When I first got involved with OFBiz, I would write to the service context Map because I didn't know any better. Then when I got to know OFBiz better, I still wrote to the service context Map because I was lazy. Now I always copy the service context Map into a local Map if I want to change things.
>
> The moral of the story is: developers shouldn't be allowed to write to the service context Map. If a service needs a Map for local storage, then it should create one.

That's kind of interesting.

What got me thinking is that the normal practice for Java service is to create a local variable for pretty much everything in the context anyway (ie like: String partyId = (String) context.get("partyId");), and there are things like that all over the place.

As far as not being to write to the context... why not? Sometimes it's handy to use the current context as the basis for calling another service. There are certainly other ways to go about that... but...

-David

[Adam Heath-2]

David E Jones wrote:

> On Jan 30, 2010, at 8:30 PM, Adrian Crum wrote:
>
>> --- On Sat, 1/30/10, David E Jones <[hidden email]> wrote:
>>> From: David E Jones <[hidden email]>
>>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>>> To: [hidden email]
>>> Date: Saturday, January 30, 2010, 5:37 PM
>>>
>>> On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:
>>>> When I originally came up with the read-only generics
>>> for service
>>>> engine calls, I tried to make the maps writable.
>>> But upon modifying
>>>> the entire stack, I discovered it wasn't
>>> possible.  The service engine
>>>> made copies in some cases, so the underlying service
>>> implementations
>>>> weren't able to send data back at all to the original
>>> caller.  To
>>>> encapsulate this, I made the map read only, by using
>>> the ? extends syntax.
>>>
>>> Maybe this is not the best way to look at it. The point is
>>> not that you can't change the context, it is just that any
>>> changes to the context are local to the service itself.
>>>
>>> If I had the service engine to design again I'd consider
>>> having it be more like the screen widget with the
>>> hierarchical context, and with the context as the local
>>> variable space (for all services except those written in
>>> plain Java where you can't inject into the variable space on
>>> the stack (not that I know of anyway... maybe there is a way
>>> and that would be cool).
>> When I first got involved with OFBiz, I would write to the service context Map because I didn't know any better. Then when I got to know OFBiz better, I still wrote to the service context Map because I was lazy. Now I always copy the service context Map into a local Map if I want to change things.
>>
>> The moral of the story is: developers shouldn't be allowed to write to the service context Map. If a service needs a Map for local storage, then it should create one.
>
> That's kind of interesting.
>
> What got me thinking is that the normal practice for Java service is to create a local variable for pretty much everything in the context anyway (ie like: String partyId = (String) context.get("partyId");), and there are things like that all over the place.
>
> As far as not being to write to the context... why not? Sometimes it's handy to use the current context as the basis for calling another service. There are certainly other ways to go about that... but...

I'm not particular tied to either way of doing things.  However, I do
believe that service implementations should not be allowed to change
the original map that the service caller passed in.  In that vein, I
believe the service engine should be modified to make this copy(or a
differential map that only stores the changes).

Maybe modify the service model to specify whether the service needs to
have a writable copy of the context, or can take an unmodifiable version.

>
> -David
>
>

[Adrian Crum-2]

--- On Sat, 1/30/10, David E Jones <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 7:02 PM
>
> On Jan 30, 2010, at 8:30 PM, Adrian Crum wrote:
>
> > --- On Sat, 1/30/10, David E Jones <[hidden email]>
> wrote:
> >> From: David E Jones <[hidden email]>
> >> Subject: Re: svn commit: r904921 - in
> /ofbiz/trunk/framework/base/src/org/ofbiz/base:
> test/BaseUnitTests.java util/string/UelUtil.java
> >> To: [hidden email]
> >> Date: Saturday, January 30, 2010, 5:37 PM
> >>
> >> On Jan 30, 2010, at 7:28 PM, Adam Heath wrote:
> >>>
> >>> When I originally came up with the read-only
> generics
> >> for service
> >>> engine calls, I tried to make the maps
> writable.
> >> But upon modifying
> >>> the entire stack, I discovered it wasn't
> >> possible.  The service engine
> >>> made copies in some cases, so the underlying
> service
> >> implementations
> >>> weren't able to send data back at all to the
> original
> >> caller.  To
> >>> encapsulate this, I made the map read only, by
> using
> >> the ? extends syntax.
> >>
> >> Maybe this is not the best way to look at it. The
> point is
> >> not that you can't change the context, it is just
> that any
> >> changes to the context are local to the service
> itself.
> >>
> >> If I had the service engine to design again I'd
> consider
> >> having it be more like the screen widget with the
> >> hierarchical context, and with the context as the
> local
> >> variable space (for all services except those
> written in
> >> plain Java where you can't inject into the
> variable space on
> >> the stack (not that I know of anyway... maybe
> there is a way
> >> and that would be cool).
> >
> > When I first got involved with OFBiz, I would write to
> the service context Map because I didn't know any better.
> Then when I got to know OFBiz better, I still wrote to the
> service context Map because I was lazy. Now I always copy
> the service context Map into a local Map if I want to change
> things.
> >
> > The moral of the story is: developers shouldn't be
> allowed to write to the service context Map. If a service
> needs a Map for local storage, then it should create one.
>
> That's kind of interesting.
>
> What got me thinking is that the normal practice for Java
> service is to create a local variable for pretty much
> everything in the context anyway (ie like: String partyId =
> (String) context.get("partyId");), and there are things like
> that all over the place.
>
> As far as not being to write to the context... why not?
> Sometimes it's handy to use the current context as the basis
> for calling another service. There are certainly other ways
> to go about that... but...

In those cases you can use ModelService to create a new Map for the new service based on the Map from the current service.

Even mini-lang has a set-service-fields element to do that.

[Adrian Crum-2]

--- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
> To: [hidden email]
> Date: Saturday, January 30, 2010, 6:46 PM
> Adrian Crum wrote:
> > The moral of the story is: developers shouldn't be
> allowed to write to the service context Map. If a service
> needs a Map for local storage, then it should create one.
>
> Maybe the service engine should make the map readonly,
> with
> Collections.unmodifiableMap?

Yes, plus make get() method calls for parameters that are not defined in the service definition throw an exception. It should be an error to try to retrieve a parameter that is not in the service definition.



     

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 1/30/10, Adam Heath <[hidden email]> wrote:
>> From: Adam Heath <[hidden email]>
>> Subject: Re: svn commit: r904921 - in /ofbiz/trunk/framework/base/src/org/ofbiz/base: test/BaseUnitTests.java util/string/UelUtil.java
>> To: [hidden email]
>> Date: Saturday, January 30, 2010, 6:46 PM
>> Adrian Crum wrote:
>>> The moral of the story is: developers shouldn't be
>> allowed to write to the service context Map. If a service
>> needs a Map for local storage, then it should create one.
>>
>> Maybe the service engine should make the map readonly,
>> with
>> Collections.unmodifiableMap?
>
> Yes, plus make get() method calls for parameters that are not defined in the service definition throw an exception. It should be an error to try to retrieve a parameter that is not in the service definition.

It gets more complex too.  keySet() needs to be modified as well,
values() needs to check if the found value is from a key that is
actually allowed by the service, etc.

I believe the service engine is allowed to pass a map to a service,
where the map actually contains values other than those required; this
might occur with SECA or some such.

Actually, restricting get calls might be the wrong thing to do.  That
would change the contract that the Map interface gives for get.
Map<K, V> says what keys are stored in the map, but get takes an
Object parameter.  So, I vote against this get restriction.

>
>
>
>      

[Adrian Crum-2]

--- On Sat, 1/30/10, David E Jones <[hidden email]> wrote:

> On Jan 30, 2010, at 8:30 PM, Adrian Crum wrote:
> > The moral of the story is: developers shouldn't be
> allowed to write to the service context Map. If a service
> needs a Map for local storage, then it should create one.
>
> That's kind of interesting.
>
> What got me thinking is that the normal practice for Java
> service is to create a local variable for pretty much
> everything in the context anyway (ie like: String partyId =
> (String) context.get("partyId");), and there are things like
> that all over the place.
>
> As far as not being to write to the context... why not?
> Sometimes it's handy to use the current context as the basis
> for calling another service. There are certainly other ways
> to go about that... but...

Maybe it would help to step back a little and compare OFBiz back in the day when it was just David and Andrew, and what it is today. Back then using a Map for passing parameters was a cool idea. I don't know what the motivation was to use a Map to pass parameters when you designed the service engine, but at the time I'm sure the two of you knew that you weren't supposed to write to it.

Today, things are different. We have a lot of contributors. Many of those contributors might not know everything they need to know about the framework. So, the framework needs to protect itself from inexperienced or lazy developers. That was the moral of the story.

I made the changes Adam suggested in this thread because I agree with the concept he was promoting: Make the framework code impose rules that developers must follow. As the OFBiz code base and community grows, I'm sure we will need to do more of the same.

-Adrian