MapStack(which extends MapContext) does not implement Map stuff correctly

[Adam Heath-2]

I haven't had this be a problem for me in production anywhere, but I
noticed that when remove(Object) is called, it'll remove the value
from the top-most map in the stack.  This then means any queries for
the key will fall-thru to lower maps, which is definately *not* what
should occur.

Additionally, entrySet() returns an unmodifiable Collection of
Map.Entry.  However, those entries point to the *original* map.  If
someone called setValue() on the entry, it would modify the original
map, instead of inserting a value into the top-most map.

If others concur with my reading/understanding, then I'll go about
adding test cases which expose this, then fixing it.

[Adrian Crum-2]

--- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:
> I haven't had this be a problem for
> me in production anywhere, but I noticed that when
> remove(Object) is called, it'll remove the value from the
> top-most map in the stack.  This then means any queries
> for the key will fall-thru to lower maps, which is
> definately *not* what should occur.

That would depend on your point of view. From a variable scoping viewpoint, that would be the correct behavior.

Let's say I have two variables, both with the same name. One is declared early on and could be considered "global." The other is declared inside a method and is considered "local." If I remove the local variable, I would expect the global variable to still be there.

> Additionally, entrySet() returns an unmodifiable Collection
> of Map.Entry.  However, those entries point to the
> *original* map.  If someone called setValue() on the
> entry, it would modify the original map, instead of
> inserting a value into the top-most map.

Huh? How can you modify an unmodifiable collection? If that's possible, then my recommendation would be to enforce the unmodifiable part and not worry about which scope is being changed.

-Adrian

[Adam Heath-2]

On 09/10/2010 01:03 PM, Adrian Crum wrote:

> --- On Fri, 9/10/10, Adam Heath<[hidden email]>  wrote:
>> I haven't had this be a problem for
>> me in production anywhere, but I noticed that when
>> remove(Object) is called, it'll remove the value from the
>> top-most map in the stack.  This then means any queries
>> for the key will fall-thru to lower maps, which is
>> definately *not* what should occur.
>
> That would depend on your point of view. From a variable scoping viewpoint, that would be the correct behavior.
>
> Let's say I have two variables, both with the same name. One is declared early on and could be considered "global." The other is declared inside a method and is considered "local." If I remove the local variable, I would expect the global variable to still be there.

java.util.Map.remove(Object) says that it removes the object from the
map, such that Map.containsKey(Object) will return false afterwards.
That is what the contract states, and that's what MapStack/MapContext
should follow.

If you want another method to do what you say, then I can go ahead and
implement that.

As it is, if an instance of MapStack/MapContext is passed to legacy
code that only deals with maps, then it'll break.

>> Additionally, entrySet() returns an unmodifiable Collection
>> of Map.Entry.  However, those entries point to the
>> *original* map.  If someone called setValue() on the
>> entry, it would modify the original map, instead of
>> inserting a value into the top-most map.
>
> Huh? How can you modify an unmodifiable collection? If that's possible, then my recommendation would be to enforce the unmodifiable part and not worry about which scope is being changed.

You're not modifying the collection.  You're modifying a value *in*
the collection.  Collections.unmodifiableFoo only protects the collection.

Collection.iterator().next().setValue("foo") should place an override
entry into the top-level map, and then change the entry instance
that's in the underlying protected read-only collection.

>
> -Adrian
>
>
>
>

[Adrian Crum-2]

--- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:

> On 09/10/2010 01:03 PM, Adrian Crum
> wrote:
> > --- On Fri, 9/10/10, Adam Heath<[hidden email]> 
> wrote:
> >> I haven't had this be a problem for
> >> me in production anywhere, but I noticed that
> when
> >> remove(Object) is called, it'll remove the value
> from the
> >> top-most map in the stack.  This then means
> any queries
> >> for the key will fall-thru to lower maps, which
> is
> >> definately *not* what should occur.
> >
> > That would depend on your point of view. From a
> variable scoping viewpoint, that would be the correct
> behavior.
> >
> > Let's say I have two variables, both with the same
> name. One is declared early on and could be considered
> "global." The other is declared inside a method and is
> considered "local." If I remove the local variable, I would
> expect the global variable to still be there.
>
> java.util.Map.remove(Object) says that it removes the
> object from the
> map, such that Map.containsKey(Object) will return false
> afterwards.
> That is what the contract states, and that's what
> MapStack/MapContext
> should follow.
>
> If you want another method to do what you say, then I can
> go ahead and
> implement that.
>
> As it is, if an instance of MapStack/MapContext is passed
> to legacy
> code that only deals with maps, then it'll break.

