New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-1617: Regions can be created with a variety of characters that … #285
Conversation
…are unsupported Added comprehensive test to validate region names
+1 |
I reworded your commit message:
|
@@ -0,0 +1,75 @@ | |||
package org.apache.geode.cache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the Apache license header on this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my local build didn't catch that and should have. Fixed.
ira.setInternalRegion(false); | ||
validateCharacters(ira); | ||
try { | ||
LocalRegion.validateRegionName("__InvalidInternalRegionName", ira); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't you missing a fail() stmt here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix up license and failure message.
} else { | ||
try { | ||
LocalRegion.validateRegionName(name, ira); | ||
fail("Should have received an IllegalArgumentException"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend adding which character to the message so we get instant info about which character should have been illegal but was allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added printing the offending character and the numeric value of it.
…are unsupported Added comprehensive test to validate region names
Pushed to develop. Thanks! |
…are unsupported
Added comprehensive test to validate region names