-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[component] Restrict character set for component.ID name #10674
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10674 +/- ##
==========================================
- Coverage 92.38% 92.24% -0.15%
==========================================
Files 403 403
Lines 18729 18726 -3
==========================================
- Hits 17303 17274 -29
- Misses 1066 1097 +31
+ Partials 360 355 -5 ☔ View full report in Codecov by Sentry. |
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.
Looks great, thank you! Just one thought about the allowed characters.
component/config.go
Outdated
// nameRegexp is used to validate the name of a component. | ||
// A name must start with an ASCII alphanumeric character and | ||
// can only contain ASCII alphanumeric characters, '_', and '-'. | ||
var nameRegexp = regexp.MustCompile(`^[0-9a-zA-Z][0-9a-zA-Z_-]{0,62}$`) |
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.
What do you think about opening this up to all Unicode letters and numbers? i.e. swapping [0-9]
for \pN
(numbers) and [a-zA-Z]
for \pL
(letters). Perhaps also replacing the hyphen and dash with \pP
(punctuation).
I think ASCII is reasonable for type, since there's a specific set of them. On the other hand, the name component is arbitrary and user-defined. So, for example, allowing Chinese characters may be more inclusive of native Chinese speakers/readers/writers.
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.
Can we start with introducing this restriction only on UnmarshalText
?
We can later think about adding Name
or not, but that is the part that impacts end-users of the binary and I think we can start there
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.
Thank you for the changes, looks great!
Description
While working on #10495 we discovered that there are not any restrictions on the
name
field of acomponent.ID
. There are restrictions on thetype
field introduced in #9208. This PR adds similar restrictions toname
.A type must
I found that we need a slightly different set of rules for name as some tests use a digit and others use a uuid as a name. A name is still optional, but if it's provided it must:
I'd be willing to adjust these restrictions if anyone has any opinions on what should or should not be allowed.
Link to tracking issue
Fixes #10673
Testing
Unit tests
Documentation
Code comments