Issue #260: Fix IllegalArgumentException with GlobalVar taking a single argument #261
Conversation
Alex-Vol-SV
changed the title
When calling a Gloval Var declared to accept one argument and the caller passes null to the function it yields an IllegalArgumentException instead of passing the null argument. This is unique to single argument method calls and appears rooted in the way Groovy lang handles the invocation when null is provided. This fix was inspired from the code in the invoked Groovy methods.
Alex-Vol-SV
force-pushed
the
fix-illegal-argument-exception-with-one-null-argument
branch
from
7d4fa5c
to
58b1bf9
Compare
| { args -> | ||
| // When calling a one argument method with a null argument the | ||
| // Groovy doMethodInvoke appears to incorrectly assume a zero | ||
| // argument call signature for the method yielding an IllegalArgumentException | ||
| if (args == null && m.getNativeParameterTypes().size() == 1) { | ||
| m.doMethodInvoke(e.value, MetaClassHelper.ARRAY_WITH_NULL) | ||
| } else { | ||
| m.doMethodInvoke(e.value, args) | ||
| } | ||
| }) |
There was a problem hiding this comment.
| { args -> | |
| // When calling a one argument method with a null argument the | |
| // Groovy doMethodInvoke appears to incorrectly assume a zero | |
| // argument call signature for the method yielding an IllegalArgumentException | |
| if (args == null && m.getNativeParameterTypes().size() == 1) { | |
| m.doMethodInvoke(e.value, MetaClassHelper.ARRAY_WITH_NULL) | |
| } else { | |
| m.doMethodInvoke(e.value, args) | |
| } | |
| }) | |
| { args -> m.doMethodInvoke(e.value, args ?: MetaClassHelper.ARRAY_WITH_NULL) }) |
The Elvis operator is a bit more succinct here.
There was a problem hiding this comment.
I do not believe it is for all use cases. The additional check for number of parameters is needed. args may be null for a zero argument method call.
There was a problem hiding this comment.
Ah, I see. Shouldn't it be m.getNativeParameterTypes().size() > 0 then?
There was a problem hiding this comment.
In my opinion this code in Groovy is incorrectly handling the null case
invoked Groovy
The line in question should assign EMPTY_ARRAY to the null argumentArray and allow the subsequent lines decide if ARRAY_WITH_NULL is instead appropriate.
There was a problem hiding this comment.
No, more than one null arguments would have manifested as array of null. I can add additional tests for the behavior with multiple arguments but I believe the null single argument is an edge case not handled properly.
There was a problem hiding this comment.
Ok, I'm a bit unfamiliar with the background of the original failure here. I need to sit down and read through the linked issue before approving this PR. However, if another maintainer sees that everything is alright, I'm fine with them merging this (hence my leaving neutral feedback and not "requested changes").
Taking one line improvement Co-authored-by: Nik Reiman <[email protected]>
Alex-Vol-SV
deleted the
fix-illegal-argument-exception-with-one-null-argument
branch
When calling a GlovalVar declared to accept one argument and the caller
passes null to the function it yields an IllegalArgumentException
instead of passing the null argument. This is unique to single argument
method calls and appears rooted in the way Grrovy lang handles the
invocation when null is provided.
This fix was inspired from the code in the invoked Groovy methods.