Build WHERE clause for search conditions [closed]

Question

Here is a set of classes that are used to build where clause for SQL Server and Oracle for different field types e.g. text, numeric and date.

public interface IConditionBuilder
{
    bool CanHandle(FilterAction filterAction);

    string BuildCondition(SearchCondition filterCondition);
}

public abstract class ConditionBuilder<TContext> : IConditionBuilder where TContext : FieldSearchContext
{
    public abstract string OperatorSymbol { get; }

    public string BuildCondition(SearchCondition searchCondition)
    {
        var conditionBuilder = new StringBuilder();

        var context = searchCondition.GetContext<TContext>();

        conditionBuilder.Append(context.FieldId);
        conditionBuilder.Append(OperatorSymbol);
        conditionBuilder.Append(GetValue(context));

        return conditionBuilder.ToString();
    }

    public abstract bool CanHandle(FilterAction filterAction);

    public abstract object GetValue(TContext context);

}

public class TextLikeConditionBuilder : ConditionBuilder<TextContext>
{
    public override string OperatorSymbol => " LIKE ";

    public override bool CanHandle(FilterAction action) => action == FilterAction.TextLike;

    public override object GetValue(TextContext context)
    {
        if (context.Text == null)
        {
            return null;
        }

        return string.Concat("%", context.Text, "%");
    }
}

public class TextEqualsConditionBuilder : ConditionBuilder<TextContext>
{
    public override string OperatorSymbol => " = ";

    public override bool CanHandle(FilterAction action) => action == FilterAction.TextEqual;

    public override object GetValue(TextContext context)
    {
        if (context.Text == null)
        {
            return null;
        }

        return "'" + context.Text + "'";
    }
}

public class NumericLessThanConditionBuilder : ConditionBuilder<NumericContext>
{
    public override string OperatorSymbol => " < ";

    public override bool CanHandle(FilterAction action) => action == FilterAction.NumericLessThan;

    public override object GetValue(NumericContext context)
    {
        return context.Number;
    }
}

public class DateGreaterThanAndLessThanEqualConditionBuilder : IConditionBuilder
{
    public const string GREATER_THAN = " > ";

    public const string LESS_THAN_EQUAL = " <= ";

    public string BuildCondition(SearchCondition filterCondition)
    {
        var conditionBuilder = new StringBuilder();

        var context = filterCondition.GetContext<DateContext>();

        conditionBuilder.Append(context.FieldId);
        conditionBuilder.Append(GREATER_THAN);
        conditionBuilder.Append("'" + context.FromDate + "'");
        conditionBuilder.Append(LESS_THAN_EQUAL);
        conditionBuilder.Append("'" + context.EndDate + "'");
        return conditionBuilder.ToString();
    }

    public bool CanHandle(FilterAction action) => action == FilterAction.DateGreaterThanLessThan;

}

I want to extend the functionality so that context.FieldId is sanitized before it is used to build the condition statement for e.g. these classes will build a statement like Name = 'Aashish', I want the classes to build statement as [Name] = 'Aashish'. These classes are consumed by other developers so I don't want to break the functionality for consumers as a result of the changes I will make, basically adhere to Open-Closed principle. So, here is how I implemented these changes. Notice how I added a virtual function SanitizeFieldId in ConditionBuilder and DateGreaterThanAndLessThanEqualConditionBuilder.

public abstract class ConditionBuilder<TContext> : IConditionBuilder where TContext : FieldSearchContext
{
    public abstract string OperatorSymbol { get; }

    public string BuildCondition(SearchCondition searchCondition)
    {
        var conditionBuilder = new StringBuilder();

        var context = searchCondition.GetContext<TContext>();

        conditionBuilder.Append(SanitizeFieldId(context.FieldId));
        conditionBuilder.Append(OperatorSymbol);
        conditionBuilder.Append(GetValue(context));

        return conditionBuilder.ToString();
    }

    public abstract bool CanHandle(FilterAction filterAction);

    public abstract object GetValue(TContext context);

    protected virtual string SanitizeFieldId(string fieldId)
    {
        return fieldId;
    }
}

public class DateGreaterThanAndLessThanEqualConditionBuilder : IConditionBuilder
{
    public const string GREATER_THAN = " > ";

    public const string LESS_THAN_EQUAL = " <= ";

    public string BuildCondition(SearchCondition filterCondition)
    {
        var conditionBuilder = new StringBuilder();

        var context = filterCondition.GetContext<DateContext>();

        conditionBuilder.Append(SanitizeFieldId(context.FieldId));
        conditionBuilder.Append(GREATER_THAN);
        conditionBuilder.Append("'" + context.FromDate + "'");
        conditionBuilder.Append(LESS_THAN_EQUAL);
        conditionBuilder.Append("'" + context.EndDate + "'");
        return conditionBuilder.ToString();
    }

    public bool CanHandle(FilterAction action) => action == FilterAction.DateGreaterThanLessThan;

    protected virtual string SanitizeFieldId(string fieldId)
    {
        return fieldId;
    }
}

public class SanitizedFieldConditionBuiler<TContext> : ConditionBuilder<TContext> where TContext : FieldSearchContext
{
    private ConditionBuilder<TContext> _baseConditionBuilder;
    private IColumnSanitizer _columnSanitizer;

    public SanitizedFieldConditionBuiler(ConditionBuilder<TContext> baseConditionBuilder, IColumnSanitizer columnSanitizer)
    {
        _baseConditionBuilder = baseConditionBuilder;
        _columnSanitizer = columnSanitizer;
    }

    public override string OperatorSymbol => _baseConditionBuilder.OperatorSymbol;

    public override bool CanHandle(FilterAction filterAction) => _baseConditionBuilder.CanHandle(filterAction);

    public override object GetValue(TContext context) => _baseConditionBuilder.GetValue(context);

    protected override string SanitizeFieldId(string fieldId)
    {
        return _columnSanitizer.Sanitize(fieldId);
    }
}

public class SanitizedDateFieldGreaterThanAndLessThanEqualConditionBuilder : DateGreaterThanAndLessThanEqualConditionBuilder
{
    private IColumnSanitizer _columnSanitizer;

    public SanitizedDateFieldGreaterThanAndLessThanEqualConditionBuilder(IColumnSanitizer columnSanitizer)
    {
        _columnSanitizer = columnSanitizer;
    }

    protected override string SanitizeFieldId(string fieldId)
    {
        return _columnSanitizer.Sanitize(fieldId);
    }
}

I use extension methods to initialize SanitizedFieldConditionBuilerand SanitizedDateFieldGreaterThanAndLessThanEqualConditionBuilderas shown below:

public static class Extensions
    {
        public static SanitizedFieldConditionBuiler<TContext> SanitizeField<TContext>(this ConditionBuilder<TContext> source, IColumnSanitizer columnSanitizer) where TContext : FieldSearchContext
        {
            return new SanitizedFieldConditionBuiler<TContext>(source, columnSanitizer);
        }

        public static SanitizedDateFieldGreaterThanAndLessThanEqualConditionBuilder SanitizeField(this IConditionBuilder source, IColumnSanitizer columnSanitizer)
        {
            return new SanitizedDateFieldGreaterThanAndLessThanEqualConditionBuilder(columnSanitizer);
        }

    }

The Sanitization is available by means of an interface IColumnSanitizer and has two different implementations, for Sql Server and Oracle respectively

 public interface IColumnSanitizer
    {
        string Sanitize(string columnName);
    }

    public class SqlSanitizer : IColumnSanitizer
    {
        public string Sanitize(string columnName)
        {
            return "[" + columnName + "]";
        }
    }

    public class OracleSanitizer : IColumnSanitizer
    {
        public string Sanitize(string columnName)
        {
            return "\"" + columnName + "\"";
        }
    }

Below is how context classes are implemented:

public abstract class FieldSearchContext
{
    public virtual string FieldId { get; }

    protected FieldSearchContext(string fieldId)
    {
        FieldId = fieldId;
    }
}

public class DateContext : FieldSearchContext
{
    public DateContext(string fieldId, DateTime? fromDate, DateTime? endDate) : base(fieldId)
    {
        FromDate = fromDate;
        EndDate = endDate;
    }

    public DateTime? FromDate { get; }

    public DateTime? EndDate { get; }
}

public class TextContext : FieldSearchContext
{
    public TextContext(string fieldId, string text) : base(fieldId)
    {
        Text = text;
    }

    public string Text { get; }
}

public class NumericContext : FieldSearchContext
{
    public NumericContext(string fieldId, decimal number) : base(fieldId)
    {
        Number = number;
    }

    public decimal Number { get; }
}

These changes work perfectly fine but I want to find out if this can be achieved in a different and better way.

Use the code below to see it in action:

 class Program
    {
        static void Main(string[] args)
        {
            var conditions = new List<SearchCondition>()
            {
                new SearchCondition(new NumericContext("Numeric Field", 1234), FilterAction.NumericLessThan),
                new SearchCondition(new TextContext("Text Field", "ASDF"), FilterAction.TextEqual),
                new SearchCondition(new TextContext("Text Field", "QWERTY"), FilterAction.TextLike),
                new SearchCondition(new DateContext("Date Field", DateTime.Now, DateTime.Now.AddYears(1)), FilterAction.DateGreaterThanLessThan)
            };

            Console.WriteLine(BuildWhereClause(Operation.AND, conditions));
            Console.Read();
        }

        private static string BuildWhereClause(Operation operation, IList<SearchCondition> conditions)
        {
            var returnValue = new List<string>();
            var conditionBuilders = new List<IConditionBuilder>()
            {
                new TextEqualsConditionBuilder().SanitizeField(new SqlSanitizer()),
                new NumericLessThanConditionBuilder().SanitizeField(new SqlSanitizer()),
                new TextLikeConditionBuilder().SanitizeField(new SqlSanitizer()),
                new DateGreaterThanAndLessThanEqualConditionBuilder().SanitizeField(new SqlSanitizer())
            };

            foreach (var condition in conditions)
            {
                var context = condition.GetContext<FieldSearchContext>();
                var conditionBuilder = conditionBuilders.FirstOrDefault(u => u.CanHandle(condition.FilterAction));
                returnValue.Add(conditionBuilder.BuildCondition(condition));
            }

            if (returnValue.Any())
                return string.Join(Convert.ToString(" " + operation + " "), returnValue);

            return string.Empty;
        }
    }

    enum Operation : int
    {
        AND = 1,
        OR = 2
    }

EDIT:

Added SearchCondition

public class SearchCondition
    {
        private readonly FieldSearchContext _fieldSearchContext;

        private FilterAction _filterAction;

        public FilterAction FilterAction
        {
            get { return _filterAction; }
        }

        public SearchCondition(FieldSearchContext fieldSearchContext, FilterAction action)
        {
            _fieldSearchContext = fieldSearchContext;
            _filterAction = action;
        }

        public T GetContext<T>() where T : FieldSearchContext
        {
            return (T)_fieldSearchContext;
        }
    }

Answer 1

Sanitizer is a dangerous misnommer IMO. A better name could be NameQuoter, since that's what it does: it uses the RDBMS-specific syntax for quoting identifiers - SQL Server using square brackets, MySQL using backticks, Oracle using backslashes: this has nothing to do with sanitizing, which from what I can tell is essentially impossible to achieve with this framework that I would dub "SQL Injection Framework".

FieldId is also a misnommer - I don't know about Oracle, but on SQL Server every database object has an ID, and I would read "field ID" as a value that's referring to that ID. What you have is a ColumnName, not an ID.

Context is also a confusing name: in Entity Framework, Context refers to the unit of work; it encapsulates a transaction and exposes the repositories - as far as client code is concernd, it is the database connection... anyone remotely familiar with standard .NET data access code will raise an eyebrow at "context" being used to refer to what's essentially a wrapper object for various types of values. Why can't a decimal be null, but a DateTime can?

This is a problem:

public abstract class FieldSearchContext
{
    public virtual string FieldId { get; }

    protected FieldSearchContext(string fieldId)
    {
        FieldId = fieldId;
    }
}

You're assigning a virtual property in the base constructor. That property should not be virtual at all. Here's why it's a problem:

public class BrokenSearchContext : FieldSearchContext
{
    public BrokenSearchContext() : base("Foo") { }

    public override string FieldId => "Bar";
}

The base constructor runs first, receives "Foo", invokes the FieldId property... which is overridden in the derived class... as an immutable getter. How is the base class assigning its FieldId now? Right: it doesn't... and that merely makes things confusing in this case (value read isn't the value written) - but in other situations it could mean a bug that's very hard to track down.

Avoid virtual member calls in a constructors - might be innocuous in this particular case, but one day you'll be invoking a side-effecting virtual method in a base constructor, and you're not going to like it. FieldId has no reason to be virtual in the first place.

But of all problems, this is the single most dangerous one:

public override object GetValue(TextContext context)
{
    if (context.Text == null)
    {
        return null;
    }

    return "'" + context.Text + "'";
}

See, I like Irish whiskey, and code like this makes me want to order a double. What happens when context.Text is Elizabeth O'Connor? Or Patrick O'Neil?

Or Robert';DROP TABLE Students;--?

I'm sorry to say, the most pressing issue with this code isn't how bloated this whole "criteria builder" tooling works, nor how well it does or doesn't adhere to OCP: the most pressing issue with this code is that it's literally a SQL Injection Framework, making it child's play to generate SQL statements that can - and if this goes anywhere near a public-facing client, eventually will - contain malicious data... all while the consuming C# code looks and reads very much like it's perfectly safe & secure.

The biggest problem with this code, is that it exists - concatenating parameter values into SQL statements is not the job of the client connection. It's the job of the server, and doing this in a secure way involves commands and parameters - not string concatenations.

I wouldn't worry about breaking existing code - the existing code is already broken, beyond repair.

To be clear: the solution isn't to escape single quotes or make sure SQL keywords aren't present in the string - the solution is to stop concatenating WHERE clauses in SQL strings.

Consider using an ORM, e.g. Entity Framework - or Dapper.NET if you want a lightweight but performant solution, to generate properly parameterized SQL statements without any concatenation.

Answer 2

Just one thing...

Use db-provider's sanitizer

There is already a sanitizer that the DbCommandBuilder provides. You can use it like this:

using (var commandBuilder = DbProviderFactories.GetFactory(sqlConnection).CreateCommandBuilder())
{
    return commandBuilder.QuoteIdentifier(name);
}

I'm pretty sure the Oracle provider has it too (when you install its NuGet package). You should use these instead of inventing your own.