Merge pull request #294 from authorizerdev/fix/sql-unique-phone-constraint

fix(server): unique constraint for phone_number on mssql

Author: lakhansamani

server/db/models/user.go

@@ -25,7 +25,7 @@ type User struct {
 	Nickname                 *string `json:"nickname" bson:"nickname" cql:"nickname" dynamo:"nickname"`
 	Gender                   *string `json:"gender" bson:"gender" cql:"gender" dynamo:"gender"`
 	Birthdate                *string `json:"birthdate" bson:"birthdate" cql:"birthdate" dynamo:"birthdate"`
-	PhoneNumber              *string `gorm:"unique" json:"phone_number" bson:"phone_number" cql:"phone_number" dynamo:"phone_number"`
+	PhoneNumber              *string `gorm:"index" json:"phone_number" bson:"phone_number" cql:"phone_number" dynamo:"phone_number"`
 	PhoneNumberVerifiedAt    *int64  `json:"phone_number_verified_at" bson:"phone_number_verified_at" cql:"phone_number_verified_at" dynamo:"phone_number_verified_at"`
 	Picture                  *string `json:"picture" bson:"picture" cql:"picture" dynamo:"picture"`
 	Roles                    string  `json:"roles" bson:"roles" cql:"roles" dynamo:"roles"`

server/db/providers/sql/provider.go

@@ -1,7 +1,6 @@
 package sql
 
 import (
-	"fmt"
 	"time"
 
 	"github.com/authorizerdev/authorizer/server/constants"
@@ -71,35 +70,43 @@ func NewProvider() (*provider, error) {
 		return nil, err
 	}
 
+	// For sqlserver, handle uniqueness of phone_number manually via extra db call
+	// during create and update mutation.
+	if sqlDB.Migrator().HasConstraint(&models.User{}, "authorizer_users_phone_number_key") {
+		err = sqlDB.Migrator().DropConstraint(&models.User{}, "authorizer_users_phone_number_key")
+		logrus.Debug("Failed to drop phone number constraint:", err)
+	}
+
 	err = sqlDB.AutoMigrate(&models.User{}, &models.VerificationRequest{}, &models.Session{}, &models.Env{}, &models.Webhook{}, models.WebhookLog{}, models.EmailTemplate{}, &models.OTP{})
 	if err != nil {
 		return nil, err
 	}
 
+	// IMPACT: Request user to manually delete: UQ_phone_number constraint
 	// unique constraint on phone number does not work with multiple null values for sqlserver
 	// for more information check https://stackoverflow.com/a/767702
-	if dbType == constants.DbTypeSqlserver {
-		var indexInfos []indexInfo
-		// remove index on phone number if present with different name
-		res := sqlDB.Raw("SELECT i.name AS index_name, i.type_desc AS index_algorithm, CASE i.is_unique WHEN 1 THEN 'TRUE' ELSE 'FALSE' END AS is_unique, ac.Name AS column_name FROM sys.tables AS t INNER JOIN sys.indexes AS i ON t.object_id = i.object_id INNER JOIN sys.index_columns AS ic ON ic.object_id = i.object_id AND ic.index_id = i.index_id INNER JOIN sys.all_columns AS ac ON ic.object_id = ac.object_id AND ic.column_id = ac.column_id WHERE t.name = 'authorizer_users' AND SCHEMA_NAME(t.schema_id) = 'dbo';").Scan(&indexInfos)
-		if res.Error != nil {
-			return nil, res.Error
-		}
-
-		for _, val := range indexInfos {
-			if val.ColumnName == phoneNumberColumnName && val.IndexName != phoneNumberIndexName {
-				// drop index & create new
-				if res := sqlDB.Exec(fmt.Sprintf(`ALTER TABLE authorizer_users DROP CONSTRAINT "%s";`, val.IndexName)); res.Error != nil {
-					return nil, res.Error
-				}
-
-				// create index
-				if res := sqlDB.Exec(fmt.Sprintf("CREATE UNIQUE NONCLUSTERED INDEX %s ON authorizer_users(phone_number) WHERE phone_number IS NOT NULL;", phoneNumberIndexName)); res.Error != nil {
-					return nil, res.Error
-				}
-			}
-		}
-	}
+	// if dbType == constants.DbTypeSqlserver {
+	// 	var indexInfos []indexInfo
+	// 	// remove index on phone number if present with different name
+	// 	res := sqlDB.Raw("SELECT i.name AS index_name, i.type_desc AS index_algorithm, CASE i.is_unique WHEN 1 THEN 'TRUE' ELSE 'FALSE' END AS is_unique, ac.Name AS column_name FROM sys.tables AS t INNER JOIN sys.indexes AS i ON t.object_id = i.object_id INNER JOIN sys.index_columns AS ic ON ic.object_id = i.object_id AND ic.index_id = i.index_id INNER JOIN sys.all_columns AS ac ON ic.object_id = ac.object_id AND ic.column_id = ac.column_id WHERE t.name = 'authorizer_users' AND SCHEMA_NAME(t.schema_id) = 'dbo';").Scan(&indexInfos)
+	// 	if res.Error != nil {
+	// 		return nil, res.Error
+	// 	}
+
+	// 	for _, val := range indexInfos {
+	// 		if val.ColumnName == phoneNumberColumnName && val.IndexName != phoneNumberIndexName {
+	// 			// drop index & create new
+	// 			if res := sqlDB.Exec(fmt.Sprintf(`ALTER TABLE authorizer_users DROP CONSTRAINT "%s";`, val.IndexName)); res.Error != nil {
+	// 				return nil, res.Error
+	// 			}
+
+	// 			// create index
+	// 			if res := sqlDB.Exec(fmt.Sprintf("CREATE UNIQUE NONCLUSTERED INDEX %s ON authorizer_users(phone_number) WHERE phone_number IS NOT NULL;", phoneNumberIndexName)); res.Error != nil {
+	// 				return nil, res.Error
+	// 			}
+	// 		}
+	// 	}
+	// }
 
 	return &provider{
 		db: sqlDB,

server/db/providers/sql/user.go

@@ -2,12 +2,15 @@ package sql
 
 import (
 	"context"
+	"fmt"
+	"strings"
 	"time"
 
 	"github.com/authorizerdev/authorizer/server/constants"
 	"github.com/authorizerdev/authorizer/server/db/models"
 	"github.com/authorizerdev/authorizer/server/graph/model"
 	"github.com/authorizerdev/authorizer/server/memorystore"
+	"github.com/authorizerdev/authorizer/server/refs"
 	"github.com/google/uuid"
 	"gorm.io/gorm"
 	"gorm.io/gorm/clause"
@@ -27,6 +30,12 @@ func (p *provider) AddUser(ctx context.Context, user models.User) (models.User,
 		user.Roles = defaultRoles
 	}
 
+	if user.PhoneNumber != nil && strings.TrimSpace(refs.StringValue(user.PhoneNumber)) != "" {
+		if u, _ := p.GetUserByPhone(ctx, refs.StringValue(user.PhoneNumber)); u != nil {
+			return user, fmt.Errorf("user with given phone number already exists")
+		}
+	}
+
 	user.CreatedAt = time.Now().Unix()
 	user.UpdatedAt = time.Now().Unix()
 	user.Key = user.ID
@@ -47,6 +56,12 @@ func (p *provider) AddUser(ctx context.Context, user models.User) (models.User,
 func (p *provider) UpdateUser(ctx context.Context, user models.User) (models.User, error) {
 	user.UpdatedAt = time.Now().Unix()
 
+	if user.PhoneNumber != nil && strings.TrimSpace(refs.StringValue(user.PhoneNumber)) != "" {
+		if u, _ := p.GetUserByPhone(ctx, refs.StringValue(user.PhoneNumber)); u != nil && u.ID != user.ID {
+			return user, fmt.Errorf("user with given phone number already exists")
+		}
+	}
+
 	result := p.db.Save(&user)
 
 	if result.Error != nil {
@@ -141,3 +156,14 @@ func (p *provider) UpdateUsers(ctx context.Context, data map[string]interface{},
 	}
 	return nil
 }
+
+func (p *provider) GetUserByPhone(ctx context.Context, phoneNumber string) (*models.User, error) {
+	var user *models.User
+	result := p.db.Where("phone_number = ?", phoneNumber).First(&user)
+
+	if result.Error != nil {
+		return user, result.Error
+	}
+
+	return user, nil
+}

server/test/add_email_template_test.go

@@ -51,8 +51,7 @@ func addEmailTemplateTest(t *testing.T, s TestSetup) {
 			assert.Nil(t, emailTemplate)
 		})
 
-		var design string
-		design = ""
+		design := ""
 
 		for _, eventType := range s.TestInfo.TestEmailTemplateEventTypes {
 			t.Run("should add email template with empty design for "+eventType, func(t *testing.T) {
@@ -70,29 +69,7 @@ func addEmailTemplateTest(t *testing.T, s TestSetup) {
 				assert.NoError(t, err)
 				assert.Equal(t, et.EventName, eventType)
 				assert.Equal(t, "Test email", et.Subject)
-				assert.Equal(t, "Test design", et.Design)
-			})
-		}
-
-		design = "Test design"
-
-		for _, eventType := range s.TestInfo.TestEmailTemplateEventTypes {
-			t.Run("should add email template for "+eventType, func(t *testing.T) {
-				emailTemplate, err := resolvers.AddEmailTemplateResolver(ctx, model.AddEmailTemplateRequest{
-					EventName: eventType,
-					Template:  "Test email",
-					Subject:   "Test email",
-					Design:    &design,
-				})
-				assert.NoError(t, err)
-				assert.NotNil(t, emailTemplate)
-				assert.NotEmpty(t, emailTemplate.Message)
-
-				et, err := db.Provider.GetEmailTemplateByEventName(ctx, eventType)
-				assert.NoError(t, err)
-				assert.Equal(t, et.EventName, eventType)
-				assert.Equal(t, "Test email", et.Subject)
-				assert.Equal(t, "Test design", et.Design)
+				assert.Equal(t, "", et.Design)
 			})
 		}
 	})