0

(SQL Server 2014 Express)

Hi,

I reference this article: http://msdn.microsoft.com/en-gb/magazine/cc164047.aspx

I have this AFTER INSERT, UPDATE, DELETE trigger to set or update DateCreated, DateModified, and WhoUpdatedID columns in several tables (delete processing to be added later):

CREATE TRIGGER [dbo].[TR_dim_TypeOfClaim_Audit]
    ON [dbo].[dim_TypeOfClaim]
    AFTER INSERT, UPDATE, DELETE
AS
    SET NOCOUNT ON
    DECLARE @event_type [char]

    --Get Event Type
    IF EXISTS(SELECT * FROM inserted)
    IF EXISTS(SELECT * FROM deleted)
        SELECT @event_type = 'U'
    ELSE
        SELECT @event_type = 'I'
    ELSE
    IF EXISTS(SELECT * FROM deleted)
        SELECT @event_type = 'D'
    ELSE
    --no rows affected - cannot determine event
        SELECT @event_type = 'K'

    IF @event_type = 'I' BEGIN
        --Date Created
        UPDATE t
            SET DateCreated = GETDATE()
            FROM INSERTED e
            JOIN [dbo].[dim_TypeOfClaim] t ON e.[TypeOfClaimID] = t.[TypeOfClaimID]
      ;
      SELECT @event_type = 'U' --also do UPDATE processing
    END

    IF @event_type = 'U' BEGIN
        --Date Modified
        UPDATE t
            SET DateModified = GETDATE()
            FROM INSERTED e
            JOIN [dbo].[dim_TypeOfClaim] t ON e.[TypeOfClaimID] = t.[TypeOfClaimID]

        --WhoModifiedID
        UPDATE t
            SET WhoModifiedID = u.UserID
            FROM INSERTED e
            JOIN [dbo].[dim_TypeOfClaim] t ON e.[TypeOfClaimID] = t.[TypeOfClaimID]
            JOIN [dbo].[dim_Users] u ON u.[Username] = dbo.udfUserName()
    END

    IF @event_type = 'D' BEGIN
        no_op:  --Nothing for now
    END
GO

The integrity rules for DateCreated, DateModified, and WhoUpdatedID is NOT NULL. Which is technically correct, but are never input by the end user. This causes errors when new records are added.

Should I just change these columns to NULL allowed, or to change the trigger to INSTEAD of? I'm not sure the best practice here.

If it matters, I intend to implement full auditing of all data changes later, either via triggers or change tracking (I need to read up on change tracking to see if it meets my needs).

Thanks for the help...

Update:

@Bogdan: Sorry, my comment wasn't clear. Based on everyone's feedback so far, this is what I currently have:

1) I've left DateCreated, DateModified, and WhoModifiedID as NOT NULL

2) I've created default values as getdate(), getdate(), and 0 respectively. All values are just to get past the NOT NULL constraints for a new record. I suppose one could argue that, with the trigger, the NOT NULL constraint is redundant - the trigger would ensure the values are NOT NULL anyway. I'm a bit unclear in this regard what is best practice. But I don't want these columns to ever be NULL, and the constraint makes this obvious.

3) My current trigger is now:

ALTER TRIGGER [dbo].[TR_dim_InjuryType_Audit]
    ON [dbo].[dim_InjuryType]
    AFTER INSERT, UPDATE
AS
    SET NOCOUNT ON

    DECLARE @CurrentUserID INT;
    SELECT  @CurrentUserID = u.UserID 
    FROM    [dbo].[dim_Users] U
    WHERE   u.[Username] = dbo.udfUserName()

    UPDATE  T
    SET     DateModified = GETDATE(),
          WhoModifiedID = @CurrentUserID
  FROM    INSERTED E
    JOIN    [dbo].[dim_InjuryType] T ON e.[InjuryTypeID] = t.[InjuryTypeID]

4) BTW, dbo.udfUserName() is merely:

-- =============================================
-- Author:      Scott Bass
-- Create date: 04JUL2014
-- Description: Return userid without the domain
-- =============================================
ALTER FUNCTION [dbo].[udfUserName]
(
)
RETURNS varchar(20)
AS
BEGIN
    -- Declare the return variable here
    DECLARE @UserName varchar(20)

    -- Add the T-SQL statements to compute the return value here
    SELECT @UserName = substring(suser_sname(),charindex('\',suser_sname())+1,99)

    -- Return the result of the function
    RETURN @UserName
END  

5) Thanks for the hint re: not using EDIT TOP 200 ROWS in SSMS

This is working as I desire. Further comments on improvements/best practice more than welcome, plus will help those that find this thread later.

I'm getting an annoyance in my Access front end that is perhaps related to the triggers but really is a separate subject for a separate post. However, I include this link for completeness, in the chance someone is interested: http://www.utteraccess.com/forum/User-Edited-Record-Sav-t2019558.html

Scott
  • 169
  • 1
  • 3
  • 14

2 Answers2

1

I would use / I've used following solutions (1) and 2.1)):

1) DateCreated is mandatory: NOT NULL plus a default constraint using GETDATE()

ALTER TABLE [dbo].[dim_TypeOfClaim]
ADD CONSTRAINT DF_dim_TypeOfClaim_DateCreated DEFAULT (GETDATE()) FOR DateCreated

Setting DateCreated using GETDATE() as default value is a better solution (in terms of performance) than using an AFTER INSERT trigger with an UPDATE ... SET DateCreated = GETDATE() ....

