-
Notifications
You must be signed in to change notification settings - Fork 36
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
update regex and parse to support multiple underscores in prefix #363
base: main
Are you sure you want to change the base?
Conversation
@sjakos Thank you for putting together this PR (and apologies for the delay). Sending comments soon. |
@@ -13,8 +13,8 @@ create or replace function typeid_generate(prefix text) | |||
returns typeid | |||
as $$ | |||
begin | |||
if (prefix is null) or not (prefix ~ '^[a-z]{0,63}$') then | |||
raise exception 'typeid prefix must match the regular expression [a-z]{0,63}'; | |||
if (prefix is null) or not (prefix ~ '^[a-z_]{0,63}$') then |
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.
Technically this is not the right regex expression for two reasons:
- It allows underscores at the beginning
- It allows underscores at the end.
But underscores should only be allowed in the middle of the prefix. See https://github.com/jetify-com/typeid/tree/main/spec#type-prefix
The regex suggested there is ^([a-z]([a-z_]{0,61}[a-z])?)?$
In addition to changing the code, we should change the corresponding tests found here: https://github.com/jetify-com/opensource/blob/main/typeid/typeid-sql/supabase/tests/03_typeid.test.sql To match the latest test cases from the spec: In particular the following test needs to be added to the valid ones:
This test needs to be removed from the invalid ones:
And these two tests need to be added to the invalid ones:
Let me know if you need some guidance adding and running the tests. Alternatively we can check in your code changes (once the regex is updated) and I can send a separate PR for the tests. |
Summary
This is an update of the parse and regex matches to support multiple underscores in the prefix for v0.3 of the spec.