-
Notifications
You must be signed in to change notification settings - Fork 2.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
Validate TXT prefix #1507
Validate TXT prefix #1507
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,26 @@ func testTXTRegistryNew(t *testing.T) { | |
_, err = NewTXTRegistry(p, "txt", "txt", "owner", time.Hour) | ||
require.Error(t, err) | ||
|
||
_, err = NewTXTRegistry(p, "@invalid#", "", "owner", time.Hour) | ||
require.Error(t, err, `invalid TXT prefix provided, expected "[a-z0-9-.]+" got "@invalid#"`) | ||
_, err = NewTXTRegistry(p, "", "@invalid#", "owner", time.Hour) | ||
require.Error(t, err, `invalid TXT suffix provided, expected "[a-z0-9-.]+" got "@invalid#"`) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I add
That also looks invalid to me, but the test fails. Are you sure the check that we are doing is fine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then we need to improve the regex that is used. Any suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
_, err = NewTXTRegistry(p, "txt", "", "owner", time.Hour) | ||
require.NoError(t, err) | ||
_, err = NewTXTRegistry(p, "", "txt", "owner", time.Hour) | ||
require.NoError(t, err) | ||
|
||
_, err = NewTXTRegistry(p, "TxT", "", "owner", time.Hour) | ||
require.NoError(t, err) | ||
_, err = NewTXTRegistry(p, "", "TxT", "owner", time.Hour) | ||
require.NoError(t, err) | ||
|
||
r, err = NewTXTRegistry(p, "-sub-.", "", "owner", time.Hour) | ||
require.NoError(t, err) | ||
r, err = NewTXTRegistry(p, "", "-sub-.", "owner", time.Hour) | ||
require.NoError(t, err) | ||
|
||
_, ok := r.mapper.(affixNameMapper) | ||
require.True(t, ok) | ||
assert.Equal(t, "owner", r.ownerID) | ||
|
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.
This message and the one on like 66 are not accurate, I recommend changing it like:
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.
Why are they not accurate? By comparing the error string I make sure that the guard that returns the error works. If I just accept any error, it would make it less good, right?
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.
Because what is happening when you have an error is not that the TXT prefix is invalid, rather than it is expected to be invalid, but there is no error. The string you are providing is only changing the messaging to report, not changing any behavior at all. The default message instead, is correct.
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.
@ruudk any thoughts on the suggestion from @Raffo