[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5159: GEODE-7956: correct legal region names

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #5159:
URL: https://github.com/apache/geode/pull/5159#discussion_r433083367



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/RegionNameValidation.java
##########
@@ -24,7 +24,7 @@
 
 public class RegionNameValidation {
 
-  private static final Pattern NAME_PATTERN = Pattern.compile("[aA-zZ0-9-_.]+");
+  private static final Pattern NAME_PATTERN = Pattern.compile("[a-zA-Z\\[\\]0-9-_.]+");

Review comment:
       When I read aA-zZ I thought it was just a special regex pattern to match a-zA-Z. I bet whoever wrote this thought that. But since the product was already released with a pattern that allows those other characters it would be safest to continue to allow those characters. To make a breaking change I think we need an RFC. For now it is best to change the docs to describe the behavior of the existing geode release. We could schedule removing these special characters from region names in a future release. I think it would be good to rewrite the regex to still match what it did before but to do it more explicitly (something like a-zA-Z0-9-_.^`[]\\) or add comments to make clear what A-z matches. I also think it would be good to have some explicit test methods that show that these special characters are allowed in region names.
   Thanks for figuring this out!




----------------------------------------------------------------
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