-
Notifications
You must be signed in to change notification settings - Fork 671
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
MG-2296 - Domain creation allowed with nil alias #2297
Conversation
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
auth/postgres/init.go
Outdated
Up: []string{ | ||
`ALTER TABLE domains | ||
ALTER COLUMN alias SET NOT NULL, | ||
ADD CONSTRAINT alias_not_empty CHECK (alias <> '');`, |
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.
There is a need for this constraint. It makes sense, but for consistency with the rest of the codebase, let's have this check on API or svc layers.
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.
Additonal to this constraint, we can add the this check in API Layer, where we do data validation.
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.
Since you have added API validation, please remove it from the DB for the sake of consistency with the rest of the codebase.
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.
Okay
auth/postgres/init.go
Outdated
Up: []string{ | ||
`ALTER TABLE domains | ||
ALTER COLUMN alias SET NOT NULL, | ||
ADD CONSTRAINT alias_not_empty CHECK (alias <> '');`, |
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.
Additonal to this constraint, we can add the this check in API Layer, where we do data validation.
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
What type of PR is this?
This is a bug fix
What does this do?
This PR modifies the
alias
field in theauth
table to be NOT NULL and/or empty and UNIQUE. This prevents the creation of a domain with a NULL or empty alias, which was previously allowed.Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Yes, I have included tests for my changes.
Did you document any new/modified feature?
Notes