Make optional parameters required

[cmeeren]

Fixes #327

Untested.

[smoothdeveloper]

@cmeeren thanks for giving it a shot, there are few things that needs to be updated before I can merge this:

docs/content
configuration and Input.fsx
faq.fsx

the documentation may stipulate that omitting the optional parameters is not an option,

FSharp.Data.SqlClient/src/SqlClient.DesignTime/SqlCommandProvider.fs

Line 67 in 37a6ae5

<param name='AllParametersOptional'>If set all parameters become optional. NULL input values must be handled inside T-SQL.</param>

The tests also probably needs updating, I'm seeing calls to Execute that don't give the extra parameters, for example:

FSharp.Data.SqlClient/tests/SqlClient.Tests/OptionalParamsTests.fs

Line 31 in 37a6ae5

actual = cmd.Execute()

Can you add those changes to your PR?

[cmeeren]

How's that? :)

[smoothdeveloper]

@cmeeren thanks a bunch! going to fetch this and get it merged, I think over the weekend.

[cmeeren]

Perfect! Looking forward to the release.

[smoothdeveloper]

I've fixed compilation of few more tests, but there is one which is concerning myself:

CREATE PROCEDURE dbo.Echo(@in SQL_VARIANT = 'Empty')
AS
	SELECT @in;
GO

[<Fact>]
let SpWithParamOfRefTypeWithNullDefault() = 
    use echo = new AdventureWorks.dbo.Echo()
    Assert.Equal( Some (Some (box "Empty")), echo.ExecuteSingle(None))

    Assert.Equal( Some(Some (box 42)), echo.ExecuteSingle 42)

    use echoText = new AdventureWorks.dbo.EchoText()
    Assert.Equal<string[]>([| "<NULL>" |], echoText.Execute(null) |> Seq.toArray)

    let param = "Hello, world!"
    Assert.Equal<string[]>([| param |], echoText.Execute( param) |> Seq.toArray)

 Message: 
    Assert.Equal() Failure
    Expected: Some(Some(Empty))
    Actual:   Some(null)
  Stack Trace: 
    at ProgrammabilityTest.SpWithParamOfRefTypeWithNullDefault() in ProgrammabilityTests.fs line: 162

I believe this goes beyond a "fix calling code by adding None" breaking change, albeit we are dealing with passing sqlvariant here, basically, without further logic in this PR we would make it impossible to pass the default value for such parameter.

I'd like to get feedback, but I believe this could be a deal breaker for users relying on such.

[cmeeren]

Dagnabbit. I have never heart of SQL_VARIANT so I can't help here.

[cmeeren]

I have an idea and would like to move this forward, if possible.

As I understand it, the problem is that when calling an sproc that has a parameter (sql_variant or not) with a default value, then with this PR which requires all parameters, there is no way to pass the default value. This comes from the fact that we are dealing with two different problems: Whether parameters are nullable or not (e.g. @param INT = NULL) and whether parameters have default values (e.g. @param INT = 2). SQL sproc definitions are only concerned with the latter, because all parameters are nullable anyway, whereas in F# and SqlClient we want to control which are nullable and not. Currently (not in this PR) SqlClient decides to make parameters optional if the sproc parameter has a default value, and furthermore, if it is also a value type, it makes the parameter option-wrapped. If the sproc parameter does not have a default value, SqlClient makes the parameter required and not option-wrapped. What this PR originally set out to do was to avoid optional parameters and ensure that all parameters are required, but that nullable parameters are option-wrapped.

Is the above assessment correct?

Now, for the possible solution. As mentioned above, I see that we are conflating nullability with default values (e.g. truly optional parameters). My idea is that we can check whether the default value is NULL or something else. If NULL, this should be a nullable (i.e. required but option-wrapped) parameter. If anything else, e.g. @param INT = 2 or PARAM SQL_VARIANT = "SomeString", then we preserve the current behavior and make the parameter optional. I think that will solve all the problems here.

Does that make sense?

It depends on whether it is possible for the TP to obtain information about whether the default value is NULL or something else. Is that possible? (I don't know how.)

[cmeeren]

Follow-up to my last question above: I see that the Parameter type has a member DefaultValue: obj option. Assuming this is None if the default sproc value is NULL, then this seems really easy:

if p.DefaultValue.IsNone then
  ProvidedParameter(parameterName, parameterType = typedefof<_ option>.MakeGenericType( p.TypeInfo.ClrType))
else
  ProvidedParameter(parameterName, parameterType = typedefof<_ option>.MakeGenericType( p.TypeInfo.ClrType) , optionalValue = null)

I have added a commit to the PR to show what I mean. (AppVeyor build fails for what seems like an unrelated reason, and I am unable to get the build to work locally, so unfortunately I can't test.)

[cmeeren]

The build succeeds and I'm happy with the current state of this PR. Please let me know if there is anything else you want changed.

[cmeeren]

@smoothdeveloper Is this ready to merge, or is there anything you want changed in this PR?

[smoothdeveloper]

@cmeeren, I haven't had a chance to take another look (since clearing the compile error in unit test project).

I see in the original issue that few users seems to be ok with breaking changes (thumbs up on comment about it) so I'll allocate some time to review and merge this PR.

Thanks for contributing it and keeping pushing on it.

[cmeeren]

Thanks a lot! Looking forward to this. Time and again I keep getting bitten by this because after adding new parameters to sprocs, I forget to add new parameters when calling them, causing bad/missing data in DB... My fault of course, but this PR would fix that for good 😁

Don't hesitate to let me know if you need anything more here.

[smoothdeveloper]

Closed by #400