jerryshao commented on code in PR #5293:
URL: https://github.com/apache/gravitino/pull/5293#discussion_r1859770061
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -210,7 +224,19 @@ private void handleCatalogCommand() {
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetCatalogProperty(url, ignore, metalake, catalog, property,
value).handle();
+ String entityName = line.getOptionValue(GravitinoOptions.NAME);
+ String owner = line.getOptionValue(GravitinoOptions.USER);
+ String group = line.getOptionValue(GravitinoOptions.GROUP);
+
+ if (owner != null && group == null) {
+ newSetOwner(url, ignore, metalake, entityName, entity, owner,
false).handle();
+ } else if (owner == null && group != null) {
+ newSetOwner(url, ignore, metalake, entityName, entity, group,
true).handle();
+ } else if (property != null && value != null) {
+ newSetCatalogProperty(url, ignore, metalake, catalog, property,
value).handle();
+ } else {
+ System.err.println(ErrorMessages.INVALID_SET_COMMAND);
+ }
Review Comment:
I feel that this bunch of `if...else` makes the code hard to maintain and
vulnerable. For example if "owner" and "property" are both set here, it will
choose the `if` branch, which is unexpected, we should not allow the user to
set both "owner" and "property", am I right?
A better way is to use some design patterns (command dispatcher pattern) to
refactor and enforce more checks.
--
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]
