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

bug? #8

Closed
kumlien opened this issue Apr 4, 2019 · 6 comments · Fixed by #9
Closed

bug? #8

kumlien opened this issue Apr 4, 2019 · 6 comments · Fixed by #9
Assignees
Labels

Comments

@kumlien
Copy link

kumlien commented Apr 4, 2019

Hi guys,
This might be a bug. Using one of the ssn:s in your tests (701063-2391) and then prepending it with a '9' still returns true.
This test fails:
assertFalse(Personnummer.valid("9701063-2391"));

br Svante

@Johannestegner
Copy link
Member

Thank you for the report!


I would say that it is the following regexp pattern that is the issue:
"(\\d{2})?(\\d{2})(\\d{2})(\\d{2})([-|+]?)?(\\d{3})(\\d?)"

Seeing that it allows for {2}? digits (optional) I'm guessing that it matches the rest and ignores the first number (and only uses it if it is two numbers rather than one). If it should be seen as illegal it should be either 0 or two rather than using the optional test.

Not really sure how we should handle this, what do you think @frozzare, force 0-2 numbers maybe?

@Johannestegner
Copy link
Member

Setting it to ^(\d{2})?(\d{2})(\d{2})(\d{2})([-|+]?)?(\d{3})(\d?)$ (forcing it to be within the bounds) would probably work (as it would see "9701063-2391" as invalid rather than valid-ish).

@kumlien
Copy link
Author

kumlien commented Apr 5, 2019

Yes, that works!

@frozzare
Copy link
Member

frozzare commented Apr 5, 2019

Sounds good @Johannestegner !

@kumlien
Copy link
Author

kumlien commented Apr 5, 2019

Ok then @frozzare @Johannestegner , if you update regexp and unit test I'll close the ticket :)

@Johannestegner
Copy link
Member

Sorry for slow fix, had RL issues to take care of and then java errors to fight with, hehee.
PR is up, should auto-close the issue as soon as it is merged :)

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

Successfully merging a pull request may close this issue.

3 participants