1

I want to create a trigger to check if a record exist before insert, if it exists rollback, if not continue to do the insert. The thing is when I do the insert it always rollback. what should I do?

ALTER TRIGGER [dbo].[CHECKCONSOMMATION]
ON [dbo].[ConsommationEau]
FOR INSERT
AS
DECLARE @IDABONNEMENT INT
DECLARE @DEFMONTH DATETIME

SELECT @IDABONNEMENT = inserted.idAbonnement FROM inserted
SELECT @DEFMONTH = inserted.Periode FROM inserted


IF EXISTS (SELECT 1 FROM ConsommationEau WHERE idAbonnement = @IDABONNEMENT AND DATEDIFF(MONTH, Periode, @DEFMONTH) = 0)
    BEGIN
    RAISERROR('THIS RECORD IS ALREADY EXISTS', 10, 1)
    ROLLBACK
    RETURN
    END

and this my table.

     USE [GESTEAU]
      GO

 /****** Object:  Table [dbo].[ConsommationEau]    Script Date: 4/20/2017  
 :08:53 AM ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

 CREATE TABLE [dbo].[ConsommationEau](
[idConsomationEau] [int] IDENTITY(1,1) NOT NULL,
[Periode] [date] NULL,
[Qte] [int] NULL,
[idAbonnement] [int] NULL
) ON [PRIMARY]

GO

ALTER TABLE [dbo].[ConsommationEau]  WITH CHECK ADD FOREIGN 
KEY([idAbonnement])
REFERENCES [dbo].[AbonnementEau] ([idAbonnement])
 GO
  • Can you clarify the exact requirement? It looks like you're trying to limit the table to only one row, per `idAbonnement`, per month. Is that correct? (By the way, your current trigger is badly broken - `inserted` can contain multiple rows. You've no guarantee, in such cases, the `@IDABONNEMENT` and `@DEFMONTH` even come from the same row, let alone correctly deal with those multiple rows) – Damien_The_Unbeliever Apr 20 '17 at 13:21
  • Yes, exactly, I want to limit the table to only one row, per idAbonnement, per month. – MOUTIK ABDELKARIM Apr 20 '17 at 13:26

3 Answers3

4

If the intention is that the table can only contain one row, per idAbonnement, per month, then I wouldn't use a trigger. I'd use a persisted computed column and a unique index:

CREATE TABLE [dbo].[ConsommationEau](
[idConsomationEau] [int] IDENTITY(1,1) NOT NULL,
[Periode] [date] NULL,
[Qte] [int] NULL,
[idAbonnement] [int] NULL,
PeriodeMonth as DATEADD(month,DATEDIFF(month,0,Periode),0) persisted
) ON [PRIMARY]
GO
create unique index IX_ConsommationEau_Monthly
    on ConsommationEau (idAbonnement, PeriodeMonth)

This way you've declared what should be unique rather than having to write procedural code, and you've avoided the problem with your current trigger where it doesn't currently deal correctly with multi-row inserts.

(The DATEADD/DATEDIFF pair in the computed column definition just ensures that any date within a month gets converted to the 1st of that same month, and is my generally preferred means of adjusting date(time) values)


To do this inside a trigger, you have to realise that a FOR INSERT trigger is also known as an AFTER trigger. You'll always find a row that matches the just inserted row(s) since by the time the trigger fires, they've already been added to the table - the rows are matching themselves.

Normally, if you want to prevent rows being inserted, I'd recommend that you use an INSTEAD of trigger to prevent them ever appearing in the final table. However, that can become awkward - especially because it's easy to forget that the two conflicting rows might not be a newly inserted one and an existing one, but instead two newly inserted rows.

So, we'll stick with the FOR INSERT trigger. All we need is something like:

ALTER TRIGGER [dbo].[CHECKCONSOMMATION]
ON [dbo].[ConsommationEau]
FOR INSERT
AS
    IF EXISTS(
         SELECT * FROM ConsommationEau WHERE
            idAbonnement IN (SELECT idAbonnement FROM inserted)
         GROUP BY idAbonnement,DATEADD(month,DATEDIFF(month,0,Periode),0)
         HAVING COUNT(*) > 1)
    BEGIN
      RAISERROR('THIS RECORD IS ALREADY EXISTS', 10, 1)
      ROLLBACK
    END

(But note that this has now been transformed into a simple uniqueness test within the table, which as you'll observe, is more neatly enforced using the constructs designed for that, as used in my first solution above)

Damien_The_Unbeliever
  • 234,701
  • 27
  • 340
  • 448
  • I have no choice here, I've told to use the trigger, It's been week now looking for a proper answer. – MOUTIK ABDELKARIM Apr 20 '17 at 13:29
  • 1
    @hyungsik - I've done a trigger version, but I'd really not recommend it. You're effectively implementing a `UNIQUE` constraint by hand rather than letting the system use built in mechanisms - I don't see the value in that. – Damien_The_Unbeliever Apr 20 '17 at 13:41
  • Thank you so much, you're right. in a development environment I would definitely avoid using it, but now since I've told to only use trigger, I'll stick with your answer. – MOUTIK ABDELKARIM Apr 20 '17 at 13:45
  • No need to use an index, a simple unique constraint would be enough on the computed column, index maintenance costs on updates/inserts and may alter performances. – Hybris95 Apr 20 '17 at 13:45
  • @Hybris95 - a `unique` constraint is always *enforced* by an index being created (at least in SQL Server). You cannot avoid the index. So far as I'm aware, there's no *additional* cost in declaring it as a `unique constraint` but that's the only direction in which a difference could occur. – Damien_The_Unbeliever Apr 20 '17 at 13:47
1

Use the keywords BEGIN and END after your IF to make sure the ROLLBACK takes the IF in count.

IF EXISTS [condition]
BEGIN
    RAISERROR('Error',10,1)
    ROLLBACK
    RETURN
END

But mostly, I would suggest you using a check constraint in the table definition :
https://stackoverflow.com/a/2570810/3635715

Microsoft Reference :
https://learn.microsoft.com/en-us/sql/t-sql/statements/alter-table-column-constraint-transact-sql

Community
  • 1
  • 1
Hybris95
  • 2,286
  • 2
  • 16
  • 33
0

You should use an INSTEAD OF trigger to only insert if it is not matched:

ALTER TRIGGER [dbo].[CHECKCONSOMMATION]
ON [dbo].[ConsommationEau]
Instead of INSERT
AS
    MERGE ConsommationEau c
    USING inserted i
    on c.idAbonnement = i.idAbonnement AND DATEDIFF(MONTH, c.Periode, i.Periode) = 0
    WHEN NOT MATCHED BY TARGET THEN
        INSERT ([idConsomationEau, [Periode], [Qte], [idAbonnement]) 
        VALUES (i.[idConsomationEau, i.[Periode], i.[Qte], i.[idAbonnement])
cloudsafe
  • 2,444
  • 1
  • 8
  • 24
  • The issue with trying to do this as an `instead of` (assuming you correct the `on` to the required conditions, which also requires `Periode` to be taken into consideration) is that `inserted`, *itself*, may contain two conflicting rows. – Damien_The_Unbeliever Apr 20 '17 at 14:07
  • Merge is not a good technique in any event. Ii is very hard to troubleshoot when you have a data issue. It has also had some bug problems. – HLGEM Apr 20 '17 at 14:10
  • @Damien_The_Unbeliever I have fixed the ON conditions. If there are conflicting rows in the insert it poses the problem of which row should be inserted - a logical problem. – cloudsafe Apr 20 '17 at 14:18