views:

373

answers:

4

I've got the following trigger;

CREATE TRIGGER trFLightAndDestination
ON checkin_flight
AFTER INSERT,UPDATE
AS
BEGIN
    IF NOT EXISTS
    (
                    SELECT 1 
            FROM Flight v
            INNER JOIN Inserted AS i ON i.flightnumber = v.flightnumber
            INNER JOIN checkin_destination AS ib ON ib.airport = v.airport
            INNER JOIN checkin_company AS im ON im.company = v.company
            WHERE i.desk = ib.desk AND i.desk = im.desk
    )
    BEGIN
        RAISERROR('This combination of of flight and check-in desk is not possible',16,1)
        ROLLBACK TRAN       
    END
END

What i want the trigger to do is to check the tables Flight, checkin_destination and checkin_company when a new record for checkin_flight is added. Every record of checkin_flight contains a flightnumber and desknumber where passengers need to check in for this destination. The tables checkin_destination and checkin_company contain information about companies and destinations restricted to certain checkin desks. When adding a record to checkin_flight i need information from the flight table to get the destination and flightcompany with the inserted flightnumber. This information needs to be checked against the available checkin combinations for flights, destinations and companies.

I'm using the trigger as stated above, but when i try to insert a wrong combination the trigger allows it. What am i missing here?

EDIT 1: I'm using the following multiple insert statement

INSERT INTO checkin_flight VALUES (5315,3),(5316,3),(5316,2)
//5315 is the flightnumber, 3 is the desknumber to checkin for that flight

EDIT 2: Tested a single row insert which isn't possible, then the error is being thrown correct. So it's the multiple insert which seems to give the problem.

+1  A: 

I'm not sure of your business logic, but you need to check that the query does the proper thing.

Your problem is the IF NOT EXISTS, if the condition is true for 1 of the 3 rows in INSERTED it does not exist. You need to convert it to find a problems row and use IF EXISTS then error out.

However, when in a trigger the best way to error out is:

RAISERROR()
ROLLBACK TRANSACTION
RETURN

I kind of doubt that the lack of a RETURN is your problem, but it is always best to include the three Rs when erroring out in a trigger.

KM
better to use TRY/CATCH of course
gbn
In my view explicit rollback is generally dangerous for a trigger because it ASSUMES an underlying behavior that may be unwarranted. There is a reason XACT_ABORT is disabled by default.
Einstein
@Einstein, I disagree. If my trigger detects a problem, I always want it to rollback. Perhaps it is just me and how I code, but I always want the rollback at the point of error, so I can insert into a log table what was going on. It is the responsibility of the calling code to realize that there was an error, and it can add additional info on the error log as it terminates as well. However I could see if you have a different error handling system that a rollback in the trigger could cause you an issue if you were not expecting it.
KM
@KM I agree with your handling, that's also how i'm coding now and trying to keep doing it, seems the most logical way if you don't want any corrupt data to be saved.
Rob
@Einstein: perhaps, if pre SQL Server 2005 and not using TRY/CATCH. if you use TRY/CATCH if does not abort the batch in the outer code any more. SQL 2k5 + TRY/CATCH makes trigger behaviour more "normal"
gbn
It is perfectly acceptable for an application to commit a transaction even if members of it have failed. Explicit rollback from triggers prevents this from working and leads to unecessary loss of synchronization of transaction status between application and RDBMS. The application should be logging errors and managing transactions :-) RAISERROR != ROLLBACK .. rollback is not neccessary to provide useful feedback at point of error.If you want XACT_ABORT then enable it .. the system will automatically rollback on ALL failures for you.
Einstein
+1  A: 

The problem is that your logic is allowing any insert that includes at least one valid set of values through. It will only fail if all of the inserted records are invalid, instead of if any of the inserted records are invalid.

Change your "IF NOT EXISTS(...)" to a statement "IF EXISTS(...)" and change your SELECT statement to return invalid flights.

eg:

IF EXISTS
(
                SELECT 1 
        FROM Flight v
        INNER JOIN Inserted AS i ON i.flightnumber = v.flightnumber
        LEFT JOIN checkin_destination AS ib ON ib.airport = v.airport
             AND i.desk = ib.desk
        LEFT JOIN checkin_company AS im ON im.company = v.company
             AND i.desk = im.desk
        WHERE (im.desk IS NULL OR ib.desk IS NULL)
)
BEGIN
    RAISERROR('This combination of of flight and check-in desk is not possible',16,1)
    ROLLBACK TRAN       
END
Chris Shaffer
Thanks chris! That did the trick, guess i need to train a bit more in logical thinking
Rob
One small question, why a left join and not an inner join? because desk can be empty?
Rob
I was making an assumption that the negative case was when the corresponding record in checkin_destination or checkin_company doesn't exist; Checking that something doesn't exist is done by LEFT JOINing to the table and then looking for no record (hence the "im.desk IS NULL" part). It is just an assumption, and you should tweak to correct it. The goal as I said is to make your query find any records in INSERTED that are invalid.
Chris Shaffer
+1  A: 

The inserted table can contain multiple rows and therefore all logic within a trigger MUST be able to apply to all rows. The idea triggers must fire once per row effect is a common misunderstanding WRT triggers. SQL Server will tend to coalesce calls to a trigger to increase performance when they occur within the same transaction.

To fix you might start with a COUNT() of inserted and compare that with a COUNT() of the matching conditions and raise an error if there is a mismatch.

Einstein
shouldn't the select 1 and the combination of joins take care of that?
Rob
@Einstein said *SQL Server will tend to coalesce calls to a trigger to increase performance when they occur within the same transaction.* what? so if I do `BEGIN TRANSACTION; INSERT YourTable VALUES(...);INSERT YourTable VALUES(...); INSERT YourTable VALUES(...); COMMIT;` you're saying that SQL Server will only call the trigger one time? no way!
KM
I don't know the exact semantics but yea its pretty cool :)
Einstein
+1  A: 

The problem is that the condition will be true if only one of the inserted records are correct. You have to check that all records are correct, e.g.:

if (
  (
    select count(*) from inserted
  ) = (
    select count(*) from flight v
    inner join inserted i ...
  )
) ...
Guffa
Ah now i also get what einstein said :) Sounds logical indeed! I'll try it. is 'if not' the right way to revert the expression?
Rob
Your count indeed did the trick, but so did Chris Shaffers solution. Thanks for helping out!
Rob
@Rob: Right, you need to check for the difference in count, so you would use the `<>` operator instead of `=`.
Guffa