From bb8dd4fddee8390043e4133c767165d0baa1050b Mon Sep 17 00:00:00 2001 From: Ville Puuska <40150442+VillePuuska@users.noreply.github.com> Date: Thu, 18 Jul 2024 20:23:54 +0300 Subject: [PATCH] update volume: changing new_name to be optional and making empty updates change nothing (#254) **PR Checklist** - [x] A description of the changes is added to the description of this PR. - [x] If there is a related issue, make sure it is linked to this PR. - [x] If you've fixed a bug or added code that should be tested, add tests! - [x] If you've added or modified a feature, documentation in `docs` is updated **Description of changes** Making `update volume` work similarly to how `update catalog` was changed to work in PR #158 and `update schema`was changed to work in PR #219 Closes #110 Two main changes: - Changed the parameter new_name to be optional in the server and CLI. - Changed both the CLI and `VolumeRepository.updateVolume` to not update anything if no parameters are provided to update. - CLI just prints a help message listing all optional parameters. - `VolumeRepository.updateVolume` just returns existing `VolumeInfo`. --- docs/usage/cli.md | 4 ++- .../java/io/unitycatalog/cli/VolumeCli.java | 13 ++++++++-- .../io/unitycatalog/cli/utils/CliUtils.java | 7 ++--- .../cli/volume/CliVolumeOperations.java | 23 ++++++++-------- .../server/persist/VolumeRepository.java | 8 +++++- .../base/volume/BaseVolumeCRUDTest.java | 26 ++++++++++++++++--- 6 files changed, 58 insertions(+), 23 deletions(-) diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 35ebdde97..6e9328d6d 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -251,9 +251,11 @@ bin/uc volume update --full_name .. --new_name optionalParams = + CliUtils.cliOptions.get(CliUtils.VOLUME).get(CliUtils.UPDATE).getOptionalParams(); + String errorMessage = "No parameters to update, please provide one of:"; + for (CliParams param : optionalParams) { + errorMessage += "\n --" + param.val(); + } + throw new CliException(errorMessage); + } + UpdateVolumeRequestContent updateVolumeRequest = + objectMapper.readValue(json.toString(), UpdateVolumeRequestContent.class); return objectWriter.writeValueAsString( apiClient.updateVolume(volumeFullName, updateVolumeRequest)); } diff --git a/examples/cli/src/main/java/io/unitycatalog/cli/utils/CliUtils.java b/examples/cli/src/main/java/io/unitycatalog/cli/utils/CliUtils.java index 3e6cf009e..d6157d6b7 100644 --- a/examples/cli/src/main/java/io/unitycatalog/cli/utils/CliUtils.java +++ b/examples/cli/src/main/java/io/unitycatalog/cli/utils/CliUtils.java @@ -125,8 +125,8 @@ public List getOptionalParams() { put( UPDATE, new CliOptions( - List.of(CliParams.FULL_NAME, CliParams.NEW_NAME), - List.of(CliParams.COMMENT))); + List.of(CliParams.FULL_NAME), + List.of(CliParams.COMMENT, CliParams.NEW_NAME))); put(DELETE, new CliOptions(List.of(CliParams.FULL_NAME), List.of())); put(READ, new CliOptions(List.of(CliParams.FULL_NAME), List.of(CliParams.PATH))); put(WRITE, new CliOptions(List.of(CliParams.FULL_NAME), List.of(CliParams.PATH))); @@ -453,7 +453,8 @@ public static void printHelp() { "By default, the client will connect to UC running locally at http://localhost:8080\n"); System.out.println("To connect to specific UC server, use --server https://\n"); System.out.println( - "Currently, auth using bearer token is supported. Please specify the token via --auth_token \n"); + "Currently, auth using bearer token is supported. Please specify the token via --auth_token" + + " \n"); System.out.println( "For detailed help on entity specific operations, use bin/uc --help"); } diff --git a/examples/cli/src/test/java/io/unitycatalog/cli/volume/CliVolumeOperations.java b/examples/cli/src/test/java/io/unitycatalog/cli/volume/CliVolumeOperations.java index c6f093007..5974869d2 100644 --- a/examples/cli/src/test/java/io/unitycatalog/cli/volume/CliVolumeOperations.java +++ b/examples/cli/src/test/java/io/unitycatalog/cli/volume/CliVolumeOperations.java @@ -67,18 +67,17 @@ public VolumeInfo getVolume(String volumeFullName) { @Override public VolumeInfo updateVolume( String volumeFullName, UpdateVolumeRequestContent updateVolumeRequest) { - String[] args = - addServerAndAuthParams( - List.of( - "volume", - "update", - "--full_name", - volumeFullName, - "--new_name", - updateVolumeRequest.getNewName(), - "--comment", - updateVolumeRequest.getComment()), - config); + List argsList = + new ArrayList<>(List.of("volume", "update", "--full_name", volumeFullName)); + if (updateVolumeRequest.getNewName() != null) { + argsList.add("--new_name"); + argsList.add(updateVolumeRequest.getNewName()); + } + if (updateVolumeRequest.getComment() != null) { + argsList.add("--comment"); + argsList.add(updateVolumeRequest.getComment()); + } + String[] args = addServerAndAuthParams(argsList, config); JsonNode updatedVolumeInfo = executeCLICommand(args); return objectMapper.convertValue(updatedVolumeInfo, VolumeInfo.class); } diff --git a/server/src/main/java/io/unitycatalog/server/persist/VolumeRepository.java b/server/src/main/java/io/unitycatalog/server/persist/VolumeRepository.java index 97d907d72..7f1f7b7bd 100644 --- a/server/src/main/java/io/unitycatalog/server/persist/VolumeRepository.java +++ b/server/src/main/java/io/unitycatalog/server/persist/VolumeRepository.java @@ -213,7 +213,9 @@ private VolumeInfo convertFromDAO( } public VolumeInfo updateVolume(String name, UpdateVolumeRequestContent updateVolumeRequest) { - ValidationUtils.validateSqlObjectName(updateVolumeRequest.getNewName()); + if (updateVolumeRequest.getNewName() != null) { + ValidationUtils.validateSqlObjectName(updateVolumeRequest.getNewName()); + } String[] namespace = name.split("\\."); String catalog = namespace[0], schema = namespace[1], volume = namespace[2]; try (Session session = SESSION_FACTORY.openSession()) { @@ -232,6 +234,10 @@ public VolumeInfo updateVolume(String name, UpdateVolumeRequestContent updateVol "Volume already exists: " + updateVolumeRequest.getNewName()); } } + if (updateVolumeRequest.getNewName() == null && updateVolumeRequest.getComment() == null) { + tx.rollback(); + return convertFromDAO(volumeInfo, catalog, schema); + } if (updateVolumeRequest.getNewName() != null) { volumeInfo.setName(updateVolumeRequest.getNewName()); } diff --git a/server/src/test/java/io/unitycatalog/server/base/volume/BaseVolumeCRUDTest.java b/server/src/test/java/io/unitycatalog/server/base/volume/BaseVolumeCRUDTest.java index e07b78fa8..18882d82a 100644 --- a/server/src/test/java/io/unitycatalog/server/base/volume/BaseVolumeCRUDTest.java +++ b/server/src/test/java/io/unitycatalog/server/base/volume/BaseVolumeCRUDTest.java @@ -88,11 +88,18 @@ public void testVolumeCRUD() throws ApiException { VolumeInfo retrievedVolumeInfo = volumeOperations.getVolume(VOLUME_FULL_NAME); assertEquals(volumeInfo, retrievedVolumeInfo); - // Update volume - System.out.println("Testing update volume.."); + // Calling update volume with nothing to update should not change anything + System.out.println("Testing updating volume with nothing to update.."); + UpdateVolumeRequestContent emptyUpdateVolumeRequest = new UpdateVolumeRequestContent(); + VolumeInfo emptyUpdatedVolumeInfo = + volumeOperations.updateVolume(VOLUME_FULL_NAME, emptyUpdateVolumeRequest); + VolumeInfo retrievedVolumeInfo2 = volumeOperations.getVolume(VOLUME_FULL_NAME); + assertEquals(volumeInfo, retrievedVolumeInfo2); + + // Update volume name without updating comment + System.out.println("Testing update volume: changing name.."); UpdateVolumeRequestContent updateVolumeRequest = - new UpdateVolumeRequestContent().newName(VOLUME_NEW_NAME).comment(COMMENT); - // Set update details + new UpdateVolumeRequestContent().newName(VOLUME_NEW_NAME); VolumeInfo updatedVolumeInfo = volumeOperations.updateVolume(VOLUME_FULL_NAME, updateVolumeRequest); assertEquals(updateVolumeRequest.getNewName(), updatedVolumeInfo.getName()); @@ -100,6 +107,17 @@ public void testVolumeCRUD() throws ApiException { assertEquals(VOLUME_NEW_FULL_NAME, updatedVolumeInfo.getFullName()); assertNotNull(updatedVolumeInfo.getUpdatedAt()); + // Update volume comment without updating name + System.out.println("Testing update volume: changing comment.."); + UpdateVolumeRequestContent updateVolumeRequest2 = + new UpdateVolumeRequestContent().comment(COMMENT); + VolumeInfo updatedVolumeInfo2 = + volumeOperations.updateVolume(VOLUME_NEW_FULL_NAME, updateVolumeRequest2); + assertEquals(VOLUME_NEW_NAME, updatedVolumeInfo2.getName()); + assertEquals(updateVolumeRequest2.getComment(), updatedVolumeInfo2.getComment()); + assertEquals(VOLUME_NEW_FULL_NAME, updatedVolumeInfo2.getFullName()); + assertNotNull(updatedVolumeInfo2.getUpdatedAt()); + // Delete volume System.out.println("Testing delete volume.."); volumeOperations.deleteVolume(VOLUME_NEW_FULL_NAME);