Like I said, it's a matter of perspective. What is MapStack's purpose - to implement a Map, or to implement variable scoping? Based on the MapStack code and how it is used in the framework, I think it is the latter.

It would be helpful to hear from others.

> >> Additionally, entrySet() returns an unmodifiable
> Collection
> >> of Map.Entry.  However, those entries point
> to the
> >> *original* map.  If someone called setValue()
> on the
> >> entry, it would modify the original map, instead
> of
> >> inserting a value into the top-most map.
> >
> > Huh? How can you modify an unmodifiable collection? If
> that's possible, then my recommendation would be to enforce
> the unmodifiable part and not worry about which scope is
> being changed.
>
> You're not modifying the collection.  You're modifying
> a value *in*
> the collection.  Collections.unmodifiableFoo only
> protects the collection.
>
> Collection.iterator().next().setValue("foo") should place
> an override
> entry into the top-level map, and then change the entry
> instance
> that's in the underlying protected read-only collection.

If it's truly unmodifiable, Collection.iterator().next().setValue("foo") should throw an exception. In other words, the MapStack should be wrapped with an unmodifiable Map, and then return an entry set from the unmodifiable Map, or implement unmodifiable Map.Entry.

-Adrian

[Adam Heath-2]

On 09/10/2010 04:22 PM, Adrian Crum wrote:

> --- On Fri, 9/10/10, Adam Heath<[hidden email]>  wrote:
>> On 09/10/2010 01:03 PM, Adrian Crum
>> wrote:
>>> --- On Fri, 9/10/10, Adam Heath<[hidden email]>
>> wrote:
>>>> I haven't had this be a problem for
>>>> me in production anywhere, but I noticed that
>> when
>>>> remove(Object) is called, it'll remove the value
>> from the
>>>> top-most map in the stack.  This then means
>> any queries
>>>> for the key will fall-thru to lower maps, which
>> is
>>>> definately *not* what should occur.
>>>
>>> That would depend on your point of view. From a
>> variable scoping viewpoint, that would be the correct
>> behavior.
>>>
>>> Let's say I have two variables, both with the same
>> name. One is declared early on and could be considered
>> "global." The other is declared inside a method and is
>> considered "local." If I remove the local variable, I would
>> expect the global variable to still be there.
>>
>> java.util.Map.remove(Object) says that it removes the
>> object from the
>> map, such that Map.containsKey(Object) will return false
>> afterwards.
>> That is what the contract states, and that's what
>> MapStack/MapContext
>> should follow.
>>
>> If you want another method to do what you say, then I can
>> go ahead and
>> implement that.
>>
>> As it is, if an instance of MapStack/MapContext is passed
>> to legacy
>> code that only deals with maps, then it'll break.
>
> Like I said, it's a matter of perspective. What is MapStack's purpose - to implement a Map, or to implement variable scoping? Based on the MapStack code and how it is used in the framework, I think it is the latter.
>
> It would be helpful to hear from others.

MapStack implements Map.  It must follow the Map contract.  If you
remove the Map from the implements, then it can do whatever we want.

I can correct these problems, it's busy work for me.

>>>> Additionally, entrySet() returns an unmodifiable
>> Collection
>>>> of Map.Entry.  However, those entries point
>> to the
>>>> *original* map.  If someone called setValue()
>> on the
>>>> entry, it would modify the original map, instead
>> of
>>>> inserting a value into the top-most map.
>>>
>>> Huh? How can you modify an unmodifiable collection? If
>> that's possible, then my recommendation would be to enforce
>> the unmodifiable part and not worry about which scope is
>> being changed.
>>
>> You're not modifying the collection.  You're modifying
>> a value *in*
>> the collection.  Collections.unmodifiableFoo only
>> protects the collection.
>>
>> Collection.iterator().next().setValue("foo") should place
>> an override
>> entry into the top-level map, and then change the entry
>> instance
>> that's in the underlying protected read-only collection.
>
> If it's truly unmodifiable, Collection.iterator().next().setValue("foo") should throw an exception. In other words, the MapStack should be wrapped with an unmodifiable Map, and then return an entry set from the unmodifiable Map, or implement unmodifiable Map.Entry.

