Is this a leaky abstraction?

Question

Is specifying a field name on middle-tier a leaky abstraction?

I feel it is too cumbersome to create a separate function just for each field, even if it is code-generated.

        public bool Assortment_IsValueExistingOnOtherRecord(
            Dictionary<string, object> exceptPkColumns,
            string field, object value
            )
        {
            var c = Connect();

            var dt = new DataTable();

            string sanitizedField = field.Replace("'","");

            var daAssortment = new SqlDataAdapter(
               string.Format(
@"SELECT 1 FROM assortment
WHERE 
/* do I violate any good programming practice here? */ [{0}] = @value 
AND NOT (assortment_id = @assortment_id)",  field), c);

            daAssortment.SelectCommand.Parameters.Add("assortment_id", exceptPkColumns["assortment_id"]);
            daAssortment.SelectCommand.Parameters.Add("value", value);
            daAssortment.Fill(dt);

            return dt.Rows.Count == 1;
        }

Answer 1

[{0}] = @value

This is fine as long as appropriate measures are taken. "Sanitization" is not among those measures.

Option1 : Do you have an implicit trust with the provider of the field, such that you know they will never ever mess with you? For example, if you (as the author of this module) are the provider of the field, you can trust yourself.

Option2 : Can you make a enum/lookup that maps requested fields to strings? Then the "client" of this code can only request things in the enum and you (the author of this module) become the field provider by mapping their requested enum value to a known string.

Option3 : If the fields change over time and you don't want to lock in with an enum, can you send the string value into the database for confirmation?

SELECT name
FROM syscolumns
WHERE name = @name

This maps the provided name to a known string that comes from the database and must be safe.

Answer 2

From an encapsulation perspective, there is nothing wrong with what you wrote.

However, as "David B" alluded to (he was a little vauge about it), there are some security issues you need to be aware of.

The dynamic SQL you are generating is subject to something called a "SQL Injection attack". Imagine, for example, that the value of field was the following string:

assortment_id] = 0 and assortment_id = 1 
delete from assortment
select 1 from assortment where [assortment_id

this would result in the database executing 3 queries:

1. A dummy select query that brought back no results
2. A delete query that deleted everything in the assortment table
3. A real select query

That could, obviously, cause some problems.

If the value of "field" is ever specified via:

1. A user interface field
2. A web page form submission
3. From a web service client
4. Loaded from a file on disk
5. Or in any way other than a constant value supplied by you in code

Then you need to validate the data to make sure it is a valid field value before running any queries. Otherwise you are vulnerable to attack.

Answer 3

Use an Expression, and you'll have compile time (and Sql runtime) safety:

public bool Assortment_IsValueExistingOnOtherRecord<TValue>(
        Dictionary<string, object> exceptPks,
        Expression<Func<Assortment, TValue>> expression,
        TValue value
        )

    propExp = expression.Body as MemberExpression;
    if (propExp == null) throw new ArgumentException();
    string field = propExp.Member.Name; 

    /* blah, blah */
}

/* Calling code */
bool exists = Assortment_IsValueExistingOnOtherRecord(
                exceptPKs, 
                (a => a.Property1),
                "property1Value"
              );

With the generic TValue, you also get a strongly typed value that matches the property - avoiding the occasional brain dead cast that your RDBMS will do for you.

You could have the entire equals expression, or even add support for < and > - but, then you'd just be inventing LINQ to homegrown ORM. ;)

As an aside, the exceptPKs seems a bit odd...but perhaps it makes sense in context?

Answer 4

Yes. All abstractions leak.

Now then, as for safety, if you don't explicitly trust all callers not to goof (or seek to actively attack you), you had better have an emum of all possible fields and validate the incoming parameter against that list.