CODEWITHSUNDEEP
sql-servert-sqltriggersidentitymaintainability
Scott
Scott

Reputation: 29

Maintainable INSTEAD OF trigger with Identity columns

I want to add a trigger to my table to validate the data before allowing insert or update.

For example, something like this:

CREATE TABLE MyHorribleLegacyTable (i int IDENTITY, name char(100),
 column2 char(2), column3 date, column4 int, ... column224 int)
CREATE TRIGGER ValidateLicense
ON MyHorribleLegacyTable
INSTEAD OF INSERT
AS 
BEGIN
    SET NOCOUNT ON;

    IF UserDefinedFunction(inserted) = 1
        THROW 9283, 'The zinger is not configured to work with the wok', 16
    IF EXISTS (SELECT * FROM inserted WHERE Name IN ('100001', 'None'))
        THROW 9284, 'Invalid Licence Number', 16

    INSERT INTO MyHorribleLegacyTable
        SELECT * FROM inserted
END

Unfortunately, this table has an identity column, so SQL Server throws an error

An explicit value for the identity column in table 'dbo.Licences' can only be specified when a column list is used and IDENTITY_INSERT is ON.

The 'fix' is to change the INSERT statement to explicitly list all the columns (except the identity column). But this is a maintenance nightmare.

What happens when another developer adds a column (with a constraint) into the table and does not check every trigger on the table. Any code that attempts to insert a non-default value into this new column does not work as the new column is not explicitly listed in the INSERT statement.

Is there a better way to do the above that is maintainable?

A constraint is not possible because of the complexity.

Assuming well written code elsewhere in the system, adding the new column to the table does not break anything except the trigger code.

Upvotes: 1

Views: 80

Answers (3)

Thom A
Thom A

Reputation: 95489

user_0 as noted what you should be doing off the back of my comment; creating a CONSTRAINT. I wanted to touch on the problems with your attempt.

Firstly, you assume that any indented statement will be interpreted as part of your IF; T-SQL isn't Python and that isn't how it works. For a multi-line IF you need to use a BEGIN...END. With your attempt your INSERT/UPDATE won't do anything as the RETURN is always reached before the INSERT. See this fiddle for evidence: db<>fiddle.

CREATE TABLE dbo.SomeTable (I int IDENTITY, SomeString varchar(10));
GO
CREATE TRIGGER dbo.SomeTrigger ON dbo.SomeTable INSTEAD OF INSERT, UPDATE AS
BEGIN

    SET NOCOUNT ON;

    IF EXISTS (SELECT 1 FROM inserted WHERE Inserted.SomeString = 'A')
        RAISERROR('Invalid Licence Number', 16, 1);
        RETURN;

    INSERT INTO dbo.SomeTable (SomeString)
    SELECT SomeString
    FROM Inserted;
END;
GO
INSERT INTO dbo.SomeTable (SomeString)
VALUES('A'),('B');
GO
INSERT INTO dbo.SomeTable (SomeString)
VALUES ('C');
GO
UPDATE dbo.SomeTable
SET SomeString = 'Z'
WHERE I = 3;
GO
SELECT *
FROM dbo.SomeTable;
I SomeString

If we fix that, and change the IF to use a BEGIN...END, we still have flaws; your TRIGGER is on INSERT, UPDATE but your TRIGGER just inserts. That means that if you UPDATE a row won't actually UPDATE it; it'll INSERT a "new version" of it, and the old will remain in place. This can, again, be seen with this db<>fiddle

CREATE OR ALTER TRIGGER dbo.SomeTrigger ON dbo.SomeTable INSTEAD OF INSERT, UPDATE AS
BEGIN

    SET NOCOUNT ON;

    IF EXISTS (SELECT 1 FROM inserted WHERE Inserted.SomeString = 'A')
        THROW 50001, N'An attempt to store the value ''A'' in the column ''SomeString'' was made.',16;

    INSERT INTO dbo.SomeTable (SomeString)
    SELECT SomeString
    FROM Inserted;
END;
GO
INSERT INTO dbo.SomeTable (SomeString)
VALUES('A'),('B');
GO
INSERT INTO dbo.SomeTable (SomeString)
VALUES ('C');
GO
UPDATE dbo.SomeTable
SET SomeString = 'Z'
WHERE I = 3;
GO
SELECT *
FROM dbo.SomeTable;
I SomeString
1 C
2 D
3 E
4 Z

Notice that the row with a value of 3 for I still has a value of 'E', and a separate row for 'Z' was created with a value of 4 for I.

You should handle your INSERT and UPDATEs separately here.

And, finally, to bang the drum, THROW an error, rather than using RAISERROR. Let the outer scope then handle the rollback of the transaction. as needed. But again, a trigger isn't needed here at all; use a CONSTRAINT.

Upvotes: 1

David Browne - Microsoft
David Browne - Microsoft

Reputation: 88852

The 'fix' is to change the INSERT statement to explicitly list all the columns

You should be doing that anyway. It's not a maintenance nightmare at all; it's just good practice.

If you don't want to type out the column list you can use SSMS and drag the "columns" folder from the object explorer to the query window. Or generate it with a query like

select object_name(c.object_id) tn, string_agg(quotename(c.name),', ')  cols
from sys.columns c
group by c.object_id

Upvotes: 2

user_0
user_0

Reputation: 3363

As Thom suggested in a comment, I also suggest to use a constraint. If I remember ritght, it must also be faster. In your case:

ALTER TABLE Licences
ADD CONSTRAINT CK_Licences_Name
CHECK (Name NOT IN ('100001', 'None'));

Upvotes: 1

Related Questions