MightyOrm / Mighty

A new, small, dynamic micro-ORM. Highly compatible with Massive, but with many essential new features.
BSD 3-Clause "New" or "Revised" License
101 stars 20 forks source link

Outparams are not utilized for all public methods that take them in as parameters? #18

Closed Kritner closed 4 years ago

Kritner commented 4 years ago

Hopefully I'm misunderstanding, but I can't seem to get updates to my outParams to work when utilizing either the strong typed or dynamic QueryFromProcedure.

This is how I'm performing my call:

var db = new MightyOrm<MyType>(_connectionString);
var outParam = new
{
    TotalRecords = (long)0
};

var data = db.QueryFromProcedure("myProc",
    new
    {
        param.PageSize, 
        param.Page,
    }, outParam);

Console.WriteLine(outParam.TotalRecords);

I would have expected the above outParam to be updated as a result of the execution of the stored procedure, but I'm not sure if that's actually happening.

I did see that QueryFromProcedure eventually gets to a method called CreateCommandWithParams:

public override DbCommand CreateCommandWithParams(
      string sql,
      object inParams = null,
      object outParams = null,
      object ioParams = null,
      object returnParams = null,
      bool isProcedure = false,
      DbConnection connection = null,
      params object[] args)
    {
      return this.CreateCommandWithParamsAndRowCountCheck(sql, inParams, outParams, ioParams, returnParams, isProcedure, connection, args).Item1;
    }

Where the return from CreateCommandWithParamsAndRowCountCheck is a value tuple, where item 2 is a boolean that indicates if out params are used. In this particular method (CreateCommandWithParams) that flag is not used.

Drilling further into the code under a separate public method such as ExecuteProcedure, you eventually get to:

Tuple<DbCommand, bool> andRowCountCheck = this.CreateCommandWithParamsAndRowCountCheck(spName, inParams, outParams, ioParams, returnParams, true, (DbConnection) null, args);

Which actually makes use of the flag returned from the Tuple<DbCommand, bool> in:

if (andRowCountCheck.Item2)
        {
          // ISSUE: reference to a compiler-generated field
          if (MightyOrm<T>.\u003C\u003Eo__296.\u003C\u003Ep__0 == null)
          {
            // ISSUE: reference to a compiler-generated field
            MightyOrm<T>.\u003C\u003Eo__296.\u003C\u003Ep__0 = CallSite<Action<CallSite, MightyOrm<T>, int, object, object>>.Create(Microsoft.CSharp.RuntimeBinder.Binder.InvokeMember(CSharpBinderFlags.InvokeSimpleName | CSharpBinderFlags.ResultDiscarded, "AppendRowCountResults", (IEnumerable<Type>) null, typeof (MightyOrm<T>), (IEnumerable<CSharpArgumentInfo>) new CSharpArgumentInfo[4]
            {
              CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.UseCompileTimeType, (string) null),
              CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.UseCompileTimeType, (string) null),
              CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.UseCompileTimeType, (string) null),
              CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, (string) null)
            }));
          }
          // ISSUE: reference to a compiler-generated field
          // ISSUE: reference to a compiler-generated field
          MightyOrm<T>.\u003C\u003Eo__296.\u003C\u003Ep__0.Target((CallSite) MightyOrm<T>.\u003C\u003Eo__296.\u003C\u003Ep__0, this, num, outParams, obj);
        }

I'm having trouble understanding what the above call actually does, but I'm guessing it's involved with the actual population of the outParams object from OUTPUT parameters from the stored procedure?

The originally mentioned QueryFromProcedure does not seem to share this similar block of code, I'm only assuming that is why I'm not getting my outParams object populated.

Can you confirm this behavior? and is there a way to get around it? In the end, I was trying to return a "total" row count via an output param declared @TotalRows BIGINT OUTPUT from my sp, using it in parallel with the strong typed QueryFromProcedure.

I have confirmed that:

declare @TotalRecords bigint
exec myProc 
20, -- page size
1, -- page number
@TotalRecords OUTPUT
SELECT @TotalRecords

does in fact return me my value, but I have not had luck doing the same with QueryFromProcedure and using outParams.

mikebeaton commented 4 years ago

Background: outParams can't be updated, as it's not officially supported to update members of an anonymous object (in C#... it IS in VB): https://stackoverflow.com/questions/17441420/how-to-set-value-for-property-of-an-anonymous-object .

So, for the purposes of a library which hopefully might get used other than by me... I decided not to directly modify outParams (as I did in my original SP params code which I wrote before ever encountering Massive).

So, if you Query, you get the query results but not the param results. If you Execute you get the param results, but not the query results.

It's a bit fiddly, but you can also get both. I need to document this in the documentation.

List<ImportantItem> GetItemsBelongingToParent(int parentId)
{
    List<ImportantItem> items;
    dynamic results;

    // this is the way to get query results AND output param results in Mighty
    using (var cmd = db.CreateCommandWithParams(
        "GetImportantItems",
        inParams: new { ParentID = parentId },
        returnParams: new { RETVAL = (int?)null },
        isProcedure: true
    ))
    {
        // because Mighty/Massive always uses delayed execution (query never executed until you
        // enumerate the enumerable), we need to use .ToList() to force execution...
        items = db.Query(cmd).ToList();

        // ... _before_ reading back the param results
        // (this does not re-execute the command, it just gathers up the results)
        results = db.ResultsAsExpando(cmd);
    }

    if (results.RETVAL != 0)
    {
        throw new ApiNotFoundException($"ParentID={parentID} not found");
    }

    return items;
}
mikebeaton commented 4 years ago

PS I'm looking into providing a .snupkg so that you can debug into Mighty.

PPS I may be trying to 'teach my Grandmother to suck eggs' here, in which case sorry! But, until I produce a .snupkg (or even when/if I have, if you'd prefer to debug into the debug-compiled version), it's up to you, but my favourite way to debug into OS projects is to clone a local copy and temporarily make my project which needs debugging depend directly on the local copy (checked out at the tag corresponding to the current release) instead of on the NuGet package - then you can step into the original code instead of release-version compiler-generated code.

Kritner commented 4 years ago

Yeah I was planning on poking around with using a project reference of a clone rather than the nuget package for a bit, after having digested your response ;)

what is the difference between output params and return params? I don't know if I saw them defined. return params isn't just "the result sets" is it?

Background: outParams can't be updated, as it's not officially supported to update members of an anonymous object

But aren't you effectively updating them by inlining the creation of the anonymous object in your example? (though from the returnParams perspective rather than outParams?)

What about some sort of Wrapper<T> object that would allow for the additional return values to be just pushed onto the Wrapper as members, while keeping the T either the IEnumerable<T>, dynamic, or IEnumerable<dynamic> ? I guess I'm just hoping for an API and return that would allow me easy access to my SPs OUTPUT parameters, while still sticking with the strong typing of MyObjectType.

Also thanks for the update from #16, I was able to confirm that as working this morning (prior to realizing that my outParam was not getting its value updated :D)

mikebeaton commented 4 years ago

I have now added the above instructions and example to the documentation 430ebd9a03bac09ce37a506c5fee37bb31e3d1f0

Kritner commented 4 years ago

Oh gotcha, did not realize there was such a difference between:

returnParams: new { RETVAL = (int?)null }

and

var returnParam = new
{
   RETVAL = (int?)null
}
// ...
returnParams: returnParam

I had tried the former as well, but I guess this goes back to your comment from https://github.com/MightyOrm/Mighty/issues/18#issuecomment-590409939:

So, if you Query, you get the query results but not the param results. If you Execute you get the param results, but not the query results.

mikebeaton commented 4 years ago

input, output, input-output and return parameter directions are defined by System.Data.Common, not by me - I just wrap 'em!

The return param gives a name to the value from RETURN [n] in SQL Server. The distinction as I recall makes less sense on some other DB platforms - but in any event these will do whatever they would do if you had manually created a DbParameter with the equivalent ParamaterDirection against the command, on the underlying driver.

mikebeaton commented 4 years ago

Just from looking quickly, I don't think there is any important distinction between your two examples just above. What were you thinking is the difference?

Kritner commented 4 years ago

Just from looking quickly, I don't think there is any important distinction between your two examples just above. What were you thinking is the difference?

This was in reference to anonymous objects not being able to be updated from your comment:

Background: outParams can't be updated, as it's not officially supported to update members of an anonymous object

is that (the updating of an anon object) not what is occurring in your sample code in the docs:

var db = new MightyOrm(connectionString);
dynamic result = db.ExecuteWithParams(
    "set @a = @b + 1",
    inParams: new { b = 1233 },
    outParams: new { a = 0 });
Assert.AreEqual(1234, result.a);

... nevermind, writing it out I now see the property a from the anon object has been projected onto the dynamic result, rather than from the anon object, in your Assert.

mikebeaton commented 4 years ago

Yes, exactly!

In many ways I think this:

var db = new MightyOrm(connectionString);
var outObj = new new { a = 0 };
db.ExecuteWithParams(
    "set @a = @b + 1",
    inParams: new { b = 1233 },
    outParams: outObj);
Assert.AreEqual(1234, outObj.a);

would be a nicer UI - but even though it is possible to do this (I've done it, it works!) it's not officially supported, as it involves changing the value of what is (ostensibly) a read-only property on outObj.

mikebeaton commented 4 years ago

Quick test to the reader (by which I mean, I made a mistake...! ;) ): What is the problem with the above nicer UI? Answer: It wouldn't be possible with Mighty/Massive queries anyway - since Massive/Mighty queries are always 'contracts' to do the underlying work, which won't actually access the db until you start to consume results.

So there's no way that the output, io and return params could be filled out immediately on return from a Query (even though they could from Execute) - at least, not without changing that pretty fundamental decision. So that nicer UI, while it is possible in the example I showed just above, couldn't actually be used where it matters (combining Mighty queries with params) anyway.

Equally, it wouldn't be possible to use C# 7 multiple return values, or a manual wrapper type, to combine out parameters into the return value from queries, for just the same reason (the query hasn't happened by the time we return, and won't happen at all until the IEnumerable is enumerated).

So - just to document it here! - that's another reason (along with the non-writeability of anonymous objects) why the approach now written up in the docs is pretty much required, I think.

Kritner commented 4 years ago

Interesting... so I guess this brings me to a few questions:

public class QueryWithOutputWrapper<T>
{
   public T data { get; set; }
   public dynamic OutParam { get; set; }
   public dynamic ReturnParam { get; set; }
}

I saw your response come in while I was writing this, so perhaps my response is moot ;)

Equally, it wouldn't be possible to use C# 7 multiple return values, or a manual wrapper type, to combine out parameters into the return value from queries, for just the same reason (the query hasn't happened by the time we return, and won't happen at all until the IEnumerable is enumerated).

Ah I guess the nature of writing a wrapper would require no longer having a deferred execution, which seems to go against the library in general.

Would you be opposed to introducing perhaps an extension or static method, something that made it clear you are opting out of deferred execution for something like this? If not, I can just write something for my own code.

mikebeaton commented 4 years ago

For Query types, is there a reason for the method to include the outParams and returnParams at all? since they can be passed in, but not accessed?

Please, look carefully at https://github.com/MightyOrm/Mighty/issues/18#issuecomment-590409939 - they can be accessed!

Kritner commented 4 years ago

For Query types, is there a reason for the method to include the outParams and returnParams at all? since they can be passed in, but not accessed?

Please, look carefully at #18 (comment) - they can be accessed!

Sorry sorry, I meant mutated, from the response:

So, if you Query, you get the query results but not the param results. If you Execute you get the param results, but not the query results.

Of course the updates you did to the documentation and https://github.com/MightyOrm/Mighty/issues/18#issuecomment-590409939 allow me to get the information i need.

thanks!

mikebeaton commented 4 years ago

Actually - catching up with what you meant! - for the variant of Query that doesn't take a DbCommand, it is true that output params can be passed in but can't be accessed, you're right. I tend to think of all the versions of Query as different versions of the 'the same thing', but yes you're right, there are specific variants that take these output params and yet, because you don't have a link to the underlying command afterwards, you can never read back the values.

In fact, it did occur to me whether or not I needed to include these in the API, but I thought that it was better to have a consistent API for everything which takes params, which always does include all the available types. The fact that they are present might - for instance - be required by what is being called; even when the user knows that they don't need the values. And if they DO need the values, they can use the more complex version which I've now documented.

Kritner commented 4 years ago

for your and anyone else's info, this is what I ended up going with (heavily based on your https://github.com/MightyOrm/Mighty/issues/18#issuecomment-590409939, just an extension method).

public class MightyResultsWithExpando<T>
    {
        public List<T> Data { get; }
        public dynamic ResultsExpando { get; }

        public MightyResultsWithExpando(List<T> data, dynamic resultsExpando)
        {
            Data = data;
            ResultsExpando = resultsExpando;
        }
    }

public static class MightyOrmExtensionMethods
    {
        public static MightyResultsWithExpando<T> QueryWithExpando<T>(this MightyOrm<T> db,
            string sql, object inParams = null, object outParams = null, bool isProcedure = true)
            where T : class, new()
        {
            var items = new List<T>();
            dynamic resultsExpando;

            using (var cmd = db.CreateCommandWithParams(sql, inParams: inParams, outParams: outParams, isProcedure: isProcedure))
            {
                items = db.Query(cmd).ToList();
                resultsExpando = db.ResultsAsExpando(cmd);
            }

            return new MightyResultsWithExpando<T>(items, resultsExpando);
        }
    }

Obviously the above is specifically for using the strongly typed query from proc, expecting multiple results, and placing the dynamic into a property that can be accessed via the caller.

To bring it all home from the opening of the issue, the calling code now looks like:

var db = new MightyOrm<MyType>(_connectionString);

var result = db.QueryWithExpando("myProc",
    new
    {
        param.PageSize, 
        param.Page,
    }, 
    new
    {
        totalRecords = (long)0
     });

var data = result.Data; // strongly typed List<MyType>

Console.WriteLine((long)result.ResultsExpando.totalRecords);

Thanks again for the help!

mikebeaton commented 4 years ago

That does look neat. Nice job. Hmmm.... processing.

It is certainly more convenient - for when you do need this. I guess I thought that that wouldn't be very often for most people... but I admit that I do use the long-hand solution in my current work project (the 'dogfooding' project) from time to time. If guess anyone who uses stored procedures a lot (I think not many people do, even if they 'should' 😉 !) is likely to need this a non-zero amount.