Sc 12283/db validation #86
Conversation
|
This pull request has been linked to Shortcut Story #12283: Add Validate method to the db Topic model and validate ProjectID and topic name in the CreateTopic function.. |
pkg/tenant/db/members.go
Outdated
|
|
||
| if strings.ContainsAny(memberName, " !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~") { | ||
| return ErrSpecialCharacters | ||
| } |
There was a problem hiding this comment.
If we are doing similar validation for the models, I'm wondering if we should put these checks into a separate method so that if we have to add or change validation checks in the future we don't have to make the same change to three different places.
Better yet, we could utilize a regular expression like this. This would allow us to more accurately specify what the acceptable names are since regex is basically designed for user input validation. However that can probably be a separate story.
There was a problem hiding this comment.
Since the methods have been added to each model, I decided to keep doing that for now. Also, the models could change.
The link didn't work for me and I wasn't able to find exactly what was referenced. Could you try resending?
There was a problem hiding this comment.
Can you create a follow up story to focus on refactoring the name checks in a way that reduces the repeated code between the different files? I can add some details on the regex there.
I do think it's a good idea to have different validation methods for the different models (to reduce coupling between them), but we might be able to standardize the input checking a bit.
pkg/tenant/db/members.go
Outdated
|
|
||
| // Validates data in the member model when a TenantID is required. | ||
| // Ex. CreateTenantProject and UpdateProject | ||
| func (m *Member) ValidateWithID() error { |
There was a problem hiding this comment.
What do you think about combining Validate and ValidateWithID into one method and just passing in a boolean which indicates whether or not the ID should be validated?
There was a problem hiding this comment.
We want to have a TenantID in the models, but this then causes errors with the server-side CreateMember and CreateProject handlers. I created a ValidateID method that only checks for the ID to eliminate the redundant code. Although, this might not exactly be the most optimal solution at the moment.
There was a problem hiding this comment.
@pdeziel For now validation will only be applied to TenantCreateMember, TenantCreateProject, and ProjectTopicCreate.
pkg/tenant/db/members_test.go
Outdated
| err = db.CreateMember(ctx, member) | ||
| require.NoError(err, "could not create member") | ||
|
|
||
| require.NotEqual("", member.ID, "expected non-zero ulid to be populated") |
There was a problem hiding this comment.
I think you can also use require.NotEmpty which is slightly cleaner.
There was a problem hiding this comment.
Thank you for making the changes; this is so much clearer to me as a reader of the code. I have one small suggestion to add a quick test case to make sure the empty string check is working and another suggestion to add a follow on refactoring story but once those are complete feel free to merge!
|
|
||
| if memberName == "" { | ||
| return ErrMissingMemberName | ||
| } |
There was a problem hiding this comment.
Nice! Let's add a test case for this and to the other models so that we protect ourselves against regressions when we make future changes to the validation code.
Scope of changes
Adds validation method to the db package for the member, project, and topic models that will be used when creating or updating these resources. The
membersandprojectsdb required two validation methods as they have 2 create methods and one requires aTenantIDwhile another doesn't.Minor updates to server-side handlers and tests were required.
Note: Validation may be added to the tenant model in the future.
Fixes SC-12283
Type of change
Acceptance criteria
Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible.
Author checklist
Reviewer(s) checklist