RFR: 7148497: javax.management.MBeanAttributeInfo.hashCode throws NullPointerException

[Eamonn McManus]

I think there is a good case to be made for throwing an exception in
the constructor if the name or type is null. I guess that would
require a minor spec change, though.

Without such a change, the code can be simplified using Objects.equals
and Objects.hash. That assumes it doesn't need to be backported to
Java 6.

Éamonn



On 13 April 2012 07:43, Frederic Parain <frederic.parain at oracle.com> wrote:
> Karen,
>
> As far as I can tell, getDescriptor() never returns null.
>
> In the javax.management package, getDescriptor() calls
> ImmutableDescriptor.nonNullDescriptor(descriptor).clone()
> which returns a reference to a singleton empty Descriptor
> instance if the descriptor field is null.
>
> The getDescriptor() method is redefined in some classes of
> the javax.management.modelmbean package, but these implementations
> never return null either.
>
> So, unless I missed some code, it is safe to use getDescriptor()
> without null check.
>
> Fred
>
>
> On 4/13/12 3:54 PM, Karen Kinnear wrote:
>>
>> Frederic,
>>
>> Looks good. And I like the refactoring.
>>
>> A question - does the Descriptor field have the same potential for being
>> null? e.g.
>> in MBeanFeatureInfo.java new line 160 - do you need to check
>> getDescriptor() == null as well?
>> in MBeanConstructorinfo.java line 197, do you need the alternative null
>> checking?
>> MBeanAttributeInfo.java line 293?
>> (there may be other lines, I didn't do a thorough search)
>>
>> thanks,
>> Karen
>>
>>
>>
>> On Apr 13, 2012, at 9:01 AM, Staffan Larsen wrote:
>>
>>> This is much easier to read!
>>>
>>> In MBeanFeatureInfo.hasSameName() this check is not needed:
>>>
>>> 274         if(info.getName() == null) {
>>> 275            return false;
>>> 276         }
>>>
>>> since String.equals() with a null argument works as expected (return
>>> false). The same is true for the implementation in hasSameDescription().
>>>
>>> When calling hasSameName() and hasSameDescription() the "this." is not
>>> needed, but perhaps using it makes the intent clearer?
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>
>>>
>>>
>>> On 13 apr 2012, at 14:45, Frederic Parain wrote:
>>>
>>>> Thanks Staffan and Dmitry for your feedback,
>>>> here's a new webrev with a refactored code for
>>>> name and descriptions fields which are common to
>>>> most MBean*Info classes:
>>>>
>>>> http://cr.openjdk.java.net/~fparain/7148497/webrev.01/
>>>>
>>>> Thanks,
>>>>
>>>> Fred
>>>>
>>>> On 4/12/12 8:18 AM, Dmitry Samersoff wrote:
>>>>>
>>>>> Frederic,
>>>>>
>>>>> I'm second to Staffan.
>>>>>
>>>>> Looks good to me but would prefer to have it refactored, e.g. create a
>>>>> method equalOrNull();
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2012-04-10 18:33, Staffan Larsen wrote:
>>>>>>
>>>>>> Looks good! (Although I wish there was a better way to write code like
>>>>>> this).
>>>>>>
>>>>>> /Staffan
>>>>>>
>>>>>> On 10 apr 2012, at 15:50, Frederic Parain wrote:
>>>>>>
>>>>>>> Greetings,
>>>>>>>
>>>>>>> This is a request for review for CR
>>>>>>> 7148497: javax.management.MBeanAttributeInfo.hashCode throws
>>>>>>> NullPointerException
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7148497
>>>>>>>
>>>>>>> Even if the initial CR only refers to MBeanAttributeInfo.hashCode(),
>>>>>>> the problem is not limited to the MBeanAttributeInfo.hashCode(), but
>>>>>>> is common to all javax.management.MBean*Info classes:
>>>>>>> MBeanAttributeInfo, MBeanConstructorInfo, MBeanFeatureInfo,
>>>>>>> MBeanInfo, MBeanNotificationInfo, MBeanOperationInfo and
>>>>>>> MBeanParameterInfo.
>>>>>>>
>>>>>>> The root cause of the problem is that these classes can be
>>>>>>> instantiated with some null fields (mainly, but not limited to, the
>>>>>>> name and type fields), and these fields are dereferenced
>>>>>>> unconditionally in some methods. Unconditional dereferencing is used
>>>>>>> in the hashCode method but also in the equals() method.
>>>>>>>
>>>>>>> The proposed fix improves implementation of equals() and hashCode()
>>>>>>> method to handle cases where some fields are null without throwing
>>>>>>> a NPE.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~fparain/7148497/webrev.00/
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Fred
>>>>>>>
>>>>>>> --
>>>>>>> Frederic Parain - Oracle
>>>>>>> Grenoble Engineering Center - France
>>>>>>> Phone: +33 4 76 18 81 17
>>>>>>> Email: Frederic.Parain at oracle.com
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Frederic Parain - Oracle
>>>> Grenoble Engineering Center - France
>>>> Phone: +33 4 76 18 81 17
>>>> Email: Frederic.Parain at oracle.com
>>>>
>>>
>>
>
> --
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: Frederic.Parain at oracle.com
>