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

Allow developers to specify the test sql statement used to infer the structure of the resultset returned when standard procs don't work #40

Closed boggye closed 2 years ago

boggye commented 2 years ago

Sometimes using sp_describe_first_result_set stored procedure doesn't return accurate results, and/or the structure (or the absence of it) of the resultset returned by a stored procedure cannot be inferred.

It would be nice if the library allows the developer to specify in the configuration yaml file a test sql statement (execute spTest 'validParameterValue') that facil can use as a last resort in the determining the structure of the sql output from a stored procedure or sql script. This avoids hitting branches in the stored procedure execution that return errors, due for instance, to parameter checking, and it allows the developer to override the default parameter values used by facil to discover the structure of the data.

Thanks

smoothdeveloper commented 2 years ago

Not sure if with result sets can be used: https://docs.microsoft.com/en-us/sql/t-sql/language-elements/execute-transact-sql?view=sql-server-ver16

It is useable in places where you have dynamic SQL, but not sure if it is usable where it is plain SQL.

cmeeren commented 2 years ago

If I understand your suggestion correctly, this is already supported using the buildValue parameter. You can read more about it here in the readme.

Currently it only supports string parameters. It is probably possible, though non-trivial, to add support for other parameter types. (I originally considered and scrapped that due to implementation complexity.) No-one has ever requested that, so I will wait until someone has a real-world use-case that actually needs it. When that time comes, it should be raised in a separate issue.

Feel free to close this issue if you find it is resolved, or provide more details if not. 🙂

boggye commented 2 years ago

Yes, it looks like the buildValue option does ultimately what I had in mind. But just to confirm my understanding, if I have a stored procedure with parameters, let's say @p1, @p2, etc. through this mechanism I can assign values to these parameters by specifyng the buildValue parameter, then facil will use these values to call the stored procedure? In other words, if I have these :

- for: spTest
  params:
    p1:
      buildValue: 1
    p2:
      buildValue: 'Some string'
    p3:
      buildValue: null

will Facil execute: exec spTest @p1 = 1, @p2 = 'Some string', @p3 = null ?

Thank you

cmeeren commented 2 years ago

You are correct, except that as I mentioned above, it currently only supports string parameters.

boggye commented 2 years ago

Sorry, one more comment: it would be great if you can find some time to extend this feature to other parameter types. Imo, it is easier just allow the developer to specify a pass through sql that facil can run to get the structure of the dataset.

cmeeren commented 2 years ago

it would be great if you can find some time to extend this feature to other parameter types

That is a feature that makes good sense. And indeed, time is the limiting factor. For that reason, as mentioned, I may prioritize this once someone actually needs it. (Not just a hypothetical example; I can think of those myself. Someone should actually need this before I consider it. I don't want to spend time implementing something that's "nice to have" but that it turned out no-one actually used.)

Imo, it is easier just allow the developer to specify a pass through sql that facil can run to get the structure of the dataset.

A custom SQL query used to infer results is a nice "escape hatch", but one that I think isn't actually needed (given that buildValue can be used). Furthermore, using such a SQL query means that your app may fail at runtime if the actual runtime query gets out of sync with the design-time query. That very much runs counter to the purpose of Facil, which is all about compile-time safety. Not to mention the additional implementation and maintenance complexity in Facil itself for such a feature.

All in all, I am highly unlikely to add this. A feature request for this would have to present a very convincing use-case without any good workarounds.

boggye commented 2 years ago

Sounds good. In general, my principle has been to offer developers as much flexibility as possible to solve any issue that might come up. As a library writer, it is impossible to foresee all the issues the users of your library might run into, and I think it is important to leave enough doors open to solve anything.

And thanks for fixing this issue so quickly!

cmeeren commented 2 years ago

As a library writer, it is impossible to foresee all the issues the users of your library might run into, and I think it is important to leave enough doors open to solve anything.

Yes, this is a premise that also guides the development of "my" libraries and frameworks. 👍 However, it must be weighed up against library implementation/maintenance costs (at least for single-maintainer spare-time projects like Facil) and whether the feature promotes or is detrimental to good client code. (Shooting yourself in the foot and all that.)