Again, that's not what the Map contract says.

MapStack implements Map, so that means programmers who see it expect
it to follow the rest of the java.util.Map universe.  Plus, MapStack
gets passed around to methods that only take a Map, and those
methods(and anything else they may call) expect it to behave correctly.

[Adrian Crum-2]

--- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:

> On 09/10/2010 04:22 PM, Adrian Crum
> wrote:
> > --- On Fri, 9/10/10, Adam Heath<[hidden email]> 
> wrote:
> >> On 09/10/2010 01:03 PM, Adrian Crum
> >> wrote:
> >>> --- On Fri, 9/10/10, Adam Heath<[hidden email]>
> >> wrote:
> >>>> I haven't had this be a problem for
> >>>> me in production anywhere, but I noticed
> that
> >> when
> >>>> remove(Object) is called, it'll remove the
> value
> >> from the
> >>>> top-most map in the stack.  This then
> means
> >> any queries
> >>>> for the key will fall-thru to lower maps,
> which
> >> is
> >>>> definately *not* what should occur.
> >>>
> >>> That would depend on your point of view. From
> a
> >> variable scoping viewpoint, that would be the
> correct
> >> behavior.
> >>>
> >>> Let's say I have two variables, both with the
> same
> >> name. One is declared early on and could be
> considered
> >> "global." The other is declared inside a method
> and is
> >> considered "local." If I remove the local
> variable, I would
> >> expect the global variable to still be there.
> >>
> >> java.util.Map.remove(Object) says that it removes
> the
> >> object from the
> >> map, such that Map.containsKey(Object) will return
> false
> >> afterwards.
> >> That is what the contract states, and that's what
> >> MapStack/MapContext
> >> should follow.
> >>
> >> If you want another method to do what you say,
> then I can
> >> go ahead and
> >> implement that.
> >>
> >> As it is, if an instance of MapStack/MapContext is
> passed
> >> to legacy
> >> code that only deals with maps, then it'll break.
> >
> > Like I said, it's a matter of perspective. What is
> MapStack's purpose - to implement a Map, or to implement
> variable scoping? Based on the MapStack code and how it is
> used in the framework, I think it is the latter.
> >
> > It would be helpful to hear from others.
>
> MapStack implements Map.  It must follow the Map
> contract.  If you
> remove the Map from the implements, then it can do whatever
> we want.
>
> I can correct these problems, it's busy work for me.

I have a feeling that will break a lot of screen widget/mini-language code.

> >>>> Additionally, entrySet() returns an
> unmodifiable
> >> Collection
> >>>> of Map.Entry.  However, those entries
> point
> >> to the
> >>>> *original* map.  If someone called
> setValue()
> >> on the
> >>>> entry, it would modify the original map,
> instead
> >> of
> >>>> inserting a value into the top-most map.
> >>>
> >>> Huh? How can you modify an unmodifiable
> collection? If
> >> that's possible, then my recommendation would be
> to enforce
> >> the unmodifiable part and not worry about which
> scope is
> >> being changed.
> >>
> >> You're not modifying the collection.  You're
> modifying
> >> a value *in*
> >> the collection.  Collections.unmodifiableFoo
> only
> >> protects the collection.
> >>
> >> Collection.iterator().next().setValue("foo")
> should place
> >> an override
> >> entry into the top-level map, and then change the
> entry
> >> instance
> >> that's in the underlying protected read-only
> collection.
> >
> > If it's truly unmodifiable,
> Collection.iterator().next().setValue("foo") should throw an
> exception. In other words, the MapStack should be wrapped
> with an unmodifiable Map, and then return an entry set from
> the unmodifiable Map, or implement unmodifiable Map.Entry.
>
> Again, that's not what the Map contract says.
>
> MapStack implements Map, so that means programmers who see
> it expect
> it to follow the rest of the java.util.Map universe. 
> Plus, MapStack
> gets passed around to methods that only take a Map, and
> those
> methods(and anything else they may call) expect it to
> behave correctly.
>

 

