Added support for array parameters

[colin-young]

#10

[maxtoroq]

Your fix breaks the default behavior. The test you wrote is incorrect:

var query = SQL
   .SELECT("*")
   .WHERE("VALUE IN ({0})", new object[] { "a", "b", "c" });

The above code should NOT transform {0} to {0}, {1}, {2}. First, the fact that you are using new object[] makes no difference on this case because you get that for free in C#. The following code is equivalent:

var query = SQL
   .SELECT("*")
   .WHERE("VALUE IN ({0})", "a", "b", "c");

So, you are really not using an array parameter here. You'd have to do this:

var query = SQL
   .SELECT("*")
   .WHERE("VALUE IN ({0})", new object[] { new object[] { "a", "b", "c" } });

The wiki article can be a bit confusing because it uses an array of a value type:

int[] ids = { 1, 2, 3 };

var query = SQL
   .SELECT("*")
   .FROM("Products")
   .WHERE("CategoryID IN ({0})", ids);

On this case, the C# compiler does not cast int[] to object[], because int is a value type. So, the above code is equivalent to:

int[] ids = { 1, 2, 3 };

var query = SQL
   .SELECT("*")
   .FROM("Products")
   .WHERE("CategoryID IN ({0})", new object[] { ids });

Hope all of this makes sense. I haven't worked on this issue because I don't think it can be easily fixed in a backwards-compatible way. I've thought about something like SQL.ArrayParam(Array) that wraps the array so it's not expanded and then un-wraps it to add it to the ParameterValues.

[colin-young]

Do you have a test for the default behavior? I'm afraid I don't understand what it is meant to be based on your comments. I've updated my tests to cover the single value case, and using arrays, both int as in the wiki example and strings (2d28107).
When using the original code in SqlBuilder.cs, 2 of my 4 tests fail:

      public void VerifyArrayToList2() {
         var query = SQL
             .SELECT("*")
             .WHERE("VALUE IN ({0})", 1, 2, 3);

         Assert.AreEqual(3, query.ParameterValues.Count);

         var cmd = query.ToCommand(conn);
         Assert.IsTrue(cmd.CommandText.Contains("@p2"));
      }

      [TestMethod]
      public void VerifyStringArrayToList() {
         string[] ids = { "a", "b", "c" };

         var query = SQL
             .SELECT("*")
             .WHERE("VALUE IN ({0})", ids);

         Assert.AreEqual(3, query.ParameterValues.Count);

         var cmd = query.ToCommand(conn);
         Assert.IsTrue(cmd.CommandText.Contains("@p2"));
      }

[maxtoroq]

Those 2 tests should fail. You either don't understand how params work in C#, or how SqlBuilder handles array parameters.

[colin-young]

Clearly we do not understand what your original intent for this feature is. Can you provide a test or sample usage that our SqlBuilder changes break? We have written the tests to reflect what we expect to be normal usage.

int id0 = 1;
string[] id1 = { "a", "b", "c" };
int[] id2 = { 1, 2, 3 };
var q3 = new List<string> {"a", "b", "c"};

var q0 = SQL.SELECT("*").WHERE("VALUE = {0}", id);
var q0_alt = SQL.SELECT("*").WHERE("VALUE = {0}", 1);
var q1 = SQL.SELECT("*").WHERE("VALUE = {0}", id1);
var q1_alt = SQL.SELECT("*").WHERE("VALUE = {0}", "a", "b", "c");
var q2 = SQL.SELECT("*").WHERE("VALUE = {0}", id2);
var q2_alt = SQL.SELECT("*").WHERE("VALUE = {0}", 1, 2, 3);
// next line won't work because we'd need to know what T to convert IEnumerable<T> to in Append method
//var q2 = SQL.SELECT("*").WHERE("VALUE = {0}", id2);
var q2 = SQL.SELECT("*").WHERE("VALUE = {0}", id2.ToArray());

In all working cases presented above, the SQL that is generated is exactly what we expect to see.

[maxtoroq]

I've added a couple of tests to the master branch. This one fails with your changes:

[TestMethod]
public void Multiple_Parameters() {

   var query = SQL
      .SELECT("{0}, {1}", 1, 2);

   Assert.AreEqual("SELECT {0}, {1}", query.ToString());
   Assert.AreEqual(2, query.ParameterValues.Count);
}

[colin-young]

That makes more sense now and seems like another obvious use pattern that we hadn't considered (yet). Let me see what we can do with that.

[colin-young]

The last check in gets your test working again, but adds a new test to verify that another scenario is (still) broken. IMHO, it's better than it was since it now handles most of the usual cases.

I started to explore how various combinations show up in the method to see if I could see any solutions.

  [TestMethod]
  public void TestParamsMethod() {
     var vals = new int[] { 1, 2 };
     var strings = new string[] { "a", "b" };

     Assert.AreEqual(3, TestParams(1, vals, "A").Count());
     Assert.AreEqual(3, TestParams(1, strings, "A").Count());
     Assert.AreEqual(2, TestParams(strings, "A").Count());
     Assert.AreEqual(2, TestParams("A", strings).Count());
     Assert.AreEqual(2, TestParams(strings).Count());
  }

  private object[] TestParams(params object[] vals) {
     return vals;
  }

I'm thinking right now that it might be able to be solved something like:

currentReplace = 0
lastFormatPlaceholder = 0
foreach parameter p
    if p is not array
        format.Replace(lastFormatPlaceholder, currentReplace)
        lastFormatPlaceholder++, currentReplace++
    else
        format.Replace(lastFormatPlaceholder, list [currentReplace..currentReplace + p.Count]
        lastFormatPlaceholder++, currentReplace += p.Count
    end if
end for

I'm sure there are a whole bunch of things I'm not considering there. When I get time I'll see how far I can get with it (since the original fix solves our problem for the way we are using the library I really need to get back to my day job).

[colin-young]

Another scenario is still broken also:

int[] ids = { 1, 2, 3 };
var query = SQL.SELECT("*").WHERE("VALUE IN ({0}, {1}, {2})", ids);

Personally I don't think that's a very good coding pattern to follow, but it would be better if it worked.

This is just all around a bad idea.

[maxtoroq]

That last case should not work, because the number of placeholders is greater than the number of parameters. As I already mentioned, you cannot fix this without breaking array-to-list placeholder substitution.

Perhaps if you explain your particular use case I can give better advice.

[colin-young]

Our use cases are simply 1 placeholder and one array or value, or maybe n placeholders and n values. On further reflection, #12 (comment) is not valid (it isn't supported for string.Format, so I don't think SqlBuilder should either). For #12 (comment) however, I can see that being desirable:

     var ids = new List<string> { "a", "b", "c" };
     var x = string.Format("{0}, {1}, {2}", 1, ids, 2);

Where we'd like the 'ids' to expand the {1} automatically and adjust {2} appropriately (to in this case {1}, {2}, {3} and {4} respectively). I'd call that a "nice to have" since it can be worked around by chaining calls. The current fix, supporting either explicitly listed values (WHERE("X={0} OR Y={1}", x, y)) or arrays (WHERE("X IN({0})", ids)) is sufficient for our current needs.

[maxtoroq]

That case is already supported, I wasn't sure so I added a test:

[TestMethod]
public void Adjust_Other_Placeholders_When_Using_Array_Parameter() {

   var query = SQL
      .SELECT("*")
      .WHERE("c IN ({0}) AND c <> {1}", new object[] { new int[] { 1, 2, 3 }, 4 });

   Assert.IsTrue(query.ToString().Contains("{3}"));
   Assert.AreEqual(4, query.ParameterValues.Count);
}

[colin-young]

I see that test passes with both the original code and the update here. To summarize, this update additionally supports:

• .WHERE("VALUE IN ({0})", 1, 2, 3) and .WHERE("VALUE IN ({0})", "a", "b", "c")
  • I'll allow that this could be considered a little odd in that it deviates from standard String.Format patterns. It feels natural here in the unit test, but it seems as though it will have limited applicability in practice.
• .WHERE("VALUE IN ({0})", ids) where id is string[]
  • This is the original issue we were trying to fix.

My opinion is that the changes now add an additional desirable feature and do not break any of the existing behavior.

[maxtoroq]

The first case is, as you say, a deviation from String.Format, and since array-to-list is already supported it adds more confusion than value.

The second case is exactly the same as the first case, since string[] is casted to object[] by the C# compiler, so there's no difference between .WHERE("VALUE IN ({0})", "a", "b", "c") and .WHERE("VALUE IN ({0})", new string[] { "a", "b", "c" }).

[maxtoroq]

You should write it like this: .WHERE("VALUE IN ({0})", new object[] { new string[] { "a", "b", "c" } })

[colin-young]

I think for now we are going to use the code as-is in my forked version here. It adds the desired behavior we need, doesn't break anything and just adds some weird capabilities that you shouldn't use (and hopefully if anyone tries testing will pick up on it).