RFR(XXS): 8156226: DiagnosticCommandImpl::invoke throws NoSuchMethodException even if the method actually exists but parameters are wrong

[David Holmes]

Hi Kirill,

On 7/05/2016 3:39 AM, Kirill Zhaldybin wrote:
> Alexander,
>
> Thank you for reviewing the fix.
>
> On 06.05.2016 20:24, Alexander Kulyakhtin wrote:
>> Is there a specification of what shall be thrown, or the justification
>> of the changes done?
>> One can think that the currently thrown NoSuchMethodException is
>> correct since　methods, which differ in signature only, are still
>> different methods.
> I spent some time trying to figure out why I got NoSuchMethodException
> for "help" despite such method actually existed.
>
> If you think that to throw NoSuchMethodException is better for wrong
> signature I could just change error message in this case.

The code doesn't really seem to be checking any detail of the 
"signature" as such, simply that the passed in "signature" array has a 
non-null zeroeth element of type String[]. Passing something else seems 
like a bad call to me and so IllegalArgumentException would be 
appropriate I think.

But the logic of that entire block is suspect to me as it does not 
handle the case where isEmpty() is true, but we pass in non-null params 
and signature anyway.

Aside: I'm not even convinced that NoSuchMethodException is appropriate 
for the case where "actionname" doesn't exist. But that is a different 
issue.

Thanks,
David


> Could you please let me know your opinion?
>
> Thank you.
>
> Regards, Kirill
>
>>
>> Best regards,
>> Alexander
>>
>>
>> ----- Original Message -----
>> From: kirill.zhaldybin at oracle.com
>> To: serviceability-dev at openjdk.java.net
>> Sent: Friday, May 6, 2016 8:11:08 PM GMT +03:00 Iraq
>> Subject: RFR(XXS): 8156226: DiagnosticCommandImpl::invoke throws
>> NoSuchMethodException even if the method actually exists but
>> parameters are wrong
>>
>> Dear all,
>>
>> Could you please review this small fix for 8156226?
>>
>> A case when a method exists but parameters' signature is wrong now
>> causes new ReflectionException(new IllegalArgumentException()) thrown
>> instead of new ReflectionException(new NoSuchMethodException()).
>>
>> WebRev:
>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8156226/webrev.00/
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8156226
>>
>> Thank you.
>>
>> Regards, Kirill
>>
>