Insert or update user data

Question

I inherited an application that uses some stored procedures. Here is a sample of one of the stored procedures used to insert or modify user data.

would like to get the group's opinion on the code. The SQL Server version is Microsoft SQL Server 2014 - 12.0.4100.1 (X64).

USE [MyDataBase]
GO
/****** Object: StoredProcedure [dbo].[InsertOrModifyUserData] Script Date: 2/2/2016 8:39:57 PM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER PROCEDURE [dbo].[InsertOrModifyUserData]
@userName NVARCHAR(200),
@isSuperAdmin BIT = NULL,
@modifiedBy NVARCHAR(200),
@modifiedDate DATETIME = NULL,
@isActive BIT = NULL
AS
BEGIN
 SET NOCOUNT ON; 
 SET XACT_ABORT ON; 
 BEGIN TRY
 BEGIN TRANSACTION
 IF (@modifiedDate IS NULL OR @modifiedDate ='1900-01-01 00:00:00.000')
 SET @modifiedDate = GETDATE()
 IF (@isActive IS NULL)
 SET @isActive = 1
 IF ((@userName IS NOT NULL OR @userName <>'') AND (@isSuperAdmin IS NOT NULL OR @isSuperAdmin<>'') AND (@modifiedBy IS NOT NULL OR @modifiedBy <>''))
 BEGIN 
 IF EXISTS(SELECT UserName FROM [dbo].[UserAccess] WHERE UserName = @userName)
 UPDATE [dbo].[UserAccess] 
 SET IsSuperAdmin = @isSuperAdmin,
 ModifiedBy = @modifiedBy,
 ModifiedDate = @modifiedDate,
 IsActive = @isActive 
 WHERE UserName = @userName 
 ELSE
 INSERT INTO [dbo].[UserAccess] (UserName, IsSuperAdmin, CreatedDate, ModifiedBy, ModifiedDate,IsActive)
 VALUES(@userName, @isSuperAdmin, GETDATE(), @modifiedBy, @modifiedDate, @isActive) 
 END 
 ELSE
 RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)
 COMMIT TRANSACTION
 select null
 END TRY
 BEGIN CATCH
 DECLARE @ErrorMessage NVARCHAR(MAX) = ERROR_MESSAGE();
 DECLARE @ErrorSeverity INT = ERROR_SEVERITY();
 DECLARE @ErrorState INT = ERROR_STATE();
 RAISERROR(@ErrorMessage, @ErrorSeverity, @ErrorState);
 ROLLBACK TRANSACTION;
 END CATCH;
END

Answer 1

Dead code

In this:

 COMMIT TRANSACTION
 select null
END TRY

This line of code: select null does nothing useful (read: nothing at all) and should be deleted. I have no idea why it is there to begin with, but querying for NULL with no criteria will always be null.

---

Errors not useful

You could improve this error reporting:

 ELSE
 RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)

TLDR: Don't filter error logs, just store stuff. Developers can filter them easily if they need to.

Why? As it is, this is raising an error with no information on "why" the error was raised. At the very least return the input parameters so the user/dev can decipher what is wrong. This is especially relevant if you are logging those errors somewhere.

You would want to include as much relevant information as possible in your errors, especially if there are multiple instances and databases involved. There are few things which hinder improvement and bug fixes more than half-arsed error logs. Log everything and let the developers sort it out.

Answer 2

Useless error handling.

Catching and re-raising the error isn't doing anything useful. And you actually are losing potentially valuable information about the line the error occurred.

Race conditions.

Unless you are running this at serializable isolation level (the default is read committed) there is nothing preventing two concurrent executions of this stored procedure with the same @userName both performing the check for existence at the same time - concluding it is not there and proceeding to insert a row, causing duplicates.

Bizarre checks

What is the purpose of (@userName IS NOT NULL OR @userName <>'')? There is no possible value that can evaluate to true for the second condition that is not already true for the first condition. If the purpose was "not null and not empty string" you would need (@userName IS NOT NULL AND @userName <>'') - though that can be simplified to just (@userName <> '') in the context you are using it as that won't evaluate to true anyway if @userName is null (it would evaluate to unknown).

This check is particularly bizarre for @isSuperAdmin<>'' as the datatype for this is bit not string. Due to the wonders of implicit casts this is actually evaluated as @isSuperAdmin<>0 but is a no-op anyway for the reason discussed earlier.

Non conventional custom severity level.

 RAISERROR('Required parameters ...',13,1)

Why 13? This generally represents deadlock. Use 16.

Rewrite.

CREATE PROCEDURE [dbo].[InsertOrModifyUserData] @userName NVARCHAR(200),
 @isSuperAdmin BIT = NULL,
 @modifiedBy NVARCHAR(200),
 @modifiedDate DATETIME = NULL,
 @isActive BIT = NULL
AS
 BEGIN
 SET NOCOUNT ON;
 /*Checks for null and empty string. Using IIF as negating unknown won't work as desired*/
 IF IIF(@userName <> ''
 AND @modifiedBy <> ''
 AND @isSuperAdmin IS NOT NULL, 1, 0) = 0
 BEGIN
 /*TODO: Consider making more informative*/
 RAISERROR('Missing values for one or more required parameters.',16,1); 
 RETURN;
 END
 IF ( @modifiedDate IS NULL
 OR @modifiedDate = '1900-01-01 00:00:00.000' )
 SET @modifiedDate = GETDATE();
 /*Using Merge to do all in one statement so no need for explicit transactions.
 HOLDLOCK still needed for race conditions though
 */ 
 MERGE [dbo].[UserAccess] WITH(HOLDLOCK) AS UA
 USING (VALUES (@userName, @isSuperAdmin, @modifiedBy, @modifiedDate, ISNULL(@isActive,1))) 
 AS Source(UserName, IsSuperAdmin, ModifiedBy, ModifiedDate, IsActive)
 ON UA.UserName = Source.UserName
 WHEN MATCHED THEN
 UPDATE SET UserName = Source.UserName,
 IsSuperAdmin = Source.IsSuperAdmin,
 ModifiedBy = Source.ModifiedBy,
 ModifiedDate = Source.ModifiedDate,
 IsActive = Source.IsActive
 WHEN NOT MATCHED THEN
 INSERT (UserName, IsSuperAdmin, CreatedDate, ModifiedBy, ModifiedDate,IsActive)
 VALUES (UserName, IsSuperAdmin, GETDATE(), ModifiedBy, ModifiedDate,IsActive); 
 END

• VincentZHANG: Is RAISERROR really suitable? it wont stop the following lines being executed.
• Martin Smith: @VincentZHANG - that's why it is followed by RETURN