0

I'm running into an issue where I need to Save a copy of a record into a "History" table with the original field values of the updated record before I do the update.

I'm using a trigger to do this. The trigger INSERTED value should contain the row field values after the update and DELETED should show the field values before the update.

However, whenever the example trigger code runs, the return of the SELECT always returns null for the ArticleID, Title, StepContent, and SortOrder parameters. However if I were to change the line of code where it pulls from DELETED to pull from INSERTED it works just fine. 

ALTER TRIGGER [dbo].[TRG_SaveHistory]
ON [dbo].[ArticleStep]
AFTER UPDATE
AS
BEGIN
SET NOCOUNT ON;


DECLARE @SortOrder INT, 
        @ArticleID INT, 
        @ArticleHistoryID INT, 
        @Title varchar(max), 
        @StepContent varchar(max);

--SELECT @SortOrder = SortOrder, @Title = Title, @StepContent = StepContent, @ArticleID = ArticleID FROM INSERTED;
SELECT TOP (1) @SortOrder = SortOrder, @Title = Title, @StepContent = StepContent, @ArticleID = ArticleID FROM DELETED WHERE @SortOrder != null;
SELECT TOP (1) @ArticleHistoryID = ID FROM dbo.ArticleHistory WHERE OriginalArticleID = @ArticleID ORDER BY ID DESC;

IF  @SortOrder IS NOT Null AND @ArticleHistoryID IS NOT NULL
    INSERT INTO [dbo].[ArticleStepHistory]
               ([ArticleHistoryID]
               ,[SortOrder]
               ,[Title]
               ,[StepContent])
         VALUES
               (@ArticleHistoryID
               ,@SortOrder
               ,@Title
               ,@StepContent);
END

Works when I change this: 

SELECT @SortOrder = SortOrder, @Title = Title, @StepContent = StepContent, @ArticleID = ArticleID FROM DELETED;

To this: 

SELECT @SortOrder = SortOrder, @Title = Title, @StepContent = StepContent, @ArticleID = ArticleID FROM INSERTED;

Here is the update query I am running: 

UPDATE [dbo].[ArticleStep] SET [ArticleID] = 2 ,[SortOrder] = 3,[StepContent] = 'Changed Text' WHERE ID = 1
TroySteven
  • 4,885
  • 4
  • 32
  • 50
  • 4
    You are making a fatal error in your trigger if you assume that `inserted` or `deleted` has only one row. You should ask another question about how to properly write a trigger in SQL Server. – Gordon Linoff Jun 18 '19 at 15:19
  • Would it be as simple as adding WHERE SortOrder != null to pull the record I want then? – TroySteven Jun 18 '19 at 15:21
  • I was using the second answer to this question as a guide. https://stackoverflow.com/questions/642822/how-can-i-do-a-before-updated-trigger-with-sql-server – TroySteven Jun 18 '19 at 15:22
  • 2
    Keep in mind that the deleted and inserted row sets may have more than one row in them. Be sure to treat them as sets. – user1443098 Jun 18 '19 at 15:22
  • 3
    BTW, Starting with 2016 version you can use system-versioned tables and let SQL Server do the heavy lifting for you instead of writing triggers for this. – Zohar Peled Jun 18 '19 at 16:57
  • 1
    Aside; Using [`sp_settriggerorder`](https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-settriggerorder-transact-sql?view=sql-server-2017) to set the firing order to `'Last'` for a logging trigger is a good idea. If another trigger causes a `rollback` then you haven't already logged the `update`, – HABO Jun 18 '19 at 17:29
  • Thanks for the information, I am on SQL Server 2014 so if anyone has any insight on why this particular trigger isn't working that would be great. – TroySteven Jun 18 '19 at 17:31
  • 1
    @matwonk Not without an example of your update statement, the rows that it updates (before and after), and a better definition of your goal. It really isn't meaningful to discuss the logic of a trigger which is known to have a major fault - that is the single row assumption. In addition, your logic depends on the existence of associated row(s) in the history table in order to insert "valid" values into that same table. I think it's time to simply start over and reconsider your entire approach. At the very least, more information is needed. – SMor Jun 18 '19 at 18:05
  • Thanks for the feedback, I've updated the question to include the update statement I am running. This is an addition to a larger database with other functionality that works like this, I have to follow this model and use a trigger. Let me know if you need additional information. Thanks – TroySteven Jun 18 '19 at 18:17
  • 1
    I did make an attempt at fixing your trigger. But your changes now make that attempt incomprehensible to other readers. Again - need to know the DDL for your tables. Now it appears that your step table has a PK of ID. Is ArticleID a FK to another ("higher level") table? Given the table name, are Articles composed of one or more steps? – SMor Jun 18 '19 at 18:46
  • Have you looked at using the trigger`BEFORE UPDATE` ? – tgolisch Jun 18 '19 at 18:52
  • @Smor, I just switched the code back, I was trying to simplify to get a better answer. – TroySteven Jun 18 '19 at 19:13
  • 1
    @tgolisch - That doesn't exist in SQL Server – TroySteven Jun 18 '19 at 19:14

1 Answers1

2

So here is a wild guess at what your trigger might look like: 

alter trigger dbo.TRG_SaveHistory
   on dbo.ArticleStep after update 
as
begin 
set nocount on;

if not exists (select * from deleted) 
    return;

with cte as (
  select max(ID) as HistoryID, OriginalArticleID 
    from dbo.ArticleHistory 
   group by OriginalArticleID)
insert dbo.ArticleStepHistory (ArticleHistoryID, SortOrder, Title, StepContent)
select cte.HistoryID, deleted.SortOrder, deleted.Title, deleted.StepContent
from deleted inner join cte 
  on deleted.ArticleID = cte.OriginalArticleID; 

end;

It is a complete guess. You might need to adjust it for minor syntax errors since I don't have your database and wrote that using notepad. If you're writing tsql code you should be able (and are expected to be able) to do correct for minor errors.

It is important to know the primary keys for ArticleStep, the primary and foreign keys (even if the FK is not enforced) for your history table, and how all these rows are supposed to relate to their original values and perhaps to other "higher level" rows. It is more than a little odd that you need to retrieve an ID from the history table in order to add rows to it. That implies you have (or should have) an insert trigger. If you do, it likely also suffers from the same single-row assumption.

But it really should not be necessary to "look up" an id from the history table in order to insert more rows. I haven't seen that sort of need in other logging situations. You should think carefully about why you think this is needed.

Title is varchar(max)? I'd rethink that as well. Perhaps this will give you the push towards a better solution.

SMor
  • 2,830
  • 4
  • 11
  • 14