fix(java): picocli ParameterException for browse --depth; warn on --parent 0
Replaces the raw IllegalArgumentException thrown by GalaxyBrowseCommand for --depth < 0 with a CommandLine.ParameterException so picocli surfaces a clean single-line error instead of an unhandled stack trace. Adds an upper bound of 50 (matching the Python client) so --depth > 50 is also rejected cleanly. Emits a stderr warning when --parent 0 is supplied explicitly, matching Go/Rust client behaviour, because gobject id 0 is the server's root-walk sentinel and passing it via --parent is almost always a mistake. Adds three new tests: negative depth, depth > 50, and the --parent 0 warning path.
This commit is contained in:
+17
-3
@@ -458,16 +458,24 @@ public final class MxGatewayCli implements Callable<Integer> {
|
|||||||
name = "galaxy-browse",
|
name = "galaxy-browse",
|
||||||
description = "Browses the Galaxy hierarchy via GalaxyRepository.BrowseChildren.")
|
description = "Browses the Galaxy hierarchy via GalaxyRepository.BrowseChildren.")
|
||||||
static final class GalaxyBrowseCommand extends GalaxyCommand {
|
static final class GalaxyBrowseCommand extends GalaxyCommand {
|
||||||
|
@Spec
|
||||||
|
private CommandSpec spec;
|
||||||
|
|
||||||
@Option(
|
@Option(
|
||||||
names = "--parent",
|
names = "--parent",
|
||||||
defaultValue = "-1",
|
defaultValue = "-1",
|
||||||
description = "Parent gobject id to browse one level of children for; omit to walk the roots.")
|
description =
|
||||||
|
"Parent gobject id to browse one level of children for."
|
||||||
|
+ " Use the default (omit) to walk root nodes;"
|
||||||
|
+ " gobject id 0 is reserved by the server to mean roots.")
|
||||||
int parent;
|
int parent;
|
||||||
|
|
||||||
@Option(
|
@Option(
|
||||||
names = "--depth",
|
names = "--depth",
|
||||||
defaultValue = "0",
|
defaultValue = "0",
|
||||||
description = "When walking roots, eagerly expand this many further levels before printing.")
|
description =
|
||||||
|
"When walking roots, eagerly expand this many further levels before printing."
|
||||||
|
+ " Must be between 0 and 50 inclusive.")
|
||||||
int depth;
|
int depth;
|
||||||
|
|
||||||
@Option(names = "--category-ids", description = "Comma-separated category ids to include.")
|
@Option(names = "--category-ids", description = "Comma-separated category ids to include.")
|
||||||
@@ -491,11 +499,17 @@ public final class MxGatewayCli implements Callable<Integer> {
|
|||||||
@Override
|
@Override
|
||||||
public Integer call() {
|
public Integer call() {
|
||||||
if (depth < 0) {
|
if (depth < 0) {
|
||||||
throw new IllegalArgumentException("--depth must be non-negative");
|
throw new CommandLine.ParameterException(spec.commandLine(), "--depth must be non-negative");
|
||||||
|
}
|
||||||
|
if (depth > 50) {
|
||||||
|
throw new CommandLine.ParameterException(spec.commandLine(), "--depth must be at most 50");
|
||||||
}
|
}
|
||||||
BrowseChildrenOptions options = buildOptions();
|
BrowseChildrenOptions options = buildOptions();
|
||||||
PrintWriter out = common.spec.commandLine().getOut();
|
PrintWriter out = common.spec.commandLine().getOut();
|
||||||
PrintWriter err = common.spec.commandLine().getErr();
|
PrintWriter err = common.spec.commandLine().getErr();
|
||||||
|
if (parent == 0) {
|
||||||
|
err.println("warning: --parent 0 is the server sentinel for root nodes; omit --parent to walk roots instead.");
|
||||||
|
}
|
||||||
try (GalaxyRepositoryClient client = connect()) {
|
try (GalaxyRepositoryClient client = connect()) {
|
||||||
if (parent >= 0) {
|
if (parent >= 0) {
|
||||||
if (depth > 0) {
|
if (depth > 0) {
|
||||||
|
|||||||
+47
@@ -215,6 +215,53 @@ final class MxGatewayCliTests {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void galaxyBrowseNegativeDepthYieldsNonZeroExitViaParameterException() {
|
||||||
|
// Fix: --depth validation must surface as a picocli ParameterException
|
||||||
|
// (clean error line on stderr) rather than an unhandled IllegalArgumentException
|
||||||
|
// stack trace. Picocli maps ParameterException to exit code 2.
|
||||||
|
CliRun run = execute(new FakeClientFactory(), "galaxy-browse", "--depth", "-1");
|
||||||
|
|
||||||
|
assertFalse(run.exitCode() == 0, "expected non-zero exit for --depth -1");
|
||||||
|
// Picocli writes ParameterException messages to the error writer.
|
||||||
|
assertTrue(run.errors().contains("--depth"), "expected --depth in error output: " + run.errors());
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void galaxyBrowseDepthAbove50YieldsNonZeroExit() {
|
||||||
|
CliRun run = execute(new FakeClientFactory(), "galaxy-browse", "--depth", "51");
|
||||||
|
|
||||||
|
assertFalse(run.exitCode() == 0, "expected non-zero exit for --depth 51");
|
||||||
|
assertTrue(run.errors().contains("--depth"), "expected --depth in error output: " + run.errors());
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void galaxyBrowseParentZeroEmitsWarningToStderr() {
|
||||||
|
// --parent 0 is the server sentinel for roots; passing it explicitly is
|
||||||
|
// almost certainly a mistake. The CLI must print a warning to stderr
|
||||||
|
// (matching Go/Rust client behaviour) but must still attempt the call
|
||||||
|
// (exit behaviour depends on gateway reachability, not tested here;
|
||||||
|
// we only assert the warning path is triggered by checking the error
|
||||||
|
// writer before any gRPC connection is attempted).
|
||||||
|
//
|
||||||
|
// GalaxyBrowseCommand connects to a real GalaxyRepositoryClient, so the
|
||||||
|
// call() body will throw after printing the warning when no gateway is
|
||||||
|
// reachable. We only assert the warning appears on stderr.
|
||||||
|
StringWriter output = new StringWriter();
|
||||||
|
StringWriter errors = new StringWriter();
|
||||||
|
// Non-zero exit is expected (no live gateway), but the warning must
|
||||||
|
// appear on stderr regardless of what happens next.
|
||||||
|
MxGatewayCli.execute(
|
||||||
|
new FakeClientFactory(),
|
||||||
|
new PrintWriter(output, true),
|
||||||
|
new PrintWriter(errors, true),
|
||||||
|
"galaxy-browse", "--parent", "0", "--depth", "1");
|
||||||
|
|
||||||
|
assertTrue(
|
||||||
|
errors.toString().contains("--parent 0"),
|
||||||
|
"expected '--parent 0' warning on stderr; got: " + errors);
|
||||||
|
}
|
||||||
|
|
||||||
// ---- galaxy command-name aliases (D9-java) ----
|
// ---- galaxy command-name aliases (D9-java) ----
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|||||||
@@ -156,7 +156,7 @@ public sealed class GatewayEndToEndFakeWorkerSmokeTests
|
|||||||
Assert.Equal(ProtocolStatusCode.Ok, infoReply.ProtocolStatus.Code);
|
Assert.Equal(ProtocolStatusCode.Ok, infoReply.ProtocolStatus.Code);
|
||||||
Assert.Equal(MxCommandKind.GetWorkerInfo, infoReply.Kind);
|
Assert.Equal(MxCommandKind.GetWorkerInfo, infoReply.Kind);
|
||||||
Assert.NotNull(infoReply.WorkerInfo);
|
Assert.NotNull(infoReply.WorkerInfo);
|
||||||
Assert.Equal(ControlCommandFakeWorkerProcessLauncher.ProcessId, infoReply.WorkerInfo.WorkerProcessId);
|
Assert.Equal(FakeWorkerHarness.DefaultWorkerProcessId, infoReply.WorkerInfo.WorkerProcessId);
|
||||||
Assert.False(string.IsNullOrEmpty(infoReply.WorkerInfo.MxaccessProgid));
|
Assert.False(string.IsNullOrEmpty(infoReply.WorkerInfo.MxaccessProgid));
|
||||||
|
|
||||||
// DrainEvents — the scripted worker returns an empty drain reply.
|
// DrainEvents — the scripted worker returns an empty drain reply.
|
||||||
@@ -522,9 +522,7 @@ public sealed class GatewayEndToEndFakeWorkerSmokeTests
|
|||||||
or MxCommandKind.GetWorkerInfo or MxCommandKind.DrainEvents
|
or MxCommandKind.GetWorkerInfo or MxCommandKind.DrainEvents
|
||||||
or MxCommandKind.ShutdownWorker)
|
or MxCommandKind.ShutdownWorker)
|
||||||
{
|
{
|
||||||
// Re-enter the harness to process the already-read envelope
|
await harness.RespondToControlCommandAsync(envelope, cancellationToken)
|
||||||
// by replaying it through the control-command responder path.
|
|
||||||
await RespondToKnownControlCommandAsync(harness, envelope, cancellationToken)
|
|
||||||
.ConfigureAwait(false);
|
.ConfigureAwait(false);
|
||||||
_commandHandled.Release();
|
_commandHandled.Release();
|
||||||
continue;
|
continue;
|
||||||
@@ -535,70 +533,6 @@ public sealed class GatewayEndToEndFakeWorkerSmokeTests
|
|||||||
$"ControlCommandFakeWorkerProcessLauncher received unexpected envelope {envelope.BodyCase}.");
|
$"ControlCommandFakeWorkerProcessLauncher received unexpected envelope {envelope.BodyCase}.");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private static async Task RespondToKnownControlCommandAsync(
|
|
||||||
FakeWorkerHarness harness,
|
|
||||||
WorkerEnvelope commandEnvelope,
|
|
||||||
CancellationToken cancellationToken)
|
|
||||||
{
|
|
||||||
MxCommand command = commandEnvelope.WorkerCommand.Command;
|
|
||||||
switch (command.Kind)
|
|
||||||
{
|
|
||||||
case MxCommandKind.Ping:
|
|
||||||
await harness.ReplyToCommandAsync(
|
|
||||||
commandEnvelope,
|
|
||||||
configureReply: reply =>
|
|
||||||
{
|
|
||||||
string? message = command.Ping?.Message;
|
|
||||||
if (!string.IsNullOrEmpty(message))
|
|
||||||
{
|
|
||||||
reply.DiagnosticMessage = message;
|
|
||||||
}
|
|
||||||
},
|
|
||||||
cancellationToken: cancellationToken).ConfigureAwait(false);
|
|
||||||
break;
|
|
||||||
|
|
||||||
case MxCommandKind.GetSessionState:
|
|
||||||
await harness.ReplyToCommandAsync(
|
|
||||||
commandEnvelope,
|
|
||||||
configureReply: reply => reply.SessionState = new SessionStateReply
|
|
||||||
{
|
|
||||||
State = SessionState.Ready,
|
|
||||||
},
|
|
||||||
cancellationToken: cancellationToken).ConfigureAwait(false);
|
|
||||||
break;
|
|
||||||
|
|
||||||
case MxCommandKind.GetWorkerInfo:
|
|
||||||
await harness.ReplyToCommandAsync(
|
|
||||||
commandEnvelope,
|
|
||||||
configureReply: reply => reply.WorkerInfo = new WorkerInfoReply
|
|
||||||
{
|
|
||||||
WorkerProcessId = ControlCommandFakeWorkerProcessLauncher.ProcessId,
|
|
||||||
WorkerVersion = "fake-control-worker",
|
|
||||||
MxaccessProgid = "LMXProxy.LMXProxyServer.1",
|
|
||||||
MxaccessClsid = "{C30B52F5-2CB5-4760-AF0A-3A344A7EB5DC}",
|
|
||||||
},
|
|
||||||
cancellationToken: cancellationToken).ConfigureAwait(false);
|
|
||||||
break;
|
|
||||||
|
|
||||||
case MxCommandKind.DrainEvents:
|
|
||||||
await harness.ReplyToCommandAsync(
|
|
||||||
commandEnvelope,
|
|
||||||
configureReply: reply => reply.DrainEvents = new DrainEventsReply(),
|
|
||||||
cancellationToken: cancellationToken).ConfigureAwait(false);
|
|
||||||
break;
|
|
||||||
|
|
||||||
case MxCommandKind.ShutdownWorker:
|
|
||||||
await harness.ReplyToCommandAsync(commandEnvelope, cancellationToken: cancellationToken)
|
|
||||||
.ConfigureAwait(false);
|
|
||||||
await harness.SendShutdownAckAsync(cancellationToken: cancellationToken).ConfigureAwait(false);
|
|
||||||
break;
|
|
||||||
|
|
||||||
default:
|
|
||||||
throw new InvalidOperationException(
|
|
||||||
$"Unexpected control command kind {command.Kind} in ControlCommandFakeWorkerProcessLauncher.");
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private sealed class FakeWorkerProcess(int processId) : IWorkerProcess
|
private sealed class FakeWorkerProcess(int processId) : IWorkerProcess
|
||||||
|
|||||||
@@ -207,7 +207,7 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
CreateCommand(MxCommandKind.Ping, cmd => cmd.Ping = new PingCommand { Message = "hello-ping" }),
|
CreateCommand(MxCommandKind.Ping, cmd => cmd.Ping = new PingCommand { Message = "hello-ping" }),
|
||||||
TestTimeout,
|
TestTimeout,
|
||||||
CancellationToken.None);
|
CancellationToken.None);
|
||||||
await fakeWorker.RespondToControlCommandAsync();
|
await fakeWorker.RespondToControlCommandAsync().WaitAsync(TestTimeout);
|
||||||
|
|
||||||
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
||||||
|
|
||||||
@@ -231,7 +231,7 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
CreateCommand(MxCommandKind.GetSessionState),
|
CreateCommand(MxCommandKind.GetSessionState),
|
||||||
TestTimeout,
|
TestTimeout,
|
||||||
CancellationToken.None);
|
CancellationToken.None);
|
||||||
await fakeWorker.RespondToControlCommandAsync();
|
await fakeWorker.RespondToControlCommandAsync().WaitAsync(TestTimeout);
|
||||||
|
|
||||||
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
||||||
|
|
||||||
@@ -256,7 +256,7 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
CreateCommand(MxCommandKind.GetWorkerInfo),
|
CreateCommand(MxCommandKind.GetWorkerInfo),
|
||||||
TestTimeout,
|
TestTimeout,
|
||||||
CancellationToken.None);
|
CancellationToken.None);
|
||||||
await fakeWorker.RespondToControlCommandAsync();
|
await fakeWorker.RespondToControlCommandAsync().WaitAsync(TestTimeout);
|
||||||
|
|
||||||
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
||||||
|
|
||||||
@@ -265,7 +265,8 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
Assert.NotNull(reply.Reply.WorkerInfo);
|
Assert.NotNull(reply.Reply.WorkerInfo);
|
||||||
Assert.Equal(FakeWorkerHarness.DefaultWorkerProcessId, reply.Reply.WorkerInfo.WorkerProcessId);
|
Assert.Equal(FakeWorkerHarness.DefaultWorkerProcessId, reply.Reply.WorkerInfo.WorkerProcessId);
|
||||||
Assert.Equal("LMXProxy.LMXProxyServer.1", reply.Reply.WorkerInfo.MxaccessProgid);
|
Assert.Equal("LMXProxy.LMXProxyServer.1", reply.Reply.WorkerInfo.MxaccessProgid);
|
||||||
Assert.False(string.IsNullOrEmpty(reply.Reply.WorkerInfo.MxaccessClsid));
|
Assert.Equal("{C30B52F5-2CB5-4760-AF0A-3A344A7EB5DC}", reply.Reply.WorkerInfo.MxaccessClsid);
|
||||||
|
Assert.Equal("fake-worker", reply.Reply.WorkerInfo.WorkerVersion);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
@@ -283,7 +284,7 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
CreateCommand(MxCommandKind.DrainEvents, cmd => cmd.DrainEvents = new DrainEventsCommand { MaxEvents = 32 }),
|
CreateCommand(MxCommandKind.DrainEvents, cmd => cmd.DrainEvents = new DrainEventsCommand { MaxEvents = 32 }),
|
||||||
TestTimeout,
|
TestTimeout,
|
||||||
CancellationToken.None);
|
CancellationToken.None);
|
||||||
await fakeWorker.RespondToControlCommandAsync();
|
await fakeWorker.RespondToControlCommandAsync().WaitAsync(TestTimeout);
|
||||||
|
|
||||||
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
||||||
|
|
||||||
@@ -314,7 +315,7 @@ public sealed class FakeWorkerHarnessTests
|
|||||||
// The harness reads the ShutdownWorker WorkerCommand and replies with
|
// The harness reads the ShutdownWorker WorkerCommand and replies with
|
||||||
// OK + ShutdownAck — the WorkerClient's read loop processes the ack and
|
// OK + ShutdownAck — the WorkerClient's read loop processes the ack and
|
||||||
// transitions to Closed.
|
// transitions to Closed.
|
||||||
await fakeWorker.RespondToControlCommandAsync();
|
await fakeWorker.RespondToControlCommandAsync().WaitAsync(TestTimeout);
|
||||||
|
|
||||||
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
WorkerCommandReply reply = await invokeTask.WaitAsync(TestTimeout);
|
||||||
|
|
||||||
|
|||||||
@@ -409,6 +409,37 @@ public sealed class FakeWorkerHarness : IAsyncDisposable
|
|||||||
CancellationToken cancellationToken = default)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
WorkerEnvelope commandEnvelope = await ReadCommandAsync(cancellationToken).ConfigureAwait(false);
|
WorkerEnvelope commandEnvelope = await ReadCommandAsync(cancellationToken).ConfigureAwait(false);
|
||||||
|
return await RespondToControlCommandAsync(commandEnvelope, cancellationToken).ConfigureAwait(false);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Accepts an already-read command envelope and, if it is one of the five control
|
||||||
|
/// command kinds (Ping, GetSessionState, GetWorkerInfo, DrainEvents, ShutdownWorker),
|
||||||
|
/// writes a canned reply that mirrors the real worker's reply shape. For ShutdownWorker
|
||||||
|
/// the method additionally sends a <see cref="WorkerShutdownAck"/> after the OK reply.
|
||||||
|
/// Use this overload when the caller has already consumed the envelope from the pipe
|
||||||
|
/// (e.g., to inspect the kind before routing) to avoid re-reading.
|
||||||
|
/// </summary>
|
||||||
|
/// <param name="commandEnvelope">The already-read command envelope to respond to.</param>
|
||||||
|
/// <param name="cancellationToken">Token to cancel the asynchronous operation.</param>
|
||||||
|
/// <returns>The command envelope that was handled.</returns>
|
||||||
|
/// <exception cref="ArgumentException">
|
||||||
|
/// Thrown when <paramref name="commandEnvelope"/> does not contain a <c>WorkerCommand</c>.
|
||||||
|
/// </exception>
|
||||||
|
/// <exception cref="InvalidOperationException">
|
||||||
|
/// Thrown when the command kind is not one of the five control command kinds.
|
||||||
|
/// </exception>
|
||||||
|
public async Task<WorkerEnvelope> RespondToControlCommandAsync(
|
||||||
|
WorkerEnvelope commandEnvelope,
|
||||||
|
CancellationToken cancellationToken = default)
|
||||||
|
{
|
||||||
|
if (commandEnvelope.BodyCase != WorkerEnvelope.BodyOneofCase.WorkerCommand)
|
||||||
|
{
|
||||||
|
throw new ArgumentException(
|
||||||
|
$"Expected WorkerCommand envelope but received {commandEnvelope.BodyCase}.",
|
||||||
|
nameof(commandEnvelope));
|
||||||
|
}
|
||||||
|
|
||||||
MxCommand command = commandEnvelope.WorkerCommand.Command;
|
MxCommand command = commandEnvelope.WorkerCommand.Command;
|
||||||
|
|
||||||
switch (command.Kind)
|
switch (command.Kind)
|
||||||
|
|||||||
Reference in New Issue
Block a user