HV-1867 Add UUID validation #1199

dheid · 2022-01-09T20:05:26Z

https://hibernate.atlassian.net/browse/HV-1867

Hibernate-CI · 2022-01-09T20:05:27Z

Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test")

dheid · 2022-01-09T20:19:51Z

@gsmet Could you have a look at it, please? I think, a character sequence UUID validation could be useful for many users.

engine/src/main/java/org/hibernate/validator/constraints/UUID.java

...ne/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/UUIDValidator.java

dheid · 2022-01-11T09:19:21Z

@yrodiere @marko-bekhta Something else I can do? Have you decided to merge it yet?

yrodiere · 2022-01-11T09:53:45Z

@dheid I don't decide that kind of things on this project :) I just saw your patch and thought I'd suggest an improvement.

You'll have to wait for @gsmet to have a look, and this can take time since he has a lot to do (on this project and others) these days.

dheid · 2022-02-20T10:42:19Z

@gsmet Do you have time to look at this pull request?

hibernate-github-bot · 2022-02-20T10:46:54Z

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

gsmet · 2022-02-22T12:05:50Z

Rebased to get the latest CI fixes.

gsmet · 2022-02-22T14:51:18Z

Hey @dheid

Sorry for the delay, I just got back from PTO and things were crazy before and obviously even crazier after.

This is on my TODO list. I definitely think it has value and it looks like a very nice work.

I'll ping you as soon as I'll get to it.

dheid · 2022-02-22T14:54:37Z

@gsmet Thank you so much!!!

beikov

Other than my suggestion the code looks good to me.

beikov · 2022-02-22T16:37:00Z

...ne/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/UUIDValidator.java

+		for ( int charIndex = 0; charIndex < valueLength; charIndex++ ) {
+
+			char ch = value.charAt( charIndex );
+
+			if ( ch == '-' ) {
+				groupIndex++;
+				groupLength = 0;
+			}
+			else {
+
+				groupLength++;
+				if ( groupLength > GROUP_LENGTHS[groupIndex] ) {
+					return false;
+				}
+
+				int numericValue = Character.digit( ch, 16 );
+				if ( numericValue == -1 ) {
+					// not a hex digit
+					return false;
+				}
+				if ( letterCase == LetterCase.LOWER_CASE && numericValue > 9 && !Character.isLowerCase( ch ) ) {
+					return false;
+				}
+				if ( letterCase == LetterCase.UPPER_CASE && numericValue > 9 && !Character.isUpperCase( ch ) ) {
+					return false;
+				}
+				checksum += numericValue;
+				version = extractVersion( version, charIndex, numericValue );
+				variant = extractVariant( variant, charIndex, numericValue );
+
+			}
+
+		}


This is not a request for a change, but I just wanted to share how I would have expected this to look like in case you want to change the implementation.
I would have expected a constant loop from 0 to 35 with a big switch covering all indices and asserting a valid char is used. Something like this:

Suggested change

for ( int charIndex = 0; charIndex < valueLength; charIndex++ ) {

char ch = value.charAt( charIndex );

if ( ch == '-' ) {

groupIndex++;

groupLength = 0;

}

else {

groupLength++;

if ( groupLength > GROUP_LENGTHS[groupIndex] ) {

return false;

}

int numericValue = Character.digit( ch, 16 );

if ( numericValue == -1 ) {

// not a hex digit

return false;

}

if ( letterCase == LetterCase.LOWER_CASE && numericValue > 9 && !Character.isLowerCase( ch ) ) {

return false;

}

if ( letterCase == LetterCase.UPPER_CASE && numericValue > 9 && !Character.isUpperCase( ch ) ) {

return false;

}

checksum += numericValue;

version = extractVersion( version, charIndex, numericValue );

variant = extractVariant( variant, charIndex, numericValue );

}

}

LOOP: for ( int charIndex = 0; charIndex < 36; charIndex++ ) {

char ch = value.charAt( charIndex );

switch ( charIndex ) {

// Handle hyphens

case 9:

case 14:

case 19:

case 24:

if ( ch != '-' ) {

return false;

}

continue LOOP;

// Handle M and N parts

case 15:

version = extractVersion( ch );

break;

case 20:

variant = extractVariant( ch );

break;

}

int numericValue = Character.digit( ch, 16 );

if ( numericValue == -1 ) {

// not a hex digit

return false;

}

switch ( letterCase ) {

case LOWER_CASE:

if ( numericValue > 9 && !Character.isLowerCase( ch ) ) {

return false;

}

break;

case UPPER_CASE :

if ( numericValue > 9 && !Character.isUpperCase( ch ) ) {

return false;

}

break;

}

checksum += numericValue;

}

Since the chars have to be in ASCII encoding, you could go even a step further if you like by introducing an array for the character code points.

private static final byte[] DIGITS = new byte[] { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, -1, -1, -1, -1, -1, -1, -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 10, 11, 12, 13, 14, 15 }; int charAsInt = ch; int numericValue = charAsInt >= 0 && charAsInt < DIGITS.length ? DIGITS[charAsInt] : -1; if ( numericValue == -1 ) { // not a hex digit return false; } switch ( letterCase ) { case LOWER_CASE: if ( ch > 'A' && ch < 'F' ) { return false; } break; case UPPER_CASE : if ( ch > 'a' && ch < 'f' ) { return false; } break; }

Tbh Christian, I find Daniel's approach more concise and I don't see how your suggestion actually improves upon it? It also doesn't account for the group length, IINM.

I like this creative solution! But if you don't mind I'll stick to my solution, because it's easier to understand from my point of view.

jrenaat · 2022-02-22T21:33:52Z

...ne/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/UUIDValidator.java

+			}
+			return Arrays.binarySearch( this.variant, variant ) != -1;
+		}
+


