You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/07 21:21:54 UTC
[GitHub] [geode] jhutchison opened a new pull request #5604: GEODE-8583: create restricted input values parameter requirements
jhutchison opened a new pull request #5604:
URL: https://github.com/apache/geode/pull/5604
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501863115
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
Review comment:
will do, thx
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r509697484
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501326934
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
It seems like it would be better to only iterate over a sublist that excludes that first item (which is the commandType). Saying a parameter is okay if it is equal "commandType" would allow someone to do this following command: "INFO INFO INFO" and it would not care about the extra INFO parameters because they match the commandType.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
Review comment:
make this private final and initialize it only in the constructor
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
+ }
+ });
+ }
+
+ private boolean isNotAllowed(String parameterString) {
+ return (allowedValues.contains(parameterString));
Review comment:
Do you want to ignore case hear also? You could store all the values given to the constructor in lower case and then here convert parameterString to lower case and then contains would work.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
Review comment:
Instead of a List<String> have the method take varargs (i.e. "(String... allowedValues)"). Then callers can just say new RestrictedInputValuesParameterRequirements("SERVER", "PERSISTENCE") instead of having to create its own collection
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -46,8 +47,10 @@ private boolean containsSectionParameter(List<ByteArrayWrapper> commands) {
private String getSpecifiedSection(ExecutionHandlerContext context,
List<ByteArrayWrapper> commands) {
- String result;
- String section = commands.get(1).toString().toLowerCase();
+ String result = "";
Review comment:
I think the old code (using default on the switch) is better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501864551
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
+ }
+ });
+ }
+
+ private boolean isNotAllowed(String parameterString) {
+ return (allowedValues.contains(parameterString));
Review comment:
probably. Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501859545
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
> This PR changes way too many files. I didn't look at all of them. But it should have only changed 2 or 3.
> I'm not sure I like this change because now we have the parameters in two places. We still have the switch statement in InfoExecutor that checks for each one, but now we also have a list of them in another place (ALLOWED_INFO_COMMANDS). The other confusing thing is that the INFO command does allow other keywords it just ignores them. In native redis "INFO foobar" does not give an error but just returns nothing. But by adding a RestrictedInputValuesParameterRequirements to INFO it makes me think that any other params would cause an error.
I'll double check, but the large number of files changed should be the result of my moving the RedisConstants class into a new subdirectory (because there is now another constants files to hold specifically allowed parameters , and it seemed to make sense to group them together). To be addressed below, I did forget to make the change to allow an error mesg for the class to be passed in as a parameter. As you say, for info, the error mesg will just be an empty string, but other commands can pass in other other err msgs as is appropriate to match native redis behavior.
I still like having the logic for the commands in the executor for now. I see your point about the list of allowable commands being in 2 places. Ultimately I would like to use a singleton enum, but wasn't able to figure out how to make that work without some more playing around (let me know if you have thoughts)
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -93,13 +86,21 @@ public GeodeRedisServer(String bindAddress, int port, InternalCache cache) {
CommandFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RenameFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RedisCommandFunction.register();
+
passiveExpirationManager =
new PassiveExpirationManager(regionProvider.getDataRegion(), redisStats);
- redisCommandExecutor = LoggingExecutors.newCachedThreadPool("GeodeRedisServer-Command-", true);
- nettyRedisServer = new NettyRedisServer(() -> cache.getInternalDistributedSystem().getConfig(),
- regionProvider, pubSub,
- this::allowUnsupportedCommands, this::shutdown, port, bindAddress, redisStats,
- redisCommandExecutor);
+ redisCommandExecutor =
+ LoggingExecutors.newCachedThreadPool("Redis Command", true);
Review comment:
makes sense. I think I'm going to slowly lobby to see if I can get some things updated in Spotless...
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
Review comment:
the idea is to allow a mesg to be passed as a parameter. I'm only using it for info command now, and forgot to make that change. Thanks for the catch
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
Also, I left the commit history, in an attempt to make it easy to see that a lot of files were changed when I moved the redisConstants class. Is there a better way to communicate/show that in PRs? just a separate PR?
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
Review comment:
will do, thx
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
Review comment:
So the reason I did this, was to allow the list of a parameters to exist as a constant, and allow that constant to be passed in when the command type is declared. Didn't seem to work with the variable arg option (but that is how I had it before introducing the constant list of params)
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
+ }
+ });
+ }
+
+ private boolean isNotAllowed(String parameterString) {
+ return (allowedValues.contains(parameterString));
Review comment:
probably. Thanks
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
looks like I also included some left over gemfire servers in one of the commits too :).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5604:
URL: https://github.com/apache/geode/pull/5604#issuecomment-714528770
> This PR changes way too many files. I didn't look at all of them. But it should have only changed 2 or 3.
> I'm not sure I like this change because now we have the parameters in two places. We still have the switch statement in InfoExecutor that checks for each one, but now we also have a list of them in another place (ALLOWED_INFO_COMMANDS). The other confusing thing is that the INFO command does allow other keywords it just ignores them. In native redis "INFO foobar" does not give an error but just returns nothing. But by adding a RestrictedInputValuesParameterRequirements to INFO it makes me think that any other params would cause an error.
OK- SO I finally see your point about what INFO returns. You are correct, there shouldn't be an error. A false pass for an integration test was making me think that this was working correctly (that the error was being caught and only the error mesg was being returned to the end user). But that's not the case. To your point, this should not be used for the info command then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r502004821
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
looks like I also included some left over gemfire servers in one of the commits too :).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r510199178
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
you were righ-thanks. change to be made
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
you were right-thanks. change to be made
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501861215
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -93,13 +86,21 @@ public GeodeRedisServer(String bindAddress, int port, InternalCache cache) {
CommandFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RenameFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RedisCommandFunction.register();
+
passiveExpirationManager =
new PassiveExpirationManager(regionProvider.getDataRegion(), redisStats);
- redisCommandExecutor = LoggingExecutors.newCachedThreadPool("GeodeRedisServer-Command-", true);
- nettyRedisServer = new NettyRedisServer(() -> cache.getInternalDistributedSystem().getConfig(),
- regionProvider, pubSub,
- this::allowUnsupportedCommands, this::shutdown, port, bindAddress, redisStats,
- redisCommandExecutor);
+ redisCommandExecutor =
+ LoggingExecutors.newCachedThreadPool("Redis Command", true);
Review comment:
makes sense. I think I'm going to slowly lobby to see if I can get some things updated in Spotless...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501946115
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
I should have looked at the diffs with the commit history, my bad for not doing so, that is a good way to address this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r510219592
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
Review comment:
change made
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501729688
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
I don't think this is what you want here. `parameter.toString()` is just going to produce something like `[B@6ea12c19`. You need to create a new String from the byte array.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501946115
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
I should have looked at the diffs with the commit history, my bad for not doing so, that is a good way to address this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on pull request #5604:
URL: https://github.com/apache/geode/pull/5604#issuecomment-720559420
not needed for intended use case; no other current use cases for this object
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501337678
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
Review comment:
It seems odd to throw an exception with an empty string message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r510199535
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/server/InfoExecutor.java
##########
@@ -46,8 +47,10 @@ private boolean containsSectionParameter(List<ByteArrayWrapper> commands) {
private String getSpecifiedSection(ExecutionHandlerContext context,
List<ByteArrayWrapper> commands) {
- String result;
- String section = commands.get(1).toString().toLowerCase();
+ String result = "";
Review comment:
changed back
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501859545
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
> This PR changes way too many files. I didn't look at all of them. But it should have only changed 2 or 3.
> I'm not sure I like this change because now we have the parameters in two places. We still have the switch statement in InfoExecutor that checks for each one, but now we also have a list of them in another place (ALLOWED_INFO_COMMANDS). The other confusing thing is that the INFO command does allow other keywords it just ignores them. In native redis "INFO foobar" does not give an error but just returns nothing. But by adding a RestrictedInputValuesParameterRequirements to INFO it makes me think that any other params would cause an error.
I'll double check, but the large number of files changed should be the result of my moving the RedisConstants class into a new subdirectory (because there is now another constants files to hold specifically allowed parameters , and it seemed to make sense to group them together). To be addressed below, I did forget to make the change to allow an error mesg for the class to be passed in as a parameter. As you say, for info, the error mesg will just be an empty string, but other commands can pass in other other err msgs as is appropriate to match native redis behavior.
I still like having the logic for the commands in the executor for now. I see your point about the list of allowable commands being in 2 places. Ultimately I would like to use a singleton enum, but wasn't able to figure out how to make that work without some more playing around (let me know if you have thoughts)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501862775
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
Review comment:
Also, I left the commit history, in an attempt to make it easy to see that a lot of files were changed when I moved the redisConstants class. Is there a better way to communicate/show that in PRs? just a separate PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501729688
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
I don't thing this is what you want here. `parameter.toString()` is just going to produce something like `[B@6ea12c19`. You need to create a new String from the byte array.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501336900
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/GeodeRedisServer.java
##########
@@ -93,13 +86,21 @@ public GeodeRedisServer(String bindAddress, int port, InternalCache cache) {
CommandFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RenameFunction.register(regionProvider.getDataRegion(), stripedExecutor, redisStats);
RedisCommandFunction.register();
+
passiveExpirationManager =
new PassiveExpirationManager(regionProvider.getDataRegion(), redisStats);
- redisCommandExecutor = LoggingExecutors.newCachedThreadPool("GeodeRedisServer-Command-", true);
- nettyRedisServer = new NettyRedisServer(() -> cache.getInternalDistributedSystem().getConfig(),
- regionProvider, pubSub,
- this::allowUnsupportedCommands, this::shutdown, port, bindAddress, redisStats,
- redisCommandExecutor);
+ redisCommandExecutor =
+ LoggingExecutors.newCachedThreadPool("Redis Command", true);
Review comment:
This is an example of why it is better not to change a bunch of formatting. Just let spotless do the formatting. In this case I think you accidently lost a change to the thread name. Someone else changed it to "GeodeRedisServer-Command-" and I think you lost that and reverted to back to "Redis Command". You should have had a conflict but when you have changed a bunch of formatting it can be hard for you to see things like this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501864369
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
Review comment:
So the reason I did this, was to allow the list of a parameters to exist as a constant, and allow that constant to be passed in when the command type is declared. Didn't seem to work with the variable arg option (but that is how I had it before introducing the constant list of params)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison closed pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison closed pull request #5604:
URL: https://github.com/apache/geode/pull/5604
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501861889
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
Review comment:
the idea is to allow a mesg to be passed as a parameter. I'm only using it for info command now, and forgot to make that change. Thanks for the catch
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jhutchison commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jhutchison commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r510259682
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
+ if (isNotAllowed(parameterString) &&
+ !parameterString.equalsIgnoreCase(commandType)) {
+ throw new RedisParametersMismatchException("");
+ }
+ });
+ }
+
+ private boolean isNotAllowed(String parameterString) {
+ return (allowedValues.contains(parameterString));
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5604: GEODE-8583: create restricted input values parameter requirements
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5604:
URL: https://github.com/apache/geode/pull/5604#discussion_r501729688
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
I don't thing this is what you want here. `parameter.toString()` is just going to produce something like `[B@6ea12c19`. You need to create a new String from the byte array.
##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/ParameterRequirements/RestrictedInputValuesParameterRequirements.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.geode.redis.internal.ParameterRequirements;
+
+import java.util.List;
+
+import org.apache.geode.redis.internal.netty.Command;
+import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+
+public class RestrictedInputValuesParameterRequirements implements ParameterRequirements {
+
+ List<String> allowedValues = null;
+
+ public RestrictedInputValuesParameterRequirements(List<String> allowedValues) {
+ this.allowedValues = allowedValues;
+ }
+
+ @Override
+ public void checkParameters(Command command,
+ ExecutionHandlerContext executionHandlerContext) {
+ List<byte[]> parameters = command.getProcessedCommand();
+ String commandType = command.getCommandType().name();
+
+ parameters.forEach(parameter -> {
+ String parameterString = parameter.toString();
Review comment:
I don't think this is what you want here. `parameter.toString()` is just going to produce something like `[B@6ea12c19`. You need to create a new String from the byte array.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org