Add support for ragged parameter matching in auto-implemented DALs

roryprimrose commented 10 years ago

There have been a number of occasions where I have a common entity that I want to throw at a procedure but also want to add an additional parameter to the procedure. Ideally Insight.Database would work out the parameter matching from combinations of reference type and value type parameters. Currently it only supports one or the other.

For example, the procedure might have the following parameters:

@AccountId UNIQUEIDENTIFIER,
@ContainerId UNIQUEIDENTIFIER,
@EntityId UNIQUEIDENTIFIER,
@Priority INT

I have a common reference type that I use to identity the entity:

public class EntityReference
{
    public Guid AccountId { get; set ; }
    public Guid ContainerId { get; set ; }
    public Guid EntityId { get; set ; }
}

Currently my interface needs to be exploded out to the following signature to avoid having to create another class for just this purpose.

Task<IEnumerable<Entity>> SetPriorityAsync(Guid accountId, Guid containerId, Guid entityId, int priority)

What I would really like to do to keep the code base as clean as possible is:

Task<IEnumerable<Entity>> SetPriorityAsync(EntityReference reference, int priority)

jonwagner commented 10 years ago

hm...

jonwagner commented 10 years ago

When I first implemented parameter binding, I thought multiple levels would get too complicated, but with the current codebase, I don't think it will be that bad.

So:

Task<IEnumerable<Entity>> SetPriorityAsync(EntityReference reference, int priority)

gets translated into:

connection.Query(new { reference = param1, priority = param2 });

I think that the best way to implement this would be to bind parameters breadth-first. i.e. when looking for @ContainerID, look at reference (not it), then priority (not it), then go back to parameter0, and look at its members.

It shouldn't be too hard to implement, but it leaves some design questions:

How far down an object hierarchy should we look? One level? All levels? Configurable?
Should this feature be on all the time? Configurable?
If we turn it on by default, there may be some currently unbound parameters that would get bound. Is this a bad change? How do we effectively let people know that this is a change? (I could bump the major version number, but then I'd like to combine it with other big features.

jonwagner commented 10 years ago

I have it working with input, output, and table-valued parameters. It's nice, because now your object graph is automatically flattened when you send objects to the database.

Another edge case to consider is null valued children.

e.g.

Parent.Child.Value (maps to) @value

What happens if Child is null? Right now, I have the parameter set to DbNull for inputs, and for output parameters, the field is ignored (i.e. we won't create the Child object for output parameters. If you want child objects created, you should return it as a recordset).

roryprimrose commented 10 years ago

Regarding your earlier point, I don't think there could be unbound parameters because how did their procedure work previously if the parameters couldn't be bound?

It's so awesome that you have made this work already. Regarding Parent.Child.Value, I would think the value would be unbound in this case because a reference to .Value would not be found because it doesn't exist when you walk the object. To pass DbNull in this scenario would be an assumption that could hide a lot of bugs in the user code.

jonwagner commented 10 years ago

I forgot that null=unbound, which is different from dbnull. You're right that working existing code won't have any unbound parameters unless there are defaults defined.

It might be better to leave interior null references unbound. i.e.

Parent.Child.Value

if parent is null, then the framework naturally throws a nullref exception (current behavior)
If child is null, then the parameter is left unbound (but could be filled in by a default)
If value is null, then the parameter is set to DbNull (current behavior)

I think this has the least chance of causing errors, but may still be confusing.

I wonder if those rules work for TVPs as well. i.e. if I leave a column as null (not DbNull), will it attempt to use a default and then fail if it's left unbound?

roryprimrose commented 10 years ago

I'm a little confused about how TVP's would come into play here. The IEnumerable that represents the TVP stored proc parameter would either be provided or not. This one is baking my noodle a little.

jonwagner commented 10 years ago

Here I'm talking about binding IN the TVPs. There's the same process for binding an object (tree) to the table representation.

On Jul 24, 2014, at 8:05 AM, "roryprimrose" notifications@github.com wrote:

I'm a little confused about how TVP's would come into play here. The IEnumerable that represents the TVP stored proc parameter would either be provided or not. This one is baking my noodle a little.

— Reply to this email directly or view it on GitHub.

jonwagner commented 10 years ago

I pushed the code for this to the 'deepparams' branch. Give it a try. LMK if you need me to do a build.

roryprimrose commented 10 years ago

Yeah a build would be good. I’ve downloaded the branch, installed psake but it’s not working for me.

Error: 13/08/2014 1:01:45 PM:

At C:\Users\Me\Downloads\Insight.Database-deepparams\Build.psake.ps1:12 char:

16 + $version = git describe --abbrev=0 --tags + ~~~ [<<==>>

] Exception: The term 'git' is not recognized as the name of a cmdlet, function

, script file, or operable program. Check the spelling of the name, or if a pat

h was included, verify that the path is correct and try again.

From: Jon Wagner [mailto:notifications@github.com] Sent: Saturday, 9 August 2014 4:57 AM To: jonwagner/Insight.Database Cc: roryprimrose Subject: Re: [Insight.Database] Add support for ragged parameter matching in auto-implemented DALs (#125)

I pushed the code for this to the 'deepparams' branch. Give it a try. LMK if you need me to do a build.

— Reply to this email directly or view it on GitHub https://github.com/jonwagner/Insight.Database/issues/125#issuecomment-51643590 . https://github.com/notifications/beacon/2044291__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcyMzE0MzQyMiwiZGF0YSI6eyJpZCI6MzcwMzc0MzN9fQ==--d2344bbec9ec24b56a1b5c45bec361cc04ac462a.gif

jonwagner commented 10 years ago

The build failed because you don't have git on your path.

I put a build up in nuget as 4.2.6-alpha1. You can get it by enabling pre-release builds.

roryprimrose commented 10 years ago

I've finally had a chance to pull this in and see what happens. Existing functionality now fails with the following.

System.AggregateException: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> System.InvalidProgramException: Common Language Runtime detected an invalid program.
    at CreateInputParameters-8d97a019-8c1b-46ea-9ec7-dd76c7d28599(IDbCommand, Object)
   at Insight.Database.DBCommandExtensions.AddParameters(IDbCommand cmd, Object parameters) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\DBCommandExtensions.cs: line 30
   at Insight.Database.DBConnectionExtensions.CreateCommand(IDbConnection connection, String sql, Object parameters, CommandType commandType, Nullable`1 commandTimeout, IDbTransaction transaction) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\DBConnectionExtensions.cs: line 276
   at Insight.Database.AsyncExtensions.<>c__DisplayClass12`1.<QueryAsync>b__10(IDbConnection c) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\AsyncExtensions.cs: line 212
   at Insight.Database.AsyncExtensions.<>c__DisplayClass47`1.<ExecuteAsyncAndAutoClose>b__46(Task`1 t) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\AsyncExtensions.cs: line 1206
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
 --- End of inner exception stack trace ---
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at Insight.Database.AsyncExtensions.<>c__DisplayClass47`1.<ExecuteAsyncAndAutoClose>b__45(Task`1 t) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\AsyncExtensions.cs: line 1219
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
 --- End of inner exception stack trace ---
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at Insight.Database.AsyncExtensions.<>c__DisplayClass47`1.<ExecuteAsyncAndAutoClose>b__44(Task`1 t) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\AsyncExtensions.cs: line 1236
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
 --- End of inner exception stack trace ---
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at Insight.Database.AsyncExtensions.<>c__DisplayClass47`1.<ExecuteAsyncAndAutoClose>b__43(Task`1 t) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\AsyncExtensions.cs: line 1249
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
 --- End of inner exception stack trace ---
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()

I've tried putting in binding redirects as well but no dice.

This call failed before it had a chance to use the new functionality. FYI, my first test is slightly different to what I posted above. In my first test, I was passing two models rather than a model and then primitives. Is this expected to work?

jonwagner commented 10 years ago

If you're getting an invalid program exception, it means that Insight is generating bad mapping code for the parameters. There were some small changes to that code, but my test cases pass. I would need to know the parameter list/structure and the SQL signature in order to debug it.

You should be able to add any number of classes or primitives and it will try to map them left-to-right, then top-down. I added a few test cases for that to make sure it works.

dschinde commented 10 years ago

The change seems to work for the most part, but I've found what seems to be a regression with number sizes in Oracle.

Oracle stored procedure:

PROCEDURE TESTPROC(id IN NUMBER, statusCode IN VARCHAR2) IS
BEGIN
    NULL;
END;

C# classes:

public interface IRepository
{
    void TestProc(long id, string statusCode);
    void TestProc(Class1 param1, string statusCode);
    void TestProc(Class2 param1, string statusCode);
    void TestProc(Class3 param);
}

public class Class1
{
    public long Id { get; set; }
}

public class Class2
{
    public int Id { get; set; }
}

public class Class3
{
    public long Id { get; set; }
    public string StatusCode { get; set; }
}

Main:

var repo = new OracleConnection(ConnectionString).As<IRepository>();
repo.TestProc(0, "status"); // This works
repo.TestProc(new Class2(), "status"); // This works
repo.TestProc(new Class3()); // This works
repo.TestProc(new Class1(), "status"); // Crashes here

Exception:

System.AccessViolationException
at CreateInputParameters-8420c0b2-2371-4efd-b6c6-da34f5e0803d(IDbCommand , Object )
at Insight.Database.DBCommandExtensions.AddParameters(IDbCommand cmd, Object parameters) in c:\projects.net\Insight\Insight.Database\Insight.Database\Extensions\DBCommandExtensions.cs:line 32

Insight.Database.dll!Insight.Database.DBCommandExtensions.AddParameters(System.Data.IDbCommand cmd, object parameters) Line 33  C#
Insight.Database.dll!Insight.Database.DBConnectionExtensions.CreateCommand(System.Data.IDbConnection connection, string sql, object parameters, System.Data.CommandType commandType, int? commandTimeout, System.Data.IDbTransaction transaction) Line 278  C#
Insight.Database.dll!Insight.Database.DBConnectionExtensions.Execute.AnonymousMethod__a(System.Data.IDbCommand _, System.Data.IDataReader __) Line 328  C#
Insight.Database.dll!Insight.Database.DBConnectionExtensions.ExecuteAndAutoClose<int>(System.Data.IDbConnection connection, System.Func<System.Data.IDbConnection,System.Data.IDbCommand> getCommand, System.Func<System.Data.IDbCommand,System.Data.IDataReader,int> translate, System.Data.CommandBehavior commandBehavior) Line 1491 C#
Insight.Database.dll!Insight.Database.DBConnectionExtensions.ExecuteAndAutoClose<int>(System.Data.IDbConnection connection, System.Func<System.Data.IDbConnection,System.Data.IDbCommand> getCommand, System.Func<System.Data.IDbCommand,System.Data.IDataReader,int> translate, bool closeConnection) Line 1459    C#
Insight.Database.dll!Insight.Database.DBConnectionExtensions.Execute(System.Data.IDbConnection connection, string sql, object parameters, System.Data.CommandType commandType, bool closeConnection, int? commandTimeout, System.Data.IDbTransaction transaction, object outputParameters) Line 324 C#

jonwagner commented 10 years ago

I had a few issues with value types in certain code paths. I put a new build up as 4.3.0-alpha1 (just for SQL and Oracle/Managed), with the latest fixes.

I'm still thinking about the best way to configure this option, but for now, I just want to make sure that it works.

dschinde commented 10 years ago

The 4.3.0-alpha1 version fixed the problem. Thanks!

roryprimrose commented 10 years ago

GOLD! I just used this with a signature of (Guid, Model) and it worked like a dream. All my other 532 SQL Server integration tests also pass as per previous functionality.

jonwagner commented 10 years ago

Yay! Now I just have to figure out the configuration story for this.

jonwagner commented 10 years ago

The good news is that v5.0 is coming along nicely, mostly centered around this feature and configuration updates.

roryprimrose commented 10 years ago

Happy dance!

jonwagner commented 10 years ago

Current thoughts on this (now called "Child Binding"), where a parameter/column can bind to a child object if it's not found on the parent object.

Default for child binding is OFF, except for interface parameters, which is ON. I think this will be the most common use case.
For each class, you can configure ChildBinding with the BindChildrenAttribute, and select whether you want to bind for Params (in/out params), tables(TVPs & merge outputs), or both. ChildBinding is applied one class-level at a time.
You can disable binding interface parameters with [BindChildren(Off)] on the method. Shouldn't be needed though.
Programmatic configuration via ColumnMapping.All.EnableChildBinding<T> and the like (but attributes supercede this).
ChildBinding for result sets is always OFF, since there is already a mechanism to describe result sets.

I didn't add binding on a per-proc or per-TVP basis, because I thought it would add a lot of complexity for little use, and would likely let devs fall into the pit of ickiness (i.e. questions on how to hack the library rather than finding a simper solution.)

(Also, new hooks for fully custom parameter/column binding, where you can return the string Parent.Child.Child.Field, and Insight will do the rest).

Thoughts?

jonwagner commented 10 years ago

This is all now in 5.0.0-alpha1 in nuget.

Unless you did some wacky configuration, this should just be a drop in.

I'm going to bundle this feature plus some more work around parent/child relationships (see #145 & #147), into v5.0

Documentation to follow.

roryprimrose commented 10 years ago

I haven't changed my existing code to leverage this although I do have a few ragged calls. The latest beta passes all 576 of my integration tests :)

roryprimrose commented 10 years ago

So presumably child binding is an extension to ragged parameters, so I can still use combination of value types and reference types.

jonwagner commented 10 years ago

Yes.

The interface implementer just packages the parameters into an anonymous class, then calls query or other extension method.

Child binding just lets that hidden anonymous class get exploded apart (one level).

If you want to bind deeper you would have to tag/configure the subclasses.

On Sep 27, 2014, at 12:55 AM, "roryprimrose" notifications@github.com wrote:

So presumably child binding is an extension to ragged parameters, so I can still use combination of value types and reference types.

— Reply to this email directly or view it on GitHub.

jonwagner commented 10 years ago

Cool. I'll close this out for now.

jonwagner / Insight.Database

Add support for ragged parameter matching in auto-implemented DALs #125