You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/11/28 20:29:06 UTC
[PR] Fix error in uuid validation, checkstyle fixes [accumulo]
EdColeman opened a new pull request, #3994:
URL: https://github.com/apache/accumulo/pull/3994
Fixes the UUID parsing check / validation in ServiceLock. The parsing of the lock string had a logic error with the indices used to parse out the parts. The error in the index parsing would pass not pass the complete UUID for validation.
The uuid check would pass for things that were UUID like, but not strictly a UUID. For example, it would accept 1-1-1-1-1 as a valid UUID string. This also allowed the incomplete uuid that was parsed from the lock string to pass.
This was found while reducing the reported check style errors and to favor Wait instead of sleep in the tests.
--
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
Re: [PR] Fix error in uuid validation, checkstyle fixes [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3994:
URL: https://github.com/apache/accumulo/pull/3994#discussion_r1408457397
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java:
##########
@@ -66,15 +68,39 @@ public void testSortAndFindLowestPrevPrefix() throws Exception {
ServiceLock.findLowestPrevPrefix(validChildren,
"zlock#00000000-0000-0000-0000-eeeeeeeeeeee#0000000010"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099"));
}
+ @Test
+ public void rejectInvalidUUID() {
+ List<String> children = new ArrayList<>();
+ String uuid = "1-1-1-1-1";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ // pass as UUID, but fail on string compare.
+ assertEquals("00000001-0001-0001-0001-000000000001", UUID.fromString(uuid).toString());
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(0, validChildren.size());
+ }
+
+ @Test
+ public void uuidTest() {
+ List<String> children = new ArrayList<>();
+ String uuid = "219ad0f6-ebe0-416e-a20f-c0f32922841d";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(1, validChildren.size());
+ String candidate = validChildren.get(0);
+ assertTrue(candidate.contains(uuid));
+ assertTrue(candidate.contains(seq));
Review Comment:
Would it be better to validate the entire string vs validating possible sections?
```suggestion
String candidate = "zlock#" + uuid + "#" + seq;
children.add(candidate);
final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
assertEquals(1, validChildren.size());
assertEquals(candidate, validChildren.get(0));
```
--
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
Re: [PR] Fix error in uuid validation, checkstyle fixes [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3994:
URL: https://github.com/apache/accumulo/pull/3994
--
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
Re: [PR] Fix error in uuid validation, checkstyle fixes [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3994:
URL: https://github.com/apache/accumulo/pull/3994#discussion_r1408553273
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockTest.java:
##########
@@ -66,15 +68,39 @@ public void testSortAndFindLowestPrevPrefix() throws Exception {
ServiceLock.findLowestPrevPrefix(validChildren,
"zlock#00000000-0000-0000-0000-eeeeeeeeeeee#0000000010"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001"));
- assertThrows(IndexOutOfBoundsException.class, () -> {
- ServiceLock.findLowestPrevPrefix(validChildren,
- "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099");
- });
+ assertThrows(IndexOutOfBoundsException.class,
+ () -> ServiceLock.findLowestPrevPrefix(validChildren,
+ "zlock#00000000-0000-0000-0000-XXXXXXXXXXXX#0000000099"));
}
+ @Test
+ public void rejectInvalidUUID() {
+ List<String> children = new ArrayList<>();
+ String uuid = "1-1-1-1-1";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ // pass as UUID, but fail on string compare.
+ assertEquals("00000001-0001-0001-0001-000000000001", UUID.fromString(uuid).toString());
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(0, validChildren.size());
+ }
+
+ @Test
+ public void uuidTest() {
+ List<String> children = new ArrayList<>();
+ String uuid = "219ad0f6-ebe0-416e-a20f-c0f32922841d";
+ String seq = "1234567891";
+ children.add("zlock#" + uuid + "#" + seq);
+
+ final List<String> validChildren = ServiceLock.validateAndSort(ServiceLock.path(""), children);
+ assertEquals(1, validChildren.size());
+ String candidate = validChildren.get(0);
+ assertTrue(candidate.contains(uuid));
+ assertTrue(candidate.contains(seq));
Review Comment:
The code in `ServiceLock.validateAndSort` parses and tests portions, but if the validation passes, it saves / returns the original strings. Testing either way then is not really a strong test. I was using this test to figure out where things were going wrong with the validation. I kept it because it was easier to use this to debug. The test above does test that the candidate string is returned correctly. If this test is not adding much value it can be deleted.
--
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