vapor/sql-kit

Allow CustomStringConvertible to be used in SQLQueryString interpolations

tarag opened this issue · 2 comments

tarag commented

It would be great to add the capability for CustomStringConvertible during the creation of SQLQueryString through interpolation.

Today, we are obliged to add .description which is not very swifty:

let myField : FieldKey = foo
sqlDB.raw("SELECT \(myField.description) FROM my_table")

I handled that through a simple extension but I think it should be included...

extension SQLQueryString: StringInterpolationProtocol {
    mutating public func appendInterpolation(_ stringConvertible: CustomStringConvertible) {
        appendLiteral(stringConvertible.description)
    }
}

After some discussion in #development on Discord, I think the potential bugs this could cause outweigh the benefit of convenience. Tons of types are CustomStringConvertible and very few of them should be bound directly to the query string. Most of the time \(bind:) should be used (which accepts Encodable values). In other words, needing to add .description to these types explicitly will likely help prevent bugs.

For your example using FieldKey, this is a special case that I think could be solved more cleanly:

extension FieldKey: SQLExpression {
    public func serialize(to serializer: inout SQLSerializer) {
        switch self {
        case .id:
            serializer.write("id")
        case .aggregate:
            serializer.write("aggregate")
        case .prefix(let prefix, let key):
            prefix.serialize(to: &serializer)
            key.serialize(to: &serializer)
        case .string(let string):
            serializer.write(string)
        }
    }
}

By conforming FieldKey to SQLExpression, you allow it to be embedded directly in \().

let myField : FieldKey = foo
sqlDB.raw("SELECT \(myField) FROM my_table")

This has the added benefit of not using .description which is not explicitly meant to be used in query strings. It's meant for debugging and its string value may change in the future.

This conformance might be worth adding to the FluentSQL module.

tarag commented

Thanks for the insight, I'll follow the evolution of related issues.