GenericEntity is a bad map

[Adam Heath-2]

GenericEntity.get(Object) does an immediate cast to String.  This is
*not allowed*.  The spec for java.util.Map says that *any* object can
be passed, and if it isn't contained in the map, to return null.  This
includes objects that don't match the generics signature(String in
this case).

Related to this, get(String) has a Debug.logWarning call, that says
it's printing an IllegalArgumentException when an invalid field is
passed.  This isn't so; no such exception actually *is* printed.
Plus, the map contract allows for unknown fields to be looked up,
without an error.

Related to this latter problem, is that remove() and containsKey()
don't do this unknown field warning.

I found these two problems, because I noticed the warning message in
my log, and wanted to fix my code, but then discovered that there
isn't really an exception.

[David E. Jones-2]

This is actually kind of annoying. The get(Object) method used to function quite differently from get(String). IMO it was nice that get(String) used to throw an exception (better immediate feedback for misspellings in code) while get(Object) logged a message in order to follow the Map semantics.

It looks like the was "refactored" and "improved" at some point. I hope it wasn't me...

-David


On Mar 16, 2010, at 2:55 PM, Adam Heath wrote:

> GenericEntity.get(Object) does an immediate cast to String.  This is
> *not allowed*.  The spec for java.util.Map says that *any* object can
> be passed, and if it isn't contained in the map, to return null.  This
> includes objects that don't match the generics signature(String in
> this case).
>
> Related to this, get(String) has a Debug.logWarning call, that says
> it's printing an IllegalArgumentException when an invalid field is
> passed.  This isn't so; no such exception actually *is* printed.
> Plus, the map contract allows for unknown fields to be looked up,
> without an error.
>
> Related to this latter problem, is that remove() and containsKey()
> don't do this unknown field warning.
>
> I found these two problems, because I noticed the warning message in
> my log, and wanted to fix my code, but then discovered that there
> isn't really an exception.

[Adam Heath-2]

David E Jones wrote:
> This is actually kind of annoying. The get(Object) method used to function quite differently from get(String). IMO it was nice that get(String) used to throw an exception (better immediate feedback for misspellings in code) while get(Object) logged a message in order to follow the Map semantics.
>
> It looks like the was "refactored" and "improved" at some point. I hope it wasn't me...
>
> -David

commit a0c5d5e6cc2ffa5fe603016a6382836fca3922ce
Author: lektran <lektran@13f79535-47bb-0310-9956-ffa450edef68>
Date:   Tue Oct 6 14:07:04 2009 +0000

    Fix an inconsistency in GenericEntity's get methods when the key
doesn't exist, get(String) throws an exception but get(Object) just
logs a warning.  Changed so that both methods just log a warning.

    git-svn-id: https://svn.eu.apache.org/repos/asf/ofbiz/trunk@822276
13f79535-47bb-0310-9956-ffa450edef68

Scott, what was your reasoning behind this?

> On Mar 16, 2010, at 2:55 PM, Adam Heath wrote:
>
>> GenericEntity.get(Object) does an immediate cast to String.  This is
>> *not allowed*.  The spec for java.util.Map says that *any* object can
>> be passed, and if it isn't contained in the map, to return null.  This
>> includes objects that don't match the generics signature(String in
>> this case).
>>
>> Related to this, get(String) has a Debug.logWarning call, that says
>> it's printing an IllegalArgumentException when an invalid field is
>> passed.  This isn't so; no such exception actually *is* printed.
>> Plus, the map contract allows for unknown fields to be looked up,
>> without an error.
>>
>> Related to this latter problem, is that remove() and containsKey()
>> don't do this unknown field warning.
>>
>> I found these two problems, because I noticed the warning message in
>> my log, and wanted to fix my code, but then discovered that there
>> isn't really an exception.
>

[Scott Gray-2]

It looks like it was me that changed it: http://svn.apache.org/viewvc?view=revision&revision=822276

I changed it because I couldn't see a reason for the inconsistency, now that I have the reason I can revert it (and add comments) if everyone agrees that it is the correct approach to take.  I can't remember what problem I encountered that prompted me to make the change in the first place, but I didn't just randomly look at the code and decide to change it.

Regards
Scott

On 16/03/2010, at 4:08 PM, David E Jones wrote:

>
> This is actually kind of annoying. The get(Object) method used to function quite differently from get(String). IMO it was nice that get(String) used to throw an exception (better immediate feedback for misspellings in code) while get(Object) logged a message in order to follow the Map semantics.
>
> It looks like the was "refactored" and "improved" at some point. I hope it wasn't me...
>
> -David
>
>
> On Mar 16, 2010, at 2:55 PM, Adam Heath wrote:
>
>> GenericEntity.get(Object) does an immediate cast to String.  This is
>> *not allowed*.  The spec for java.util.Map says that *any* object can
>> be passed, and if it isn't contained in the map, to return null.  This
>> includes objects that don't match the generics signature(String in
>> this case).
>>
>> Related to this, get(String) has a Debug.logWarning call, that says
>> it's printing an IllegalArgumentException when an invalid field is
>> passed.  This isn't so; no such exception actually *is* printed.
>> Plus, the map contract allows for unknown fields to be looked up,
>> without an error.
>>
>> Related to this latter problem, is that remove() and containsKey()
>> don't do this unknown field warning.
>>
>> I found these two problems, because I noticed the warning message in
>> my log, and wanted to fix my code, but then discovered that there
>> isn't really an exception.
>

[Scott Gray-2]

Looking at the diff again, I did also mess up and not print the exception when verbose logging was switched on.

Regards
Scott

On 16/03/2010, at 4:21 PM, Scott Gray wrote:

> It looks like it was me that changed it: http://svn.apache.org/viewvc?view=revision&revision=822276
>
> I changed it because I couldn't see a reason for the inconsistency, now that I have the reason I can revert it (and add comments) if everyone agrees that it is the correct approach to take.  I can't remember what problem I encountered that prompted me to make the change in the first place, but I didn't just randomly look at the code and decide to change it.
>
> Regards
> Scott
>
> On 16/03/2010, at 4:08 PM, David E Jones wrote:
>
>>
>> This is actually kind of annoying. The get(Object) method used to function quite differently from get(String). IMO it was nice that get(String) used to throw an exception (better immediate feedback for misspellings in code) while get(Object) logged a message in order to follow the Map semantics.
>>
>> It looks like the was "refactored" and "improved" at some point. I hope it wasn't me...
>>
>> -David
>>
>>
>> On Mar 16, 2010, at 2:55 PM, Adam Heath wrote:
>>
>>> GenericEntity.get(Object) does an immediate cast to String.  This is
>>> *not allowed*.  The spec for java.util.Map says that *any* object can
>>> be passed, and if it isn't contained in the map, to return null.  This
>>> includes objects that don't match the generics signature(String in
>>> this case).
>>>
>>> Related to this, get(String) has a Debug.logWarning call, that says
>>> it's printing an IllegalArgumentException when an invalid field is
>>> passed.  This isn't so; no such exception actually *is* printed.
>>> Plus, the map contract allows for unknown fields to be looked up,
>>> without an error.
>>>
>>> Related to this latter problem, is that remove() and containsKey()
>>> don't do this unknown field warning.
>>>
>>> I found these two problems, because I noticed the warning message in
>>> my log, and wanted to fix my code, but then discovered that there
>>> isn't really an exception.
>>
>