findmypast-oss / mssql_ecto

Ecto adapter for Mssqlex
Apache License 2.0
49 stars 20 forks source link

DML statement cannot have any enabled triggers if the statement contains an OUTPUT clause without INTO clause. #27

Open LoganBresnahan opened 6 years ago

LoganBresnahan commented 6 years ago

Problem:

When a database table has a trigger attached to it, insert/1 will fail because the adapter is not utilizing an INTO clause.

This blog post from the SQL Server team explains the issue well: https://blogs.msdn.microsoft.com/sqlprogrammability/2008/07/11/update-with-output-clause-triggers-and-sqlmoreresults/

Error Example:

m=%{
  client_sport_id: 1, 
  user_id_entered: 1, 
  tracking_type: "TT-mon", 
  subject: "Incoming Call", 
  notes: "This is a note.", 
  tracking_date: "2017-09-14T11:21:39.592873Z",
  entry_date: "2017-09-14T11:21:39.592873Z"
}

cs=OpsMessagingService.ClientTracking.changeset(%OpsMessagingService.ClientTracking{},m)

OpsMessagingService.Repo.insert(cs)

result:

insert/1 error:
** (Mssqlex.Error) [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]The target table 'client_tracking' of the DML statement cannot have any enabled triggers if the statement contains an OUTPUT clause without INTO clause. | ODBC_CODE 42000 | SQL_SERVER_CODE 334
    (ecto) lib/ecto/adapters/sql.ex:571: Ecto.Adapters.SQL.struct/7
    (ecto) lib/ecto/repo/schema.ex:469: Ecto.Repo.Schema.apply/4
    (ecto) lib/ecto/repo/schema.ex:205: anonymous fn/13 in Ecto.Repo.Schema.do_insert/4

DB Trigger Example:

CREATE TRIGGER [dbo].[briu_client_tracking] on [dbo].[client_tracking]
  for INSERT,UPDATE
  as
begin
  declare  
           @icnt int,
           @dcnt int,
           @errno   int,
           @errmsg  varchar(255)

   begin
     select @icnt = count(*) from inserted;
     select @dcnt = count(*) from deleted;
     IF @icnt > 0 and @dcnt=0
       begin
         update client_tracking
         set entry_date=current_timestamp,
          mod_date=current_timestamp
          where client_tracking_id in (select client_tracking_id from inserted)

       end
     IF @dcnt > 0 
        begin 
          update client_tracking
          set mod_date=current_timestamp
                where client_tracking_id in (select client_tracking_id from deleted)

        end
    end
return      

end

Temporary Workaround:

Even if you're only looking to insert one record, you can use insert_all/2 and pass in your one entry because it does not rely on a return value from the database. https://hexdocs.pm/ecto/Ecto.Repo.html#c:insert_all/3

Environment:

iDarshn commented 6 years ago

I am facing the same issue. Do you guys have any update on it?

harri88 commented 6 years ago

any update for this issue?

jbachhardie commented 6 years ago

I've just come back from a month's holiday and things are a bit less hectic now than they were for the first half of the year so I'm going to try to dedicate at least a few hours a week to updating this library. I'll prioritise merging PRs, since my personal Elixir skills are rather rusty at this point, but if nobody else is picking this issue up I'll get to it.

cpursley commented 6 years ago

I just ran into this issue as well. I'd be glad to try my hand at patching this if someone could provide some pointers on where to start in this codebase.

This seems related:

cpursley commented 6 years ago

It looks like making inserted an optional parameter is one possible solution: https://github.com/findmypast-oss/mssql_ecto/blob/master/lib/mssql_ecto/query.ex#L107

Unfortunately, we won't be able to get the inserted data back with this approach.

cpursley commented 6 years ago

Another alternative might be to check if a trigger exists on a table and dynamically add/remove the inserted bit: https://wateroxconsulting.com/archives/find-triggers-sql-server-database/

But perhaps this would add too much overhead?

jbachhardie commented 6 years ago

Does setting returning: false on the insert/2 call allow the query to complete? I think that might be the best we can do, allow the user to turn off OUTPUT. I don't want to have every DML statement have to make an extra query to figure out if there are triggers and I don't think we can get the ODBC adapter we depend on to do the multiline statement necessary to output into a temp table and then select from it (though I might be wrong there).

cpursley commented 6 years ago

I think you're right (allow the user to turn off OUTPUT).

Setting returning: false didn't work for me. Where is insert/2 defined? All I'm seeing is this def: https://github.com/findmypast-oss/mssql_ecto/blob/master/lib/mssql_ecto/query.ex#L98

jbachhardie commented 6 years ago

I meant Ecto.Repo.insert/2 which is how the end-user interacts with the adapter. I also think I got it wrong and "return nothing" is expressed as returning: [] rather than false, which I believe gets translated into the returning argument being [] in query.ex which looking at the code should remove the OUTPUT clause.

apurvaraut commented 4 years ago

In the db you can delete the trigger which is not in use. can solve this issue. I did delete the trigger from the table . and it worked fine.