Help with SQL Server procedure rollback

Question

I need some help with this procedure:

What its supposed to do is try to insert a new user if there is no other with same NAME.

If there is a already an user, it should rollback else commit. But it doesn't work, it commits anyway.

Any suggestions?

SET ANSI_NULLS ON
SET QUOTED_IDENTIFIER ON
GO

ALTER procedure [dbo].[SP_USUARIOS_INSERT]
@usu_ds varchar(50),
@usu_dt_create datetime,
@usu_dt_lst_log datetime,
@usu_ds_senha varchar(255),
@usu_ds_email varchar(100)
as
begin
declare @varCheckUser varchar(100) = null;
set @varCheckUser = (select COUNT(usu.usu_Ds) from Usuarios usu where usu.usu_ds = @usu_ds);
begin transaction
insert into Usuarios(usu_ds,usu_dt_create,usu_dt_lst_log,usu_ds_senha,usu_ds_email) values(@usu_ds,@usu_dt_create,@usu_dt_lst_log,@usu_ds_senha,@usu_ds_email)
if (@varCheckUser <> null)
begin
 RAISERROR('User already exists',16,1)
 rollback transaction
 return
end
else
begin
commit transaction
end
end

Answer 1

I don't think that @varCheckUser will ever be NULL, if there is no row it will be 0

set @varCheckUser = (select COUNT(usu.usu_Ds) 
from Usuarios usu where usu.usu_ds = @usu_ds);

this will make it 0

also you check like this for NULL

if (@varCheckUser IS NOT null)

why don't you do something like this

IF  EXISTS (select 1 
               from Usuarios usu 
                where usu.usu_ds = @usu_ds)
SET @varCheckUser =1

then check that it is not 1

why do you need the tran? Just do something like this

IF  EXISTS (select 1 
                   from Usuarios usu 
                    where usu.usu_ds = @usu_ds)
BEGIN
RAISERROR('User already exists',16,1)
RETURN
END
ELSE
BEGIN
insert into Usuarios(usu_ds,usu_dt_create,usu_dt_lst_log,usu_ds_senha,usu_ds_email)
values(@usu_ds,@usu_dt_create,@usu_dt_lst_log,@usu_ds_senha,@usu_ds_email)

END

Probably a good idea to make usu_ds a primary key or add a unique constraint, that way nobody can update their user name to something that exists and nobody can by mistake use SSMS and change a user name to something that is already in the table

Answer 2

@varCheckUser can never be null. It will always have a value that is a string representation of an integer.

Instead of:

if (@varCheckUser <> null)

Do:

if (@varCheckUser = 0)

Answer 3

You need to change your check to use @varCheckUser = 0 - or better yet, change it to use IF EXISTS and only ever begin a transaction to insert the values if that user doesn't already exist:

IF NOT EXISTS(SELECT * FROM dbo.Usuarios usu WHERE usu.usu_ds = @usu_ds)
BEGIN
   BEGIN TRANSACTION

   INSERT INTO 
      dbo.Usuarios(usu_ds, usu_dt_create, usu_dt_lst_log, usu_ds_senha, usu_ds_email)     
   VALUES(@usu_ds, @usu_dt_create, @usu_dt_lst_log, @usu_ds_senha, @usu_ds_email)

   COMMIT TRANSACTION
END

There's really no point in starting a transaction just to roll it back, if you can check for the existance of the user before hand.

Plus: if the column usu_ds should be unique, you ought to put a UNIQUE constraint on it, too! That way, if you'll get errors (constraint violations) if someone manages to try and insert a user some other way (other than through your stored proc):

ALTER TABLE dbo.Usuarios
  ADD CONSTRAINT UX_usu_ds UNIQUE(usu_ds)

Answer 4

@varCheckUser <> NULL will always return FALSE.

You must use @varChechUser IS NOT NULL

Answer 5

It doesn't have to that complicated.

ALTER procedure [dbo].[SP_USUARIOS_INSERT]
   @usu_ds varchar(50),
   @usu_dt_create datetime,
   @usu_dt_lst_log datetime,
   @usu_ds_senha varchar(255),
   @usu_ds_email varchar(100)
AS

SET NOCOUNT, XACT_ABORT ON 
INSERT Usuarios(usu_ds, usu_dt_create, usu_dt_lst_log, usu_ds_senha, usu_ds_email) 
SELECT @usu_ds, @usu_dt_create, @usu_dt_lst_log, @usu_ds_senha, @usu_ds_email
WHERE
   NOT EXISTS (
      SELECT 1
      FROM Usuarios WITH (UPDLOCK, HOLDLOCK)
      WHERE usu_ds = @usu_ds
   )
IF @@RowCount = 0 BEGIN
   RAISERROR('User already exists', 16, 1)
   RETURN
END

This code completely solves any concurrency problems for you as well (see Conditional Insert/Update Race Condition).