WFCORE-5247 IllegalArgumentException and CLI crash with read-attribute in /system-property #4464
Conversation
parsharma
force-pushed
the
WFCORE-5247
branch
from
a2fa914
to
f8fb60e
Compare
github-actions
bot
added
the
deps-ok
|
@jfdenise Please review |
| addrNode.add(node.getType(), node.getName()); | ||
| for (OperationRequestAddress.Node node : address) { | ||
| if (!(node.getName() == null)) { | ||
| addrNode.add(node.getType(), node.getName()); |
There was a problem hiding this comment.
@parsharma , in this case you are replacing the current address (that is being built) by the empty address, so you loose the context (the various cd commands the user did). In order to keep the context, you could detect that the node.getName() returns null and in this case use the "*" value for the name to express the fact that the node name has not been set.
As a side note, it would be nice if you could add a test. It should be added in this file:
https://github.com/wildfly/wildfly-core/blob/master/testsuite/standalone/src/test/java/org/jboss/as/test/integration/management/cli/CliCompletionTestCase.java
parsharma
force-pushed
the
WFCORE-5247
branch
from
f8fb60e
to
71b912e
Compare
| for(OperationRequestAddress.Node node : address) { | ||
| addrNode.add(node.getType(), node.getName()); | ||
| for (OperationRequestAddress.Node node : address) { | ||
| if (!(node.getName() == null)) { |
There was a problem hiding this comment.
Thank-you for the change, I would suggest a tiny change to first deal with the null case if (node.getName() == null), it simplify a bit code reading.
| @@ -118,6 +118,36 @@ public void complexOperationWithObjectAsAttributeTest() { | |||
| assertEquals(candidates.toString(), Arrays.asList(","), candidates); | |||
| } | |||
| } | |||
| /** | |||
There was a problem hiding this comment.
Nice to add some tests. It would be great to cover the fix you are putting in place.
Something like:
CommandContext ctx = CLITestUtil.getCommandContext(TestSuiteEnvironment.getServerAddress(), TestSuiteEnvironment.getServerPort(), System.in, System.out);
ctx.connectController();
// cd into a type to validate that completion tha toccurs in this context is correct.
ctx.handle("cd /system-property");
try {
{
String cmd = "read-attribute ";
List candidates = new ArrayList<>();
ctx.getDefaultCommandCompleter().complete(ctx,
cmd, cmd.length(), candidates);
candidates = complete(ctx, cmd, null);
}
} finally {
ctx.terminateSession();
}
There was a problem hiding this comment.
Thanks for the suggestion @jfdenise. Please feel free to let me know if any further changes needed.
parsharma
force-pushed
the
WFCORE-5247
branch
from
71b912e
to
949fd0f
Compare
| import java.util.stream.Collectors; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
It looks like the order was wrong in the first place on this file, the static imports should be first - also can you please expand these back out instead of switching to a wildcard import?
…e in /system-property
parsharma
force-pushed
the
WFCORE-5247
branch
from
949fd0f
to
1aea910
Compare
Issue: https://issues.redhat.com/browse/WFCORE-5247