[Adam Heath-2]

On 09/10/2010 04:46 PM, Adrian Crum wrote:

> --- On Fri, 9/10/10, Adam Heath<[hidden email]>  wrote:
>> On 09/10/2010 04:22 PM, Adrian Crum
>> wrote:
>>> --- On Fri, 9/10/10, Adam Heath<[hidden email]>
>> wrote:
>>>> On 09/10/2010 01:03 PM, Adrian Crum
>>>> wrote:
>>>>> --- On Fri, 9/10/10, Adam Heath<[hidden email]>
>>>> wrote:
>>>>>> I haven't had this be a problem for
>>>>>> me in production anywhere, but I noticed
>> that
>>>> when
>>>>>> remove(Object) is called, it'll remove the
>> value
>>>> from the
>>>>>> top-most map in the stack.  This then
>> means
>>>> any queries
>>>>>> for the key will fall-thru to lower maps,
>> which
>>>> is
>>>>>> definately *not* what should occur.
>>>>>
>>>>> That would depend on your point of view. From
>> a
>>>> variable scoping viewpoint, that would be the
>> correct
>>>> behavior.
>>>>>
>>>>> Let's say I have two variables, both with the
>> same
>>>> name. One is declared early on and could be
>> considered
>>>> "global." The other is declared inside a method
>> and is
>>>> considered "local." If I remove the local
>> variable, I would
>>>> expect the global variable to still be there.
>>>>
>>>> java.util.Map.remove(Object) says that it removes
>> the
>>>> object from the
>>>> map, such that Map.containsKey(Object) will return
>> false
>>>> afterwards.
>>>> That is what the contract states, and that's what
>>>> MapStack/MapContext
>>>> should follow.
>>>>
>>>> If you want another method to do what you say,
>> then I can
>>>> go ahead and
>>>> implement that.
>>>>
>>>> As it is, if an instance of MapStack/MapContext is
>> passed
>>>> to legacy
>>>> code that only deals with maps, then it'll break.
>>>
>>> Like I said, it's a matter of perspective. What is
>> MapStack's purpose - to implement a Map, or to implement
>> variable scoping? Based on the MapStack code and how it is
>> used in the framework, I think it is the latter.

If it is supposed to implement variable scoping, then it doesn't, as
remove doesn't insert a sentinal.

And, to be safe, MapContext can't actually insert a sentinal into the
map at the top level.  It can't insert null, as that is a valid value.
  It can't insert an object of a different type, as that would break
the generics.  The only way to solve it is to have each level of the
stack have an out-of-band store for removed items(basically, a set of
keys).

>>>
>>> It would be helpful to hear from others.
>>
>> MapStack implements Map.  It must follow the Map
>> contract.  If you
>> remove the Map from the implements, then it can do whatever
>> we want.
>>
>> I can correct these problems, it's busy work for me.
>
> I have a feeling that will break a lot of screen widget/mini-language code.

Removing implements Map, or fixing the implementation?

I've seen code scattered around that checks if the Map is an instance
of MapStack, and then does a push, falling back on wrapping the map in
a new MapStack.  Such code shouldn't break

[Adrian Crum-2]

--- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:

> On 09/10/2010 04:46 PM, Adrian Crum
> wrote:
> > I have a feeling that will break a lot of screen
> widget/mini-language code.
>
> Removing implements Map, or fixing the implementation?
>
> I've seen code scattered around that checks if the Map is
> an instance
> of MapStack, and then does a push, falling back on wrapping
> the map in
> a new MapStack.  Such code shouldn't break

Let's cross our fingers.

I understand your viewpoint and I'm not arguing against it. If MapStack implements Map, then it should follow the Map contract - from a Java developer's viewpoint.

