Copilot commented on code in PR #10296:
URL: https://github.com/apache/gravitino/pull/10296#discussion_r3008038814
##########
clients/cli/src/main/java/org/apache/gravitino/cli/ModelCommandHandler.java:
##########
@@ -310,6 +314,23 @@ private void handleRemoveCommand() {
}
}
+ /** Handles the "PROPERTIES" command. */
+ private void handlePropertiesCommand() {
+ if (line.hasOption(GravitinoOptions.ALIAS) ||
line.hasOption(GravitinoOptions.VERSION)) {
+ Integer version = getVersionFromLine(line);
+ String alias = getAliasFromLine(line);
+ gravitinoCommandLine
+ .newListModelVersionProperties(context, metalake, catalog, schema,
model, version, alias)
+ .validate()
+ .handle();
+ } else {
+ gravitinoCommandLine
+ .newListModelProperties(context, metalake, catalog, schema, model)
+ .validate()
+ .handle();
+ }
Review Comment:
`model_help.txt` (used by `gcli help model`) still lists `gcli model
[list|details|create|update|delete]` and doesn’t mention the newly-added
`properties` command (and the existing `set/remove` commands). Please update
`clients/cli/src/main/resources/model_help.txt` so the help output matches the
supported commands and includes an example for listing model/model-version
properties.
##########
clients/cli/src/test/java/org/apache/gravitino/cli/commands/TestListModelVersionProperties.java:
##########
@@ -0,0 +1,168 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.cli.commands;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.commons.cli.CommandLine;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.cli.CommandContext;
+import org.apache.gravitino.cli.Main;
+import org.apache.gravitino.client.GravitinoClient;
+import org.apache.gravitino.model.ModelCatalog;
+import org.apache.gravitino.model.ModelVersion;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+class TestListModelVersionProperties {
+
+ @BeforeEach
+ void setUp() {
+ Main.useExit = false;
+ }
+
+ @AfterEach
+ void tearDown() {
+ Main.useExit = true;
+ }
+
+ @Test
+ void handleByVersionPrintsProperties() throws Exception {
+ CommandLine mockCmdLine = Mockito.mock(CommandLine.class);
+ CommandContext context = new CommandContext(mockCmdLine);
+
+ ListModelVersionProperties command =
+ Mockito.spy(
+ new ListModelVersionProperties(
+ context, "metalake1", "catalog1", "schema1", "model1", 1,
null));
+
+ GravitinoClient mockClient = Mockito.mock(GravitinoClient.class);
+ Catalog mockCatalog = Mockito.mock(Catalog.class);
+ ModelCatalog mockModelCatalog = Mockito.mock(ModelCatalog.class);
+ ModelVersion mockModelVersion = Mockito.mock(ModelVersion.class);
+
+ Map<String, String> properties = new HashMap<>();
+ properties.put("vp1", "vval1");
+ properties.put("vp2", "vval2");
+
+ Mockito.doReturn(mockClient).when(command).buildClient("metalake1");
+ Mockito.when(mockClient.loadCatalog("catalog1")).thenReturn(mockCatalog);
+ Mockito.when(mockCatalog.asModelCatalog()).thenReturn(mockModelCatalog);
+
Mockito.when(mockModelCatalog.getModelVersion(Mockito.any(NameIdentifier.class),
Mockito.eq(1)))
+ .thenReturn(mockModelVersion);
+ Mockito.when(mockModelVersion.properties()).thenReturn(properties);
+
+ command.handle();
+
+
Mockito.verify(mockModelCatalog).getModelVersion(NameIdentifier.of("schema1",
"model1"), 1);
+ Mockito.verify(command).printProperties(properties);
+ }
+
+ @Test
+ void handleByAliasPrintsProperties() throws Exception {
+ CommandLine mockCmdLine = Mockito.mock(CommandLine.class);
+ CommandContext context = new CommandContext(mockCmdLine);
+
+ ListModelVersionProperties command =
+ Mockito.spy(
+ new ListModelVersionProperties(
+ context, "metalake1", "catalog1", "schema1", "model1", null,
"v1"));
+
+ GravitinoClient mockClient = Mockito.mock(GravitinoClient.class);
+ Catalog mockCatalog = Mockito.mock(Catalog.class);
+ ModelCatalog mockModelCatalog = Mockito.mock(ModelCatalog.class);
+ ModelVersion mockModelVersion = Mockito.mock(ModelVersion.class);
+
+ Map<String, String> properties = new HashMap<>();
+ properties.put("vp1", "vval1");
+
+ Mockito.doReturn(mockClient).when(command).buildClient("metalake1");
+ Mockito.when(mockClient.loadCatalog("catalog1")).thenReturn(mockCatalog);
+ Mockito.when(mockCatalog.asModelCatalog()).thenReturn(mockModelCatalog);
+ Mockito.when(
+
mockModelCatalog.getModelVersion(Mockito.any(NameIdentifier.class),
Mockito.eq("v1")))
+ .thenReturn(mockModelVersion);
+ Mockito.when(mockModelVersion.properties()).thenReturn(properties);
+
+ command.handle();
+
+
Mockito.verify(mockModelCatalog).getModelVersion(NameIdentifier.of("schema1",
"model1"), "v1");
+ Mockito.verify(command).printProperties(properties);
+ }
+
+ @Test
+ void handleWithBothNullVersionAndAliasShouldExitWithError() {
+ CommandLine mockCmdLine = Mockito.mock(CommandLine.class);
+ CommandContext context = new CommandContext(mockCmdLine);
+
+ ListModelVersionProperties command =
+ new ListModelVersionProperties(
+ context, "metalake1", "catalog1", "schema1", "model1", null, null);
+
+ RuntimeException ex = Assertions.assertThrows(RuntimeException.class,
command::handle);
+ Assertions.assertTrue(ex.getMessage().contains("Exit with code"));
+ }
Review Comment:
The new `validate()` path for the “both version and alias are null” case
isn’t directly tested. `handleWithBothNullVersionAndAliasShouldExitWithError()`
currently calls `handle()`, which can fail for unrelated reasons (e.g., null
passed into `getModelVersion`) and doesn’t assert the intended validation
message. Add a unit test that calls `validate()` with both `version` and
`alias` null and asserts the expected error text.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
