Skip to content
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

HV-1999 new CNPJ alphanumeric format #1365

Closed
wants to merge 1 commit into from
Closed

Conversation

detinho
Copy link

@detinho detinho commented Jun 21, 2024

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

Added an option to allow for gradual adoption.

Copy link
Member

@marko-bekhta marko-bekhta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋🏻
Thanks for your pull request. I've added some comments inline.

@@ -31,7 +31,7 @@
*
* @author George Gastaldi
*/
@Pattern(regexp = "([0-9]{2}[.]?[0-9]{3}[.]?[0-9]{3}[/]?[0-9]{4}[-]?[0-9]{2})")
@Pattern(regexp = "([0-9A-Z]{2}[.]?[0-9A-Z]{3}[.]?[0-9A-Z]{3}[/]?[0-9A-Z]{4}[-]?[0-9]{2})")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this pattern now depends on the CNPJ's version/format, I'd suggest moving it into the CNPJ validator itself and picking the pattern that suits the format.
Doing so would also mean that @ReportAsSingleViolation can be removed.

* @return Whether upper case letters characters in the validated input should be allowed ({@code true}) or result in a
* validation error ({@code false}).
*/
boolean alphanumeric() default false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest limit the changes to CNPJ constraint only

@@ -45,6 +45,8 @@

Class<? extends Payload>[] payload() default { };

boolean alphanumeric() default false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't an enum here be more future-proof option? something like

Suggested change
boolean alphanumeric() default false;
FormatRevision formatRevision() default FormatRevision.V1;
enum FormatRevision {
    // Obviously, the constant names have to be more meaningful 
    //  and connected to the versions of current and new formats.
    V1, V2; 
}

This way, if another version is published, it can be added without any new parameters on the constraint annotation...

Comment on lines +85 to +93
protected void initialize(int startIndex, int endIndex, int checkDigitIndex, boolean ignoreNonValidCharacters) {
this.startIndex = startIndex;
this.endIndex = endIndex;
this.checkDigitIndex = checkDigitIndex;
this.ignoreNonDigitCharacters = ignoreNonDigitCharacters;
this.ignoreNonValidCharacters = ignoreNonValidCharacters;
this.alphanumeric = false;

this.validateOptions();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just call

initialize(startIndex, endIndex, checkDigitIndex, ignoreNonValidCharacters, false)

and also if you'll end up still needing to modify the ModCheckBase consider using this overloaded version of initialize in places where alphanumeric is not needed so that less classes are modified.

return Character.digit( value, DEC_RADIX );
if ( alphanumeric ) {
if ( (value >= '0' && value <= '9' ) || (value >= 'A' && value <= 'Z') ) {
return value - BASE_CHAR_INDEX;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way of converting seems somewhat specific to CNPJ, I'd suggest creating an extended version of Mod11CheckValidator in CNPJValidator and overriding extractDigit in it.

@marko-bekhta
Copy link
Member

Thanks for the work and for bringing up the fact that there's an upcoming change in the format. I've addressed the review suggestions and aligned things more closely to how HV does things on top of your initial patch in this PR #1386 .

I am closing this one. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants