Zaid-Ajaj / DustyTables

Thin F# API for SqlClient for easy data access to ms sql server with functional seasoning on top
MIT License
74 stars 11 forks source link

Sql.executeNonQueryAsync not capturing potential exception #18

Closed AlbertoDePena closed 3 years ago

AlbertoDePena commented 3 years ago

In this code:

/// <summary>Executes the query asynchronously and returns the number of rows affected</summary>    
let executeNonQueryAsync  (props: SqlProps) = task {           
  let! token = Async.CancellationToken            
  use mergedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(token, props.CancellationToken)            
  let mergedToken = mergedTokenSource.Token            
   if props.SqlQuery.IsNone then failwith "No query provided to execute. Please use Sql.query"            
  let connection = getConnection props            
  try                
    if not (connection.State.HasFlag ConnectionState.Open) then 
      do! connection.OpenAsync(mergedToken)                
    use command = new SqlCommand(props.SqlQuery.Value, connection)                
    populateCmd command props                
    if props.NeedPrepare then command.Prepare()                
    let! affectedRows = command.ExecuteNonQueryAsync(mergedToken)                
    return Ok affectedRows            
  finally                
    if props.ExistingConnection.IsNone then connection.Dispose()        
}

Why wrap affectedRows in Result.Ok if potential exception is not being captured?

It should probably not wrap it at all and let the consumer handle it... no?

Zaid-Ajaj commented 3 years ago

You are absolutely right! It shouldn't return Result at all 🤦 🤦 🤦 I will fix it in the next release

AlbertoDePena commented 3 years ago

There are a couple of places where Result.Ok and Result.Error are being used... Fyi :) I am amazed at how you can make FSharp libs so simple yet so powerful! Keep up the good work 😃

Zaid-Ajaj commented 3 years ago

@AlbertoDePena

There are a couple of places where Result.Ok and Result.Error are being used... Fyi :)

Fixed those, now we are not returning Result anymore. Published as of v4.0 (bumped major version because of breaking change) can you give it a try?

Keep up the good work 😃

Thanks 🙏 ❤️ will sure do