2.1) DateModified and WhoModifiedID should be NULLable and if you are using [only] stored procedures to UPDATE the rows from [dbo].[dim_TypeOfClaim] then I would change these procedures to update, also, the DateModified and WhoModifiedID columns. This way, you could delete this AFTER UPDATE trigger. The downside of this approach is that if someone run an ad-hoc script that UPDATE dim_TypeOfClaim rows could easily forget to update ,also , DateModified and WhoModifiedID columns.

or

2.2) DateModified and WhoModifiedID should be NULLable plus an AFTER UPDATE trigger:

-- tr = trigger, U = INSERT only trigger
ALTER TRIGGER [dbo].[trU_dim_TypeOfClaim_Audit] 
    ON [dbo].[dim_TypeOfClaim]
    -- This trigger should be activated only by UPDATE statements (or MERGE ... UPDATE SET ...)
    -- This trigger shouldn't be activated by INSERT or DELETE statements
    AFTER UPDATE 
AS
BEGIN
    DECLARE @CurrentUserID INT;
    SELECT @CurrentUserID = u.UserID 
    FROM   [dbo].[dim_Users] u 
    WHERE  u.[Username] = @CurrentUserName;

    UPDATE  t
    SET     DateModified = GETDATE(),
            WhoModifiedID = @CurrentUserID
    FROM    INSERTED e
    JOIN    [dbo].[dim_TypeOfClaim] t ON e.[TypeOfClaimID] = t.[TypeOfClaimID]

END
Bogdan Sahlean
  • 19,233
  • 3
  • 42
  • 57
  • Ok, what I've done is 1) left the columns as NOT NULL, 2) added default values of getdate(), getdate(), and 0 to DateCreated, DateModified, and WhoModifiedID respectively. I removed the above INSERT trigger, but changed the 2nd one to if @event_type IN ('I','U'). I still need the insert trigger because of the complexity of the WhoModifiedID processing (stores foreign key). However, when I run an edit query in SSMS, I get the message "However, a problem occurred when trying to retrieve the data back after the commit". How can I get rid of that msg? Refreshing the query works fine. – Scott Jul 05 '14 at 14:54
  • Also, I'm using an Access front end with linked tables to SQL Server back end. The updates are via Access, but I'd like DateCreated, DateModified, and WhoModifiedID derived on the server. I'm not using SP's to update the data, and not sure how I would do so from the Access front end. – Scott Jul 05 '14 at 14:56
  • @Scott: (1) According to your trigger, `WhoModifiedID` column is modified only when rows are updated (`@event_type = 'U'`). So, when `@event_type = 'I'` only `DateCreated` column is updated and a default constraint for `DateCreated` should be enough. From this point of view, only an `AFTER UPDATE` trigger is necessary. (2) You might get that error when using `Edit TOP 200 rows` (SSMS). I never use this option to edit rows. Instead, I use only T-SQL statements. – Bogdan Sahlean Jul 05 '14 at 17:16
0

It doesn't make sense (to me) to have DateModifed and WhoUpdatedID as NOT NULL, since they will always be NULL when a record is inserted.

In my opinion, only DateCreated should be NOT NULL, and it should be set by the database, either by a trigger or Stored Proc.

DeanOC
  • 7,142
  • 6
  • 42
  • 56
  • First of all, I'm new to SQL Server, so keep that in mind ;) I set them NOT NULL since they should never be NULL; I need to track (and display) when the record is created, updated, and by who. However, if I've added a trigger to set/update them, then perhaps best practice is to remove the NOT NULL constraint? But the way I read the article, it seemed to dissuade using triggers to enforce constraints. – Scott Jul 05 '14 at 09:29
  • 1
    I normally just use a default constraint for create date and don't insert a value. It would also be possible to use the permissions system to prevent an explicit value being inserted other than the default. – Martin Smith Jul 05 '14 at 09:29
  • @Scott In my opinion, `DateModified` and `WhoUpdatedID` should be NULL until such time as the record is actually modified. To put values into them when nothing has changed would be misleading. – DeanOC Jul 05 '14 at 09:51
  • @MartinSmith The use of a default constraint for create date isn't something that I've considered. So you can specify that this should be the current sys date? I've always gone with setting the value in my Insert SP. – DeanOC Jul 05 '14 at 09:53
  • @Dean IMO this is analogous to creating a file on Windows, but leaving DateModified as null. The behavior I want: When the record is inserted, DateCreated and DateModified are equal, and WhoModified is whoever entered the record. When the record is edited, DateModified is updated, as well as whoever modified the record. – Scott Jul 05 '14 at 10:20
  • @MartinSmith To clarify: Is that RMB Database -> Design -> click DateCreated -> set Default Value or Binding = GETDATE()??? Then set permissions so the column is readonly? And this is better than the AFTER INSERT trigger? (I'll be protecting the column anyway, either in the database, the front end, or both). – Scott Jul 05 '14 at 10:30
  • 1
    @Scott - Yes that's right on the default. I don't bother setting the permissions myself I just ensure that insert code doesn't set a value. I imagine that you could use column permissions to do this. If that fails you can provide a view that does not contain the column. You then provide `INSERT` and `UPDATE` permissions on the view then deny those permissions on the base table. Default constraints have less overhead than triggers. – Martin Smith Jul 05 '14 at 10:34