Reusing appended Sql instances sometimes causes StackOverflowException

[ialexj]

Hi, this is my first time contributing an issue on github, so apologies if I'm doing anything wrong.

I'm using PetaPoco's Sql class quite extensively on a project, and I've come across an issue that occurs when I reuse instances. When a Sql builder is built, it affects the state of the builders that have been appended to it. If for some reason the builder is reused, the final SQL is corrupted (at best), or there's a StackOverflowException.

This shows up a lot when debugging, because inspecting an instance causes it to build, which will often result in a crash.

It's a bit hard to visualize, so consider the following tests:

[Fact]
public void Sql_AppendShouldNotChangeAppended_ShouldBeValid()
{
    var sql1 = new Sql("TEST 1");
    var sql2 = new Sql("TEST 2");
    var sql3 = new Sql("TEST 3");

    var final1 = new Sql().Append(sql1).Append(sql2).Append(sql3);
    final1.SQL.ShouldBe("TEST 1\nTEST 2\nTEST 3");
    sql2.SQL.ShouldBe("TEST 2"); // This fails, actual value: "TEST2\nTEST3"
}

[Fact]
public void Sql_AppendTwiceShouldNotStackOverflow_ShouldBeValid()
{
    var sql1 = new Sql("TEST");
    var final = new Sql().Append(sql1).Append(sql1);
    Should.NotThrow(() => {
        final.SQL.ShouldBe("TEST\nTEST"); // This fails with StackOverflowException
    });
}

One could argue that Sql instances aren't meant to be used that way, but I've found it very useful to keep them around in certain occasions. One use-case is to cache a fixed part of a query, and then use it to build up more complex queries. The fluent interface, as designed, seems to imply that the above should be valid, since you're always supposedly appending to the first builder in the chain.

The root cause is how the builder works for appending. Appends make up a linked list which is recursed over, building up each builder in sequence. Which means that each node can only be appended-to once. Sometimes you might get into a situation where you append a builder that was already appended, and create a circular reference to itself. This can be detected if you add a guard clause to Append to check if it's not adding itself.

The way I solved it in my project is by changing the Sql builder to use a list to store the appended instances, like so:

@@ -13,7 +13,7 @@
     {
         private object[] _args;
         private object[] _argsFinal;
-        private Sql _rhs;
+        private List<Sql> _rhs = new List<Sql>();
 
         private string _sql;
         private string _sqlFinal;
@@ -91,11 +91,10 @@
         /// <returns>A reference to this builder, allowing for fluent style concatenation</returns>
         public Sql Append(Sql sql)
         {
-            if (_rhs != null)
-                _rhs.Append(sql);
-            else
-                _rhs = sql;
+            if (sql == this)
+                throw new ArgumentException("Can't append self.");
 
+            _rhs.Add(sql);
             _sqlFinal = null;
             return this;
         }
@@ -140,8 +139,11 @@
             }
 
             // Now do rhs
-            if (_rhs != null)
-                _rhs.Build(sb, args, this);
+            var last = this;
+            foreach (var rhs in _rhs) {
+                rhs.Build(sb, args, last);
+                last = rhs;
+            }
         }
 
         /// <summary>

This does have the issue of not picking up on changes in any appended instances after it has been cached, but I think this behavior is better than before. I find it easier to think of the instances as immutable after they'd been created. This passes all the tests and it hasn't caused any unwanted effects in my codebase.

[asherber]

I think very little attention has been paid to the SQL builder over the years; as the original docs stated, the functionality has always been really basic.

For the kind of thing you want to do, my suggestion would be to look at SqlKata and PetaPoco.SqlKata. SqlKata is very well suited towards building up queries from reusable pieces, and I use it extensively. One huge benefit in terms of query construction is that you don't need to add pieces in SQL linguistic order. So you can do things like:

var baseQuery = new Query().Where("created_date", "2022-05-06");

var todaysPosts = baseQuery.Clone().From("posts");
var todaysBooks = baseQuery.Clone().From("books");

In terms of your suggestion, as long as it doesn't break any existing functionality, I think it's a nice idea. Can you put together a pull request?

[ialexj]

I've looked at SqlKata, but I've opted to build my own SELECT generator, which works for my goals of getting intellisense and getting rid of strings for table and column names, but not in a map-to-objects kind of way, but in a generate-classes-that-represent-database-columns kind of way.

All I'm really looking for from the Sql builder is a container for passing around SQL and the corresponding parameters, which it does perfectly, except for this one thing :)

I will look for how to make a pull request.

[asherber]

FWIW, PetaPoco.SqlKata lets SqlKata work just like PetaPoco in terms of auto-generating selects from the corresponding poco classes.

[Table("MyTable")]
class MyClass
{
    public int ID { get; set; }
    public string Name { get; set; }
}

// Both of the samples below will generate 'SELECT [ID], [Name] from [MyTable]'
db.Fetch<MyClass>();

var q = new Query().GenerateSelect<MyClass>();
db.Fetch<MyClass>(q);

[Curlack]

Another suggestion would be to create a new instance from the existing one, before appending. Remember that the Sql class is not immutable or at least treated as such:

`
var sql1 = new Sql("TEST 1");
var sql2 = new Sql("TEST 2");
var sql3 = new Sql("TEST 3");

var final1 = new Sql()
    .Append(new Sql(sql1.SQL, sql1.Arguments))
    .Append(new Sql(sql2.SQL, sql2.Arguments))
    .Append(new Sql(sql3.SQL, sql3.Arguments));
final1.SQL.ShouldBe("TEST 1\nTEST 2\nTEST 3");
sql2.SQL.ShouldBe("TEST 2");

`

An extension method could clean up the syntax a bit e.g.
public static Sql Clone(this Sql self) => new Sql(self.SQL, self.Arguments);

[Ste1io]

@Curlack said exactly what I had in mind also; implementing a deep copy member method that returns this so it can be chained or dropped into some extension methods would be my approach personally. I can add that and whip up some tests for it if that's the consensus.

In the meantime, I'll add an xml doc comment to the class noting the mutability of the internal state when reusing a cached instance, to hopefully avoid any confusion until it gets an upgrade.