Shouldn't there be some sort of check in here that the version belongs to {1,2,3,4,5} ?
In the UUIDValidatorTest you do verify that versions match, but you're using versions that are (IIUC) are not allowed?

I hope that I understand you correctly: The version can have a value that is not yet officially existing. But maybe in the future there will be a new kind of UUID and the implementation will support it.

Ok, in general I'm somewhat more in favor of not foreseeing everything that could potentially happen in the future, but it's not a big issue, I'll leave it up to you. I do like your validation implementation, I think it's quite elegant.

Thank you! Really cheers me up. ☺️

jrenaat · 2022-02-23T13:05:41Z

...st/java/org/hibernate/validator/test/internal/constraintvalidators/hv/UUIDValidatorTest.java

+		assertFalse( uuidValidator.isValid( "2d5614ff-891e-07a8-b49e-9758506a9bab", null ) );
+		assertFalse( uuidValidator.isValid( "2d5614ff-891e-07a8-b49e-a758506a9bab", null ) );
+		assertFalse( uuidValidator.isValid( "2d5614ff-891e-07a8-b49e-b758506a9bab", null ) );
+


Minor detail: is there a point in doing 12 invalid variant checks if only the last group is different?

Haha, that's a mistake. I'll fix that.

yep, thanks

dheid · 2022-02-23T13:45:12Z

@jrenaat @yrodiere @beikov @gsmet @marko-bekhta I extracted the letter case check to a separate method for enhanced readability

gsmet

Thanks for the various reviews everyone.

This looks good and will go into 8.0.0 (note that 8.0.0 is targeting Jakarta EE 10 and will be released when we have all the Jakarta EE 10 specs in their final versions). I adjusted the @since accordingly.

gsmet · 2022-02-28T13:51:22Z

@dheid I will merge as soon as CI is green. Thanks for this work!

gsmet · 2022-02-28T13:59:04Z

And merged! Thanks.

dheid · 2022-02-28T14:00:20Z

Thanks everyone so much!

yrodiere reviewed Jan 10, 2022

View reviewed changes

engine/src/main/java/org/hibernate/validator/constraints/UUID.java Outdated Show resolved Hide resolved

marko-bekhta reviewed Jan 10, 2022

View reviewed changes

...ne/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/UUIDValidator.java Outdated Show resolved Hide resolved

beikov approved these changes Feb 22, 2022

View reviewed changes

jrenaat reviewed Feb 22, 2022

View reviewed changes

jrenaat reviewed Feb 23, 2022

View reviewed changes

HV-1867 Add UUID validation

6e623e9

gsmet approved these changes Feb 28, 2022

View reviewed changes

gsmet merged commit 233e4ea into hibernate:main Feb 28, 2022

dheid mentioned this pull request Mar 9, 2022

Add validator for UUIDs apache/commons-validator#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HV-1867 Add UUID validation #1199

HV-1867 Add UUID validation #1199

dheid commented Jan 9, 2022

Hibernate-CI commented Jan 9, 2022

dheid commented Jan 9, 2022

dheid commented Jan 11, 2022

yrodiere commented Jan 11, 2022 •

edited

dheid commented Feb 20, 2022

hibernate-github-bot bot commented Feb 20, 2022 •

edited

gsmet commented Feb 22, 2022

gsmet commented Feb 22, 2022

dheid commented Feb 22, 2022

beikov left a comment

beikov Feb 22, 2022

beikov Feb 22, 2022

jrenaat Feb 22, 2022 •

edited

dheid Feb 22, 2022

jrenaat Feb 22, 2022

dheid Feb 22, 2022

jrenaat Feb 23, 2022

dheid Feb 23, 2022

jrenaat Feb 23, 2022

dheid Feb 23, 2022

dheid Feb 23, 2022

jrenaat Feb 23, 2022

dheid commented Feb 23, 2022

gsmet left a comment

gsmet commented Feb 28, 2022

gsmet commented Feb 28, 2022

dheid commented Feb 28, 2022

HV-1867 Add UUID validation #1199

HV-1867 Add UUID validation #1199

Conversation

dheid commented Jan 9, 2022

Hibernate-CI commented Jan 9, 2022

dheid commented Jan 9, 2022

dheid commented Jan 11, 2022

yrodiere commented Jan 11, 2022 • edited

dheid commented Feb 20, 2022

hibernate-github-bot bot commented Feb 20, 2022 • edited

gsmet commented Feb 22, 2022

gsmet commented Feb 22, 2022

dheid commented Feb 22, 2022

beikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrenaat Feb 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheid commented Feb 23, 2022

gsmet left a comment

Choose a reason for hiding this comment

gsmet commented Feb 28, 2022

gsmet commented Feb 28, 2022

dheid commented Feb 28, 2022

yrodiere commented Jan 11, 2022 •

edited

hibernate-github-bot bot commented Feb 20, 2022 •

edited

jrenaat Feb 22, 2022 •

edited