0

How do I modify this registration code so it checks if email entered already exists in the database? I already have a query written for it, but I don't know how to implement it

[HttpPost("Register")]
        public async Task<ActionResult<User>> Register(UserDto request, Guid guid)
        {
            string query = @"
                           insert into dbo.Users(UserID,Name,Email,PasswordHash,PasswordSalt)
                           values (@UserID,@Name,@Email,@PasswordHash,@PasswordSalt)
                            ";
            string emailValidationQuery = @"SELECT * FROM dbo.Users WHERE Email = @Email";

            CreatePasswordHash(request.Password, out byte[] passwordHash, out byte[] passwordSalt);
            string psw = PasswordHash(request.Password);

            Guid guid1 = Guid.NewGuid();
            guid = guid1;

            user.UserID = guid;
            user.Username = request.Username;
            user.Email = request.Email;
            user.PasswordHash = Encoding.UTF8.GetBytes(psw);
            user.PasswordSalt = passwordSalt;

            DataTable table = new DataTable();
            string sqlDataSource = _configuration.GetConnectionString("ContactAppCon");
            SqlDataReader myReader;
            using (SqlConnection myCon = new SqlConnection(sqlDataSource))
            {
                myCon.Open();
                using (SqlCommand myCommand = new SqlCommand(query, myCon))
                {
                    myCommand.Parameters.AddWithValue("@UserID", Guid.NewGuid());
                    myCommand.Parameters.AddWithValue("@Name", request.Username);
                    myCommand.Parameters.AddWithValue("@Email", request.Email);
                    myCommand.Parameters.AddWithValue("@PasswordHash", psw);
                    myCommand.Parameters.AddWithValue("@PasswordSalt", passwordSalt);
                    myReader = myCommand.ExecuteReader();
                    table.Load(myReader);
                    myReader.Close();
                    myCon.Close();
                }
            }
            return Ok(user);
        }
Shadow
  • 33,525
  • 10
  • 51
  • 64
Youve
  • 1
  • We can't know, as we don't know where you store the mail-adresses. Please be more specific on your problem. What exactly should happen and what happens instead? – MakePeaceGreatAgain Oct 18 '22 at 11:01
  • @MakePeaceGreatAgain I guess it currently inserts the user regardless the existence of an entry with same email. OP needs to check if it already exists, and only insert it if not – Rafalon Oct 18 '22 at 11:05
  • You have already framed a query in the variable `emailValidationQuery`, execute that first and only if that returns zero records, then proceed with your insert query. – Anand Sowmithiran Oct 18 '22 at 11:13
  • Please be careful with tagging your question as you are using ms sql server and not mysql! – Shadow Oct 18 '22 at 11:38
  • 2
    can't the `email` column be set `unique` and just catch the relevant exception should the insertion violates the constraint? – Bagus Tesa Oct 18 '22 at 11:41
  • 2
    Side note, [AddWithValue is evil](https://www.dbdelta.com/addwithvalue-is-evil/). For your `INSERT` it (likely) won't cause any problems, but for your `SELECT` statement (`SELECT * FROM dbo.Users WHERE Email = @Email`) if `Email` isn't an `n(var)char` then your query isn't going to be SARGable, and thus will perform poorly. – Thom A Oct 18 '22 at 11:44
  • As for what you have `COUNT`ing the number of rows, and then if you get a `0` `INSERT`ing it is going to result in race conditions when you have no kind of locking. A `CONSTRAINT`, like @BagusTesa mentions, would be your best solution here. – Thom A Oct 18 '22 at 11:45
  • The solution @BagusTesa suggested worked. Thank you very much. I don't think I can mark a comment as an answer. – Youve Oct 18 '22 at 12:26
  • Actually from my reading seems [it is better to check first than to blindly let exceptions happen](https://www.mssqltips.com/sqlservertip/2632/checking-for-potential-constraint-violations-before-entering-sql-server-try-and-catch-logic/). – Stuck at 1337 Oct 18 '22 at 12:30
  • @RhythmWasaLurker op will need to use [`transaction`](https://stackoverflow.com/a/58619563) then to avoid race condition. i'm suggesting `unique` index due to the fact it involves fewest changes (hence easiest). ¯\\_(ツ)_/¯ op can choose whatever floats the boat i guess. so far none wrote up using transaction in the answer, perhaps you can add one. also, that article is for `catch` block in sql side, what i'm referring to is exception on c# side. – Bagus Tesa Oct 18 '22 at 12:34
  • @BagusTesa What is wrong with using a transaction and avoiding race conditions? My answer shows how to do that in a single statement (a statement _is_ a transaction even if you don't explicitly say `BEGIN TRANSACTION;`). And it doesn't matter where you _catch_ the exception, the exception is still costly on the database side - it has to process that regardless of where you catch it. – Stuck at 1337 Oct 18 '22 at 12:41

2 Answers2

0

Try something like below (open and dispose the connections properly)

string emailValidationQuery = @"SELECT Count(*) FROM dbo.Users WHERE Email = @Email";
....

using SqlCommand command = new SqlCommand(emailValidationQuery, myCon);
int count = (Int32) command.ExecuteScalar();

if(count > 0)
    return new User() // or whatever you required
WisdomSeeker
  • 854
  • 6
  • 8
0

Why not a single statement:

INSER INTO dbo.Users (UserID, Name, Email, ...)
  VALUES (@UserID, @Name, @Email, ...)
WHERE NOT EXISTS
(
  SELECT 0 
    FROM dbo.Users WITH (SERIALIZABLE, UPDLOCK)
    WHERE Email = @Email
);

If this affects 0 rows (you can check with @@ROWCOUNT), then the e-mail already existed (and maybe you should run an update instead in that case, but it's not clear from the question) or the insert failed for some other reason (which you can check with simple try/catch patterns).

And you can prevent race conditions and avoid costly exceptions by doing it a little differently:

BEGIN TRANSACTION;

IF NOT EXISTS 
(
  SELECT 0 FROM dbo.Users WITH (SERIALIZABLE, UPDLOCK)
  WHERE Email = @Email
)
BEGIN
  INSERT ...
END
ELSE
BEGIN
  -- UPDATE? RAISERROR? Again requirements aren't clear.
END

COMMIT TRANSACTION;

Don't go for simple or expensive when correct and more efficient are better.

Stuck at 1337
  • 1,900
  • 1
  • 3
  • 13