My concern is for the OFBiz user who is writing a simple method, and they expect their changes to local variables to be local - not global. The OFBiz user is not aware of - or concerned with - a "Map contract."

-Adrian

[Adam Heath-2]

On 09/10/2010 06:46 PM, Adrian Crum wrote:

> --- On Fri, 9/10/10, Adam Heath<[hidden email]>  wrote:
>> On 09/10/2010 04:46 PM, Adrian Crum
>> wrote:
>>> I have a feeling that will break a lot of screen
>> widget/mini-language code.
>>
>> Removing implements Map, or fixing the implementation?
>>
>> I've seen code scattered around that checks if the Map is
>> an instance
>> of MapStack, and then does a push, falling back on wrapping
>> the map in
>> a new MapStack.  Such code shouldn't break
>
> Let's cross our fingers.
>
> I understand your viewpoint and I'm not arguing against it. If MapStack implements Map, then it should follow the Map contract - from a Java developer's viewpoint.
>
> My concern is for the OFBiz user who is writing a simple method, and they expect their changes to local variables to be local - not global. The OFBiz user is not aware of - or concerned with - a "Map contract."

Huh?  Changes to map expected to be local, not global?  What do you
mean by that?  If I call map.remove(key) in a simple method, I expect
that the very next line after that will return false for
map.containsKey(key).

>
> -Adrian
>
>
>
>

[Adrian Crum-2]

--- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:

> On 09/10/2010 06:46 PM, Adrian Crum
> wrote:
> > --- On Fri, 9/10/10, Adam Heath<[hidden email]> 
> wrote:
> >> On 09/10/2010 04:46 PM, Adrian Crum
> >> wrote:
> >>> I have a feeling that will break a lot of
> screen
> >> widget/mini-language code.
> >>
> >> Removing implements Map, or fixing the
> implementation?
> >>
> >> I've seen code scattered around that checks if the
> Map is
> >> an instance
> >> of MapStack, and then does a push, falling back on
> wrapping
> >> the map in
> >> a new MapStack.  Such code shouldn't break
> >
> > Let's cross our fingers.
> >
> > I understand your viewpoint and I'm not arguing
> against it. If MapStack implements Map, then it should
> follow the Map contract - from a Java developer's
> viewpoint.
> >
> > My concern is for the OFBiz user who is writing a
> simple method, and they expect their changes to local
> variables to be local - not global. The OFBiz user is not
> aware of - or concerned with - a "Map contract."
>
> Huh?  Changes to map expected to be local, not
> global?  What do you
> mean by that?  If I call map.remove(key) in a simple
> method, I expect
> that the very next line after that will return false for
> map.containsKey(key).

If I write a simple method that declares a variable, then calls another simple method, I would expect the variable I declared to still be there when the simple method I called returns.

-Adrian

[David E. Jones-2]

On Sep 10, 2010, at 5:46 PM, Adrian Crum wrote:

> --- On Fri, 9/10/10, Adam Heath <[hidden email]> wrote:
>> On 09/10/2010 04:46 PM, Adrian Crum
>> wrote:
>>> I have a feeling that will break a lot of screen
>> widget/mini-language code.
>>
>> Removing implements Map, or fixing the implementation?
>>
>> I've seen code scattered around that checks if the Map is
>> an instance
>> of MapStack, and then does a push, falling back on wrapping
>> the map in
>> a new MapStack.  Such code shouldn't break
>
> Let's cross our fingers.
>
> I understand your viewpoint and I'm not arguing against it. If MapStack implements Map, then it should follow the Map contract - from a Java developer's viewpoint.
>
> My concern is for the OFBiz user who is writing a simple method, and they expect their changes to local variables to be local - not global. The OFBiz user is not aware of - or concerned with - a "Map contract."
>
> -Adrian

You may not want to argue against this point of view Adrian, but I'd be happy to.

Adam: sorry, but that interpretation of the Map interface for something like a MapStack is silly. The priority is how the MapStack should behave. I'd prefer to look at is in a conciliatory way that generally makes more sense. For example, sure the remove() is complete and a call to containsKey() would be expected to return false, UNLESS something else comes along and changes it first, and that's what the MapStack does.

-David