You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/12/30 16:13:44 UTC
[GitHub] [accumulo] ddanielr opened a new pull request, #3146: Check Prop Value Format and Key types.
ddanielr opened a new pull request, #3146:
URL: https://github.com/apache/accumulo/pull/3146
relates to #3145
The shell config command was only checking Property Key Types and not Value Formats on Set operations. Changed Set operation to also validate value types to ensure proper format before setting properties.
This changes the shell client behavior to now throw an error message to the user on an invalid property setting.
Boolean Example:
```
root@uno> config -s tserver.port.search=123
2022-12-30T15:14:21,363 [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Invalid Property value (requires type: boolean) near index 30
config -s tserver.port.search=123
```
Port Example:
```
root@uno> config -s tserver.port.client=65539
2022-12-30T15:15:29,328 [shell.Shell] ERROR: org.apache.accumulo.core.util.BadArgumentException: Invalid Property value (requires type: port) near index 30
config -s tserver.port.client=65539
```
This PR is staged against main, but we may want to backport this change.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1088691250
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -353,6 +344,31 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
return 0;
}
+ private void validatePropertyType(String property, String value, String fullCommand,
+ String tableName) throws BadArgumentException {
+ if (tableName != null) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property: " + property, fullCommand,
+ fullCommand.indexOf(property));
+ }
+ }
+ if (!Property.isValidZooPropertyKey(property)) {
Review Comment:
Makes sense to me, I'll move the code up.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr closed pull request #3146: Check Prop Value Format and Key types.
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr closed pull request #3146: Check Prop Value Format and Key types.
URL: https://github.com/apache/accumulo/pull/3146
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
ddanielr commented on PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#issuecomment-1382618383
> Ideally we'd validate property values on the server side, and reject the RPC request (and propagate the error through the public API call), when validation fails.
Yep, I'm working on that now. I think it's also valuable to have the shell perform validation and not issue the RPC request in the first place.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#issuecomment-1368086476
> Ideally we'd validate property values on the server side, and reject the RPC request (and propagate the error through the public API call), when validation fails.
Is there a bigger problem here? Re my comment earlier about testing the shell, it might be nice to have an IT for the public API also that tries to set a bad value if we do not have one.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1059461050
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -127,19 +127,29 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
value = pair[1];
if (tableName != null) {
- if (!Property.isValidTablePropertyKey(property)) {
- throw new BadArgumentException("Invalid per-table property.", fullCommand,
- fullCommand.indexOf(property));
+ if (!Property.isTablePropertyValid(property, value)) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property.", fullCommand,
+ fullCommand.indexOf(property));
+ } else {
+ throw new BadArgumentException("Invalid property value.", fullCommand,
+ fullCommand.indexOf(value));
+ }
}
if (property.equals(Property.TABLE_DEFAULT_SCANTIME_VISIBILITY.getKey())) {
new ColumnVisibility(value); // validate that it is a valid expression
}
shellState.getAccumuloClient().tableOperations().setProperty(tableName, property, value);
Shell.log.debug("Successfully set table configuration option.");
Review Comment:
It looks like a lot of this code is duplicated in the next `else if` block. I wonder if it would be worth it to extract some of the logic into a new method for reuse.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1059461433
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -127,19 +127,29 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
value = pair[1];
if (tableName != null) {
- if (!Property.isValidTablePropertyKey(property)) {
- throw new BadArgumentException("Invalid per-table property.", fullCommand,
- fullCommand.indexOf(property));
+ if (!Property.isTablePropertyValid(property, value)) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property.", fullCommand,
Review Comment:
This exception message could mention that the prop key specifically is invalid.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1088990088
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -353,6 +344,31 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
return 0;
}
+ private void validatePropertyType(String property, String value, String fullCommand,
+ String tableName) throws BadArgumentException {
Review Comment:
Properties are a hierarchy `default <- system <- namespace <- table` with the more specific overriding the more generic. I'm not sure of the top of my head, but I expect that all table properties are valid namespace properties. There may be a few specific system only props, but in general they should also be override-able in a namespace or table.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#issuecomment-1368086013
If possible, it would be nice to add an IT to the ShellIT that exercises setting a prop w/ a bad value.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1088690055
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -353,6 +344,31 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
return 0;
}
+ private void validatePropertyType(String property, String value, String fullCommand,
+ String tableName) throws BadArgumentException {
Review Comment:
I'm not sure how to determine the best fix for this since `namespace` and `table` don't have good property separation.
Only `ValidTableProperties` are defined in `Property.java`.
There is not a separate `ValidNamespaceProperties` hashset defined.
https://github.com/apache/accumulo/blob/339672a6abb13762c68e46a2d8ba49f1d0b0150e/core/src/main/java/org/apache/accumulo/core/conf/Property.java#L1378-L1381
You can also see the interchangeability in the property set code for `namespace`. It's just checking if it's a valid table property vs anything `namespace` specific.
https://github.com/apache/accumulo/blob/339672a6abb13762c68e46a2d8ba49f1d0b0150e/shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java#L105-L106
So, should namespaces have separate properties that aren't table properties?
if so, I can rework this and also add a `isValidNamespacePropertyKey` method to `Property.java`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1084464373
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -353,6 +344,31 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
return 0;
}
+ private void validatePropertyType(String property, String value, String fullCommand,
+ String tableName) throws BadArgumentException {
+ if (tableName != null) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property: " + property, fullCommand,
+ fullCommand.indexOf(property));
+ }
+ }
+ if (!Property.isValidZooPropertyKey(property)) {
Review Comment:
Should this be checked first? It does not really matter if it is in the correct format or not if it can never be change in ZooKeeper.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on pull request #3146: Check Prop Value Format and Key types.
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#issuecomment-1409706096
I've thought about these changes and I'm closing this PR in favor of the fix in #3177
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1084461119
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -353,6 +344,31 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
return 0;
}
+ private void validatePropertyType(String property, String value, String fullCommand,
+ String tableName) throws BadArgumentException {
Review Comment:
It looks like instead of `tableName` the parameter is more generic name (either table name of namespace name)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
ddanielr commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1070184250
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -127,19 +127,29 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
value = pair[1];
if (tableName != null) {
- if (!Property.isValidTablePropertyKey(property)) {
- throw new BadArgumentException("Invalid per-table property.", fullCommand,
- fullCommand.indexOf(property));
+ if (!Property.isTablePropertyValid(property, value)) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property.", fullCommand,
+ fullCommand.indexOf(property));
+ } else {
+ throw new BadArgumentException("Invalid property value.", fullCommand,
+ fullCommand.indexOf(value));
+ }
}
if (property.equals(Property.TABLE_DEFAULT_SCANTIME_VISIBILITY.getKey())) {
new ColumnVisibility(value); // validate that it is a valid expression
}
shellState.getAccumuloClient().tableOperations().setProperty(tableName, property, value);
Shell.log.debug("Successfully set table configuration option.");
Review Comment:
Yes, extracting this out to another method cleaned up a good bit of the if/else logic in the "set operation" block.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ddanielr commented on a diff in pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
ddanielr commented on code in PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#discussion_r1070184539
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -127,19 +127,29 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
value = pair[1];
if (tableName != null) {
- if (!Property.isValidTablePropertyKey(property)) {
- throw new BadArgumentException("Invalid per-table property.", fullCommand,
- fullCommand.indexOf(property));
+ if (!Property.isTablePropertyValid(property, value)) {
+ if (!Property.isValidTablePropertyKey(property)) {
+ throw new BadArgumentException("Invalid per-table property.", fullCommand,
Review Comment:
Added property value to the exception message
https://github.com/apache/accumulo/pull/3146/files#diff-dedfd7e340f58e41d93da9c80014542cb08d27c4448d77cfcf69ff85457a0f0cR351
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3146: Check Prop Value Format and Key types.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3146:
URL: https://github.com/apache/accumulo/pull/3146#issuecomment-1368012817
I think the fix, when ready, can be applied to the 2.1 branch. This PR is currently targeting the main branch. If you cherry-pick the changes to a 2.1 bugfix branch, and force push to the branch that is backing this PR, we can edit the PR's base branch to make this a PR against 2.1.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org