Added a new static parameter ProcResulstSets to allow config the with result sets clause

[erlis]

Hi,

With this change I was able to use the provider to call stored procedures that use temp tables. Here I'm creating a new static parameter ProcResultSets to configure a dictionary with the values that will be passed to the sys.sp_describe_first_result_set procedure.

One challenge was that I couldn't pass a Map as a static parameter so I created a dictionary using the following string syntax: key1:val1|key2:val2|... I'm not happy with this part of the code and I'll really like to find a better way.

Here is an example of how I'm using it now in my project:

let ConnectionString = @"Data Source=(local);Initial Catalog=MyDatabase; Integrated Security=True"
type Db = SqlProgrammabilityProvider<
               ConnectionString,
               ProcResultSets = "dbo.usp_SalesforceData_Create:with result sets none|
                                 Staging.GetCompaniesForImigit:with result sets none|
                                 Staging.usp_SetRabbitMessageProcess_Upd:with result sets none|
                                 Staging.usp_CompanyObject_ProcessSPSMessages:with result sets none|
                                 staging.usp_CompanyObject_ProcessUTEMessages:with result sets none|
                                 staging.usp_CompanyObject_MappingToDBAmp:with result sets none|
                                 staging.usp_CompanyObject_SendToSF:with result sets none|
                                 staging.usp_CompanyObject_ToDBAmp:with result sets none|
                                 staging.usp_CompanyObject_MappingToSF:with result sets none|
                                 staging.usp_CompanyObject_PrepareTaxEngineDB:with result sets none">

this is in response to my comment in #302

UPDATE: Including the feedback on the UX aspect of this feature:

I'm writing a code generator that will give me compile time validation for all the procs in my database using the TP. After I generated the code I noticed that a handful of procs that were using temp table for performance improvements were giving compilation error

The generator will generate code similar to this:

    let  usp_PdfFiles_Get (conn: SqlConnection) ids = 
        use sproc = new Db.dbo.usp_PdfFiles_Get (conn)
        sproc.AsyncExecute ids 

the error reads: The metadata could not be determined because statement XXXX uses a temp table. This is the result on the dependency on the proc sp_describe_first_result_set

So as it was, the TP was imposing a big restriction on my codebase. I tried with other type providers with no luck, so one idea was to provide a way to bypass the temp table validation per stored proc.

I don't know if this is what you were looking to understand but, do not hesitate to let me know and I'll reply as soon as possible

[smoothdeveloper]

Hi @erlis, sorry I didn't see your last comment (#302 (comment)) until now.

I'd like to gather more feedback on the UX aspect of this additional feature.

To make this easier, could you add in the PR description some sample code that you had to write before the feature and how it looks after, to better understand the annoyance and double check possible other work arounds that wouldn't require the feature?

I'll give some comments in the code nonetheless.

Thanks in anycase!

[smoothdeveloper]

I'd rename this mapFromString to something more descriptive of the usage of this code ("why/what") rather than "how" it is done, for example parseResultSetsDeclarationList, and then make splitKeyVal local to that function.

[erlis]

I don't know if passing the string is the correct way, I'd prefer to pass a Map to as the static parameter but I couldn't find how. Maybe is a constrain in what type can be passed. I'm not really happy passing the string with the dictionary syntax.

Also I'm not sure if the syntax I use is the best, I was considering also:
key1->value1, key2->value2.... but again, this is not relevant, what is really important is to pass the information to the TP so it knows when to append to the sys.sp_describe_first_result_set command.

If we ended up using this with a string this code maybe even needs to be moved to a better place, this was just a way to keep the change small while proving that something like this might work.

[smoothdeveloper]

For now AFAIK, the only things that can be passed to a TP as static parameters are literals (string, numbers and enum), we are indeed stuck to string and figuring out a format that makes the most sense.

Workarounds for now:

• make a sql command as I was describing, basically containing the same text you are running through sys.sp_describe_first_result_set
• changing the stored procedures involving temp tables to run the code as dynamic SQL and add the with result sets inside

I'm doing the later in most cases but I understand it makes maintenance of those procedures a more annoying.

[smoothdeveloper]

After some more exchanges in the PR discussion, we will need unit tests and if possible some additions to the doc pages.

