cmeeren / Facil

Facil generates F# data access source code from SQL queries and stored procedures. Optimized for developer happiness.
MIT License
140 stars 7 forks source link

Facil generator fails when stored procedure contains merge statement (odd issue) #39

Closed boggye closed 2 years ago

boggye commented 2 years ago

Hi,

I spent at least 2 hours on this one.

What do I want to do? I want to generate the code that calls a stored procedure that processes data that is shoved in a sql server temp table.

Here are the files that can help you reproduce the issue:

TempTable1.sql:

CREATE TABLE #TempTable1
(
    Id int NOT NULL PRIMARY KEY
  , Name varchar(30) NOT NULL
);

This file goes under some directory containing sql (I put it under a folder called CreateTable).

in the yaml I have under procedures:

      - include: (?i)^dbo\.spTestFacil$
        tempTables:
          - definition: CreateTable/TempTable1.sql

Here is the stored procedure code:

-- File Name: dbo.spTestFacil.sql
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

CREATE OR ALTER PROCEDURE dbo.spTestFacil @psParam nvarchar(50)
as
begin

  DROP TABLE IF EXISTS #TmpJtsSnapshot;

  CREATE TABLE #TmpJtsSnapshot
  (
    Id int NOT NULL PRIMARY KEY
  , Name  varchar(30) NOT NULL

  );

  INSERT INTO #TmpJtsSnapshot ( Id, Name )
  values (1, 'Item 1')

  MERGE INTO #TmpJtsSnapshot AS Target
  USING
  (
    SELECT Id
         , Name
      FROM #TempTable1
  ) AS Source
     ON ( Target.Id = Source.Id )
   WHEN MATCHED THEN
    UPDATE SET Target.Name = Source.Name

   WHEN NOT MATCHED BY TARGET THEN
    INSERT ( Id, Name)
    VALUES
      ( Source.Id, Source.Name );

end
go

When I build the project I get these errors:

   1>Facil : error : System.Exception: Error getting output columns for stored procedure dbo.spTestFacil
          ---> Microsoft.Data.SqlClient.SqlException (0x80131904): Invalid object name '#TempTable1'.
            at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
            at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
            at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
            at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
            at Microsoft.Data.SqlClient.SqlDataReader.TryConsumeMetaData()
            at Microsoft.Data.SqlClient.SqlDataReader.get_MetaData()
            at Microsoft.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)
            at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, Task& task, Boolean asyncWrite, Boolean inRetry, SqlDataReader ds, Boolean describeParameterEncryptionRequest)
            at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method)
            at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method)
            at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior)
            at Facil.Db.getColumnsFromQuery(RuleSet cfg, FSharpChoice`3 executable, SqlConnection conn) in C:\projects\facil\src\Facil.Generator\Db.fs:line 336
            at Facil.Db.getColumns(SqlConnection conn, RuleSet cfg, FSharpMap`2 sysTypeIdLookup, FSharpChoice`3 executable) in C:\projects\facil\src\Facil.Generator\Db.fs:line 403

I was curious and I used the sql server profiler to check what facil executes and it looks like facil executes exec dbo.spTestFacil @psParam=N'1' twice for whatever reason, but before it runs the second time it creates a new connection and it does not create the temp tables.

It's very odd. Any idea?

For now, to move on, I am going to comment out the bodies of the stored procedures, run the generator, and them add them back. It is a pain but it works.

Thanks

Update: I forgot to include the usual info: .net 6.0, latest version of facil as of today (2.5.4), windows 2016 server. I use rider.net 2022.1.2, sql server 2017.

cmeeren commented 2 years ago

Thanks for the detailed report and minimal repro, I'll try to have a look at it during the weekend or next week.

boggye commented 2 years ago

Thank you!!

cmeeren commented 2 years ago

Fix published in v2.5.5.

I was curious and I used the sql server profiler to check what facil executes and it looks like facil executes exec dbo.spTestFacil @psParam=N'1' twice for whatever reason, but before it runs the second time it creates a new connection and it does not create the temp tables.

It's very odd. Any idea?

The bug was that it did not create the temp table the second time (on the second connection).

Opening two connections and executing the sproc twice is as designed. In order to get the result set metadata, Facil tries the following in succession (stopping at whatever works):

  1. sp_describe_first_result_set
  2. Execute sproc/script with SET FMTONLY ON
  3. Execute sproc/script normally

If step 3 is needed, it is performed on a separate connection/transaction (which is rolled back) to ensure there are no changes to the database. The bug was that the creation of the temp tables was not performed in step 3, so if a stored procedure (or script) needed this approach and used temp tables, they would fail with the error you encountered.

boggye commented 2 years ago

Hi, thinking more, just an idea.

It would be best for step 3 if you can allow the users of the library to specify the test sql statement in the configuration yaml file. The reason for this is that providing some parameter random value can lead to an execution branch that raises an exception, especially if the code in the procedure takes a defensive approach and checks thoroughly the parameter values to make sure they comply with expected values. The developer or the owner of the procedure know exactly what the procedure does and can provide parameter values that will return the expected runtime structure (the happy path).

What do you think? I know it is more work, but it makes your library sturdier.

Thanks

boggye commented 2 years ago

I will create a separate request.

cmeeren commented 2 years ago

Feel free, but I am unlikely to add it (was writing a response here, will wait for the separate issue instead), so don't spend too much time on it.