Fixes #4550 XmlConfiguration named parameters#4553
Conversation
Fixed #4550 named parameters with a moderate refactor. The named parameter matching was duplicated, only considering number of args and not applied to call arguments. This refactor puts all the behaviour in common methods and reorders the arguments to match parameters. Signed-off-by: Greg Wilkins <gregw@webtide.com>
Do not match a missing varArgs executable is there is a matching method with no varArgs. Signed-off-by: Greg Wilkins <gregw@webtide.com>
| Objects.requireNonNull(namedArgMap, "Named Argument Map cannot be null"); | ||
| Objects.requireNonNull(args, "Named list cannot be null"); | ||
|
|
||
| constructors: |
| if (executable instanceof Constructor) | ||
| _class.getConstructor(types); | ||
| else | ||
| _class.getMethod(((Method)executable).getName(), types); |
| // Does a non varargs method exist? | ||
| Class<?>[] types = Arrays.copyOf(executable.getParameterTypes(), count - 1); | ||
| if (executable instanceof Constructor) | ||
| _class.getConstructor(types); |
There was a problem hiding this comment.
This branch does not appear to be covered by tests, needs a varargs constructor.
| _class.getConstructor(types); | ||
| else | ||
| _class.getMethod(((Method)executable).getName(), types); | ||
| } |
There was a problem hiding this comment.
IIUC, the logic of this try block is that if a varargs member is probed before a non-varargs member, we want to skip the varargs one to match the non-varargs one.
I think it's way better to sort the members putting varargs methods last, and among varargs those with less parameters last.
In this way, we are guaranteed that precise match always comes before varargs match and we don't need this block. Thoughts?
There was a problem hiding this comment.
I think I have implemented it... feels a little fragile that applyTo depends on it's calling order, but OK
| } | ||
| catch (NoSuchMethodException e) | ||
| { | ||
| // There is not a no varArgs alternative so let's try a null varArgs match |
There was a problem hiding this comment.
The comment is slightly confusing, since it's not a null varArgs match, it's an empty varArgs match. Indeed the method asEmptyVarArgs() is named correctly and does correctly return an empty array.
| { | ||
| final Class<?> _class; | ||
| final List<Object> _arguments; | ||
| final List<String> _names; |
There was a problem hiding this comment.
sure... but you do know that package scope fields of a private inner class are already really private :)
| } | ||
| } | ||
|
|
||
| private NamedArgs(List<Object> arguments, List<String> names) |
There was a problem hiding this comment.
Since all other constructors/methods are package private, either make them all private, or all package private.
|
|
||
| // No match of wrong number of parameters | ||
| if (count != _arguments.size()) | ||
| return null; |
There was a problem hiding this comment.
This does not cover the case of executable(String first, String... others) with args=["one", "two", "three"], where count==2 but _arguments.size()==3.
Not sure it was covered before, but while we are at it...
There was a problem hiding this comment.
I don't think we can support that, at least not easily. To call executable(String first, String... others) with args=["one", "two", "three"] you will need to do:
<Call name=executable>
<Arg>one</Arg>
<Arg>
<Array type=String>
<Item>two</Item>
<Item>three</Item>
</Array>
</Arg>
</Call>|
|
||
| String attr = _node.getAttribute(attrName); | ||
| if (attr != null) | ||
| private class NamedArgs |
There was a problem hiding this comment.
Rename to just Args since it does both named and non-named arguments?
Signed-off-by: Greg Wilkins <gregw@webtide.com>
…pty var args. Signed-off-by: Greg Wilkins <gregw@webtide.com>
sbordet
left a comment
There was a problem hiding this comment.
Other than the 2 nits, LGTM.
jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Fixed #4550 named parameters with a moderate refactor.
The named parameter matching was duplicated, only considering number of args and not applied to call arguments. This refactor puts all the behaviour in common methods and reorders the arguments to match parameters.
Signed-off-by: Greg Wilkins gregw@webtide.com