[smoothdeveloper]

@erlis have you found a way to define with result sets in the stored procedure itself?

In my case, I'm basically relying on dynamic SQL so I can add the with result sets clause idiomatically in the stored procedure.

You can also wrap a call to a sp in a dynamic call and add the with result sets there.

I'm not sure there is ideal answer, but I believe an exclude list as input to your code generator, or simply excluding code that wouldn't compile based on that error could be an effective work around.

For now the suggestion doesn't feel useable / maintainable enough to me, to make it worth extending the library that way, I believe there are valid work arounds that pretty much require the same amount of input on user end but don't add complexity to the library.

Please let me know if that makes sense or update us if you are going to explore this idea further, thanks again for this first round of investigation.

[erlis]

@smoothdeveloper, this is what I did, and I'm using my fork with great success:

I've modified the type provider in a way that I can overwrite the result set of any stored procedure using the syntax below.

Here I'm posting the definition of what I'm using today. I do have a lot of stored procs, but this limitation only affects a few of them, for me is not an issue to add another line here if I need to..

The change for this is in my fork but I haven't merged with the main branch and I see github detecting conflict... It will be great if something like this is added to the official release, in the mean time I'm using my fork in my code...

I don't understand why my suggestion was rejected the first time I presented it. I'm not too familiar with the project structure, I did the minimum number of changes possible to get this added, but I remember you guys asking for tests, and I think is great to have the tests too but I'm not that familiar with the project.

Please let me know if this makes sense!
--Erlis

open FSharp.Data
open System.Data.SqlClient

[<Literal>]
let ConnectionString = @"Data Source=(local);Initial Catalog=XXXXX;Integrated Security=True"
type Db = SqlProgrammabilityProvider<
                ConnectionString,
                ProcResultSets = "garnishment.usp_SalesforceData_Create:with result sets none|
                                  Staging.GetCompaniesForImigit:with result sets none|
                                  Staging.usp_SetRabbitMessageProcess_Upd:with result sets none|
                                  Staging.usp_CompanyObject_ProcessSPSMessages:with result sets none|
                                  staging.usp_CompanyObject_ProcessUTEMessages:with result sets none|
                                  staging.usp_CompanyObject_MappingToDBAmp:with result sets none|
                                  staging.usp_CompanyObject_SendToSF:with result sets none|
                                  staging.usp_CompanyObject_ToDBAmp:with result sets none|
                                  staging.usp_CompanyObject_MappingToSF:with result sets none|
                                  staging.usp_CompanyObject_PrepareTaxEngineDB:with result sets none|
                                  pts.usp_SalesforceData_Input_Merge:with result sets none|
                                  pts.usp_SalesforceData_InputExceptions_Merge: with result sets none|
                                  staleCheck.usp_Salesforce_Merge: with result sets none|
                                  staleCheck.usp_ReportingEvent_Merge: with result sets none|
                                  pts.usp_SalesforceData_Import_Merge: with result sets none|
                                  ftr.usp_SalesforceData_Merge: with result sets none |
                                  staleCheck.usp_GenerateEmailForCaseFailure: with result sets ((val varchar(max)))|
                                  staleCheck.usp_GenerateEmailForBankTransactionWithoutGarnishment: with result sets ((val varchar(max)))|
                                  staleCheck.usp_GarnishemntReadyForPaymentToGarnishment_Merge: with result sets none">

[smoothdeveloper]

@erlis it makes a lots of sense.

The way I see it now, is we could integrate your PR, but we need a bit of effort spent on adding tests and a bit of documentation; and ideally enhancing the design for broader consumption.

I'll go over the code at some point, but I think this is in good shape since:

• you have been using it in your fork
• it feels relevant and is delineated for now to the SqlProgrammabilityProvider

Extra niceties we should consider for this to make it robust for long term:

• Have an extra SqlResultSetFile provider, which does syntax validation, encourages moving that declaration in a text file outside F# code
• Looking further how this could interact with some of the recent feature additions, that go along the same lines around TVP and temp tables in adhoc queries / SqlCommandProvider
• make sure the internal implementation is well tested / documented if extra effort can be spent

[charlesroddie]

Related to #331 (comment)