Translating an inputted query to SQL

Question

I'm working on a small RestApi that would be able to:

1. Interpret different query languages like (...)/countries?where="..."&select="..." or (...)/GetCountries?ids="1,2,3"
2. Translate theses queries to SQL
3. Pass the SQL command to dapper and return the result

Note this API will be used by other services and that it won't be exposed to the "public".

I have a first version running but I'm not satisfied with some aspects of my solution...

So how does it work? When the rest api (ASP.NET CORE) receives a query, it will try to find a parser able to interpret it. Currently I have only one simple parser.

public interface IQueryParser
{
    Query Parse(QueryContext context);
}

public class DqlQueryParser : IQueryParser
{
    private readonly Func<QueryContext, bool> _predicate;
    private readonly IQueryOperatorParser _parser;

    public DqlQueryParser(Func<QueryContext, bool> predicate, IQueryOperatorParser parser)
    {
        _predicate = predicate ?? (x => true);
        _parser = parser;
    }

    public Query Parse(QueryContext context)
    {
        if (context == null)
            throw new ArgumentNullException(nameof(context));
        if (!string.Equals(context.QueryLanguage, "dql", StringComparison.OrdinalIgnoreCase) || !_predicate(context))
            return null;
        if (string.IsNullOrEmpty(context.Query))
            throw new QueryParserException(context, "The supplied query context does not contain a valid query string. It shouldn't empty or null");

        var type = context.Query.GetEntityType();
        if (type == null) throw new QueryParserException(context, $"The supplied '{context.Query}' entity is missing or unknown");

        switch (context.Method)
        {
            case "GET":
                return new DqlSelectQuery(context.Query, type, _parser?.Parse(context) ?? Enumerable.Empty<QueryOperator>());
            case "POST":
                return new DqlInsertQuery(context.Query, type, (JObject)JsonConvert.DeserializeObject(context.Body));
            case "DELETE":
                return new DqlDeleteQuery(context.Query, type, _parser?.Parse(context) ?? Enumerable.Empty<QueryOperator>());
            case "PATCH":
            case "PUT":
                return new DqlUpdateQuery(context.Query, type, (JObject)JsonConvert.DeserializeObject(context.Body));
            default:
                throw new QueryParserException(context, $"The '{context.Method}' method is not supported");
        }
    }
}

Each parser can return a sub class of Query. In my case it may be an instance of type DqlSelectQuery, DqlInsertQuery, DqlDeleteQuery, DqlUpdateQuery.

public class DqlDeleteQuery : DqlSelectQuery
{
    public DqlDeleteQuery(string raw, Type targetEntityType, IEnumerable<QueryOperator> operators)
        : base(raw, targetEntityType, operators) { }
}

public class DqlSelectQuery : Query
{
    public List<QueryOperator> Operators { get; }

    public DqlSelectQuery(string raw, Type targetEntityType, IEnumerable<QueryOperator> operators)
        : base(raw, targetEntityType)
    {
        Operators = new List<QueryOperator>(operators);
    }
}

(...)

In my Select/Delete queries, I'm using the concept of operators. It can be seen as a way to add extra behaviors to a query (select some fields, filter the result, join a table...).

Once the query fully parsed, I identity a translator able to process this query. Indeed, in my current design I may have "lots" of parsers but only very few translators.

public interface IQueryTranslator
{
    Type SupportedType { get; }
    string Translate(Query query);
}

public class DqlDeleteQueryTranslator : Translator<DqlDeleteQuery>
{
    protected override string Translate(DqlDeleteQuery query)
    {
        var sb = new StringBuilder();
        var ops = query.Operators;

        sb.AppendLine($"DELETE FROM {query.TargetEntityType.GetSqlEntityName()}");
        if (ops.AnyOfType<DqlWhereOperator>())
            sb.AppendLine($"WHERE {ops.SingleOfType<DqlWhereOperator>().Condition}");

        return sb.ToString().Trim();
    }
}   

public class DqlSelectQueryTranslator : Translator<DqlSelectQuery>
{
    protected override string Translate(DqlSelectQuery query)
    {
        var ops = query.Operators;
        var sb = new StringBuilder();

        sb.AppendLine(ops.AnyOfType<DqlSelectOperator>()
            ? $"SELECT {string.Join(",", ops.SingleOfType<DqlSelectOperator>().Fields)} FROM {query.TargetEntityType.GetSqlEntityName()}"
            : $"SELECT * FROM {query.TargetEntityType.GetSqlEntityName()}");

        if (ops.AnyOfType<DqlIncludeOperator>())
        {
            foreach (var dst in ops.OfType<DqlIncludeOperator>())
            {
                var src = query.TargetEntityType;
                var type = src.GetRelationship<RelationshipAttribute>(dst.Type);

                if (type == null)
                    throw new NotSupportedException($"Unknown relationship between {src.Name} and {dst.Type.Name}");

                if (type is OneToManyAttribute)
                    sb.AppendLine($"JOIN {dst.Type.GetSqlEntityName()} {dst.Alias} ON {dst.Name}.{src.GetPrimaryKeys().First()} = {src.Name}.{src.GetPrimaryKeys().First()}");
                if (type is ManyToOneAttribute)
                    sb.AppendLine($"JOIN {dst.Type.GetSqlEntityName()} {dst.Alias} ON {dst.Name}.{dst.Type.GetPrimaryKeys().First()} = {src.Name}.{dst.Type.GetPrimaryKeys().First()}");
                if (type is ManyToManyAttribute)
                {
                    string joinName = $"_{src.Name}{dst.Type.Name}";
                    sb.AppendLine($"JOIN {src.GetRelationship<ManyToManyAttribute>(dst.Type).Join} {joinName} ON {joinName}.{src.GetPrimaryKeys().First()} = {src.Name}.{src.GetPrimaryKeys().First()}");
                    sb.AppendLine($"JOIN {dst.Type.GetSqlEntityName()} {dst.Alias} ON {dst.Name}.{dst.Type.GetPrimaryKeys().First()} = {joinName}.{dst.Type.GetPrimaryKeys().First()}");
                }
            }
        }
        if (ops.AnyOfType<DqlWhereOperator>())
            sb.AppendLine($"WHERE {ops.SingleOfType<DqlWhereOperator>().Condition}");
        if (ops.AnyOfType<DqlOffsetOperator>())
            sb.AppendLine($"OFFSET {ops.SingleOfType<DqlOffsetOperator>().Offset} ROWS");
        if (ops.AnyOfType<DqlLimitOperator>())
            sb.AppendLine($"FETCH NEXT {ops.SingleOfType<DqlLimitOperator>().Limit} ROWS ONLY");
        return sb.ToString().Trim();
    }
}

I'm not very happy about these pieces of code. Currently to handle new operators, I need to add more code in this class. It's not exactly scalable or readable. I tried to find some SQL generator but didn't find any!

My guess would be associate a class designed to translate each individual operators. It would be cleaner but then I would need to add many classes with one line of code. Do you have good design patterns/ideas that could me improve my code?

Answer 1

A proper code review would need more details about your design, especially about DqlSelectQuery (especially purpose and intended use), then I'd limit myself to few things in your visible code.

---

First things I'd consider to change is string interpolation. In this moment you're, in my opinion, abusing this feature. It's very handy for short interpolated expressions but you're extensively using this language feature with long expressions and it's killing readability. Don't go to refactor this now...

Major problem is you're building SQL code by hand. Biggest problem is that it's completely open to SQL injection (for example check WHERE, OFFSET and FETCH NEXT). As you can imagine this is a terribly bad thing. OFFSET and FETCH NEXT are integers and they may be sanitized however I don't like to reinvent the wheel (with high chances to make it wrong). What I'd like to return from a function like this is the SQL command text plus an object that can be used for SQL parameters (in conjunction with a mapper, for example Dapper). Something like this:

dynamic parameters = new ExpandoObject();
if (ops.AnyOfType<DqlOffsetOperator>())
{
    parameters.Offset = ops.FirstOfType<DqlOffsetOperator>().Offset;
    sb.AppendLine("OFFSET @Offset ROWS");
}

if (ops.AnyOfType<DqlLimitOperator>())
{
    parameters.Limit = ops.FirstOfType<DqlLimitOperator>().Limit;
    sb.AppendLine("FETCH NEXT @Limit ROWS ONLY");
}

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. You may like to read Building LINQ Queries at Runtime in C# for inspiration. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

void OperatorOffset(StringBuilder sql, dynamic parameters)
{
    if (!_query.Operators.AnyOfType<DqlOffsetOperator>())
        return;

    parameters.Offset = ops.FirstOfType<DqlOffsetOperator>().Offset;
    sql.AppendLine("OFFSET @Offset ROWS");
}

Note that _query is an instance field (the parameter of Translate() method). Now I'd like to address another problem: you're roaming around with a SQL string and errors will be catched only at run-time. Multiple places with string concatenation means that you have to test every single possible code path with all possible relevant values. Nice to have but I'd like to make it little bit more safe! You may introduce a command builder class, it's pretty handy especially for JOIN and SELECT:

var command = new CommandBuilder();

if (query.Operators.AnyOfType<DqlSelectOperator>())
{
    command.AddSelect(ops.FirstOfType<DqlSelectOperator>().Fields,
        query.TargetEntityType.Name);
}
else
{
    command.AddSelect(query.TargetEntityType.Name);
}

Where AddSelect() may be something like this:

private const string SqlCommandSelect = "SELECT";
private const string SqlFromClause = "FROM";

public CommandBuilder AddSelect(IEnumerable<string> fields, string tableName)
{
    _sql.AppendLine(SqlCommandSelect)
        .AppendLine(String.Join(",\n", fields.Select(x => QuoteIdentifier(x)))
        .AppendLine($"{SqlFromClause} {QuoteIdentifier(tableName)}");

    return this;
}

Of course code is not complete but I guess you get the point. Note the call to QuoteIdentifier() (to be written), identifiers must be properly quoted! The two overloaded versions may even be merged (if (fields?.Count()) ?? 0 == 0 then just add *). Fields may be prefixed with table alias (useful in case of complex joins to avoid ambiguity.)

Now that you have a CommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>. Something similar, but with completely different purpose, can be seen in SqlCommandBuilder class.

I think you're almost done but there is still a thing that hurts me. All those AnyOfType() and FirstOfType(). Can you specify more than one DqlSelectOperator operator? Isn't it an error? Yes, it is then caller must be warned of this. You introduced your own FirstOfType<T>() extension method then also add SingleOfType<T>() (or use plain syntax .OfType<T>().Single()): if query is broken then it won't silently do something (probably unexpected).

---

Last note: things may be more complicate if you're using this interface to abstract queries to different repositories (let's say you have a query language for relational databases, NoSQL databases, XML databases/files, JSON files, etc...) In this caseyou need to return ICommandBuilder and you will need few specializations (one for each target format):

public interface IQueryTranslator
{
    Type SupportedType { get; }
    ICommandBuilder Translate(Query query);
}

Don't be tempted to always return a string, parameters validation/sanitization isn't an easy thing.