[
https://issues.apache.org/jira/browse/OFBIZ-5004?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14246209#comment-14246209
]
Scott Gray commented on OFBIZ-5004:
-----------------------------------
I'm not sure, a ClassCastException seems specifically for class casting issues
and I don't really think that fits here. If I wasn't aware of this discussion,
seeing a ClassCastException because a field I referenced didn't exist would
seem very strange.
I don't think we should be too concerned about throwing an
IllegalArgumentException, it doesn't need to be declared and would be less
surprising to see as an error given that it seems like the most appropriately
named exception available. Another interesting thing to keep in mind:
Javolution didn't allow null keys in its Maps and would throw a
NullPointerException if one was provided. I think our case is quite similar to
that one.
> 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: Trunk
> Reporter: Christoph Neuroth
> Assignee: Jacopo Cappellato
> Attachments: iae.patch
>
>
> 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 was sent by Atlassian JIRA
(v6.3.4#6332)
