[jira] [Created] (OFBIZ-5004) Warning in GenericEntity.get unnecessarily clutters logs

[Nicolas Malin (Jira)]

Christoph Neuroth created OFBIZ-5004:
----------------------------------------

             Summary: Warning in GenericEntity.get unnecessarily clutters logs
                 Key: OFBIZ-5004
                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
             Project: OFBiz
          Issue Type: Bug
          Components: framework
    Affects Versions: SVN trunk
            Reporter: Christoph Neuroth


This code in GenericEntity.java:
{code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
{code}
is really annoying and IMHO it is wrong.

First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).

Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?

Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.

Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13432984#comment-13432984 ]

Adrian Crum commented on OFBIZ-5004:
------------------------------------

I agree that the log messages are annoying. From my perspective, the Map implementation should follow the Map API, and the other getXxx(String) methods should throw an exception - since those methods are specific to GenericEntity.

               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13433001#comment-13433001 ]

Adrian Crum commented on OFBIZ-5004:
------------------------------------

I modified my local copy to throw IllegalArgumentException for invalid field names passed to the getXxx(String) methods, and all tests pass without throwing the exception. So, the impact on trunk code should be minimal, but it could cause major headaches for user modifications.

               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13437506#comment-13437506 ]

Paul Foxworthy commented on OFBIZ-5004:
---------------------------------------

In principle, it is perfectly valid to store a value of null in a Map, so if you look up a key in a Map and receive a null, you don't know if that occurred because the key is not present, or if it *is* present but has a corresponding value of null.

Therefore, calling code really should use containsKey() to test if the key is present or not. It's the only way to be sure.

I would be perfectly happy if GenericEntity.get simply throws the exception. It would encourage us to use containsKey() where necessary.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13437668#comment-13437668 ]

Christoph Neuroth commented on OFBIZ-5004:
------------------------------------------

Differentiating between null (not present) and null (the value null present) looks like a smell to me. On the other hand, so is returning null. But I'm still convinced that when implementing the Map interface, the API should not be changed in regard to exceptions and return values. containsKey, after all is available to use if you really have the need to distinguish between those cases.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439005#comment-13439005 ]

Jacques Le Roux commented on OFBIZ-5004:
----------------------------------------

+1 to let this pop up and let developpers handle it
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439102#comment-13439102 ]

Adrian Crum commented on OFBIZ-5004:
------------------------------------

Jacques,

I don't understand. Are you saying we should continue to log pointless warnings that a perfectly legitimate Map method was invoked?

               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439112#comment-13439112 ]

Adrian Crum commented on OFBIZ-5004:
------------------------------------

To be absolutely clear, executing this code:

{code}
Object someValue = someMap.get("thisKeyDoesNotExist");
{code}

does not generate any warnings in any Map implementation EXCEPT GenericEntity.

And we are proposing that GenericEntity follow the Map convention.


               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439112#comment-13439112 ]

Adrian Crum edited comment on OFBIZ-5004 at 8/22/12 9:31 AM:
-------------------------------------------------------------

To be absolutely clear, executing this code:


{code}
Object someValue = someMap.get("thisKeyDoesNotExist");
{code}


does not generate any warnings in any Map implementation EXCEPT GenericEntity.

And we are proposing that GenericEntity follow the Map convention.


               
      was (Author: [hidden email]):
    To be absolutely clear, executing this code:

{code}
Object someValue = someMap.get("thisKeyDoesNotExist");
{code}

does not generate any warnings in any Map implementation EXCEPT GenericEntity.

And we are proposing that GenericEntity follow the Map convention.


                 

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439208#comment-13439208 ]

Scott Gray commented on OFBIZ-5004:
-----------------------------------

The warnings are not by any means pointless.

Reason to remove: "I'm dealing with an unknown entity and the warnings fill my logs"
Solution: Use GenericEntity.getEntityName() or (GenericEntity.getModelEntity().isField(String) *do not* use GenericEntity.containsKey(String) because GenericEntity can carry a partially populated value that will return false for valid fields.  Doing a set or a get without knowing that it is a valid operation for the given entity sounds like a terrible practice to me.

Reason to keep: Typos are easy to make and there's no build-time protection available, I've located issues in production numerous times because of this logging and that's exactly why it exists, to warn developers when they do something they shouldn't.  The only reason it doesn't actually throw the exception is because the Map spec doesn't allow it.

Please don't change this, the risks far outweigh the benefits here.

               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439300#comment-13439300 ]

Christoph Neuroth commented on OFBIZ-5004:
------------------------------------------

{quote}Typos are easy to make and there's no build-time protection available{quote}
Actually there is: Running your tests during the build.

{quote}The only reason it doesn't actually throw the exception is because the Map spec doesn't allow it.{quote}
Again, it doesn't have to: This is a perfectly valid case to throw an IllegalArgumentException which is a RuntimeException and as such doesn't have to be explicitly allowed by the interface.

Developers do not read logs during development. They read logs when things break in production.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439324#comment-13439324 ]

Jacques Le Roux commented on OFBIZ-5004:
----------------------------------------

Adrian,

Actually yes I was not clear. I was in the mood to remove these warnings from the logs using a mean or another. I find them disturbing, especially for developers not yet acquainted to it. On the other hand, I find Scott's arguments strong. Finally why taking the risk to remove them? For me a better solution would be to allow those annoyed by them to take the risk. So I propose a general property showGenericEntityWarning=false by default.

@Christoph: I read logs during development a lot ;)
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439329#comment-13439329 ]

Jacopo Cappellato commented on OFBIZ-5004:
------------------------------------------

I don't see the need for another parameter for this use case.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439413#comment-13439413 ]

Scott Gray commented on OFBIZ-5004:
-----------------------------------

Hey Christoph,

Interesting point on the IllegalArgumentException, I missed your comment initially and had always taken the code comment at face value without much thought.  I'd actually be in favor of throwing the exception when an invalid field name is passed in.

While tests are great they aren't the answer here, the reality is that we have the ability to build this protection into the framework and it should be taken advantage of considering the 99% use-case is that the developer expects the field they've passed in to exist on the entity.

For the record, I definitely look at the logs while developing.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439465#comment-13439465 ]

Jacques Le Roux commented on OFBIZ-5004:
----------------------------------------

+1 with Scott, missed that also. I mean test on field existence is ok for throwing exception with me, null value is not.
               

> Warning in GenericEntity.get unnecessarily clutters logs
> --------------------------------------------------------
>
>                 Key: OFBIZ-5004
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5004
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>
> This code in GenericEntity.java:
> {code}Debug.logWarning("The field name (or key) [" + name + "] is not valid for entity [" + this.getEntityName() + "], printing IllegalArgumentException instead of throwing it because Map interface specification does not allow throwing that exception.", module);
> {code}
> is really annoying and IMHO it is wrong.
> First, it does not print an exception, it only prints that string with no stacktrace (I think that was changed at some point).
> Second, IllegalArgument is a RuntimeException so the interface doesn't really need to allow it to be thrown, right?
> Personally, I think the warning is not even justified. We sometimes don't know exactly what kind of entity we're dealing with and just check if it has that field or not. With this code, to prevent excessive log clutter, we have to wrap each call with a "if (it.containsKey())". A java map will just return null silently as well, and this behavior is documented in the Map interface. If anyone is really worried about accessing fields wrong (your tests should catch that error), there could be an extra method "getOrThrow" or something, but get should just return the value or null.
> Otherwise, at least throw the exception. Log warnings are usually found while monitoring production logs, developers will find this much earlier otherwise.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira