Best practices for getting data by different criteria

Question

I have one BL function and one SQL stored procedure (let's name it: getPersonDetailsById) which returns person details by passing his PersonId.

Now I need to get the same details (data table) but by passing the person's Email because for the same flow I don't have the person Id.

What's the right way to do it:

Option 1

Add new BL function and new stored procedure (let's name it: getPersonIdByEmail) that will get personId from the database by passing the Person's Email and then use the existing function (getPersonDetailsById) to get the Person Details?

In other words, Creating a SP "In the Middle" that will just return PersonId as scalar result and then call to the getPersonDetailsById SP with the returned personId. (2 DB Calls but one central SP with no duplication).

Option 2

Change the existing SP (getPersonDetailsById) so it will get Email as additional parameter and pass null for PersonId.

Will happy to learn another better option.

Answer 1

It seems like in your app, having two separate functions (Option 1) is a simple enough solution because you have these distinct ways of getting user details at different places in your application. If you wanted to have a single function that has to determine whether you're giving it an ID or an email, that type of logic isn't too difficult. You can even create some sort of UserItentificationType interface and have various uses of it. The basic rule of thumb here is keeping your business logic in this part of your application.

This doesn't really hold up on the database side. Two stored procedures would be preferable. There's no need to have this type of business logic in the database and most of all, it can also cause a performance problem. I'm sure your application will be accessing user's details a lot, so why add any complexity or performance hindrance.

Queries get messy when you start checking for null parameters and making decisions on a WHERE clause using this field in one case OR using another field. RDBMS really like having everything all set up beforehand and very simple. Indexes get utilized more consistently. Other statistics and query plans get cached.

Of course these two procs will be nearly identical (They'll return the same fields from the same table(s)/view(s)), but don't rely too much on a programmer's tendency to avoid duplication and all that other stuff. As much as I like to write my code for a programmer, sometimes you have to write a query for the database. Sometimes the database will help you out so much more than the benefits of cleaner code.

Answer 2

I am strongly against procedures, I don't believe any business logic (including logic that says in which format you should query a database) belongs to the database, but to the code itself.

Even though in your case the simplest solution is to simply create a new function in your code, make a getUsersByEmail procedure in the database and call this newly created procedure from the newly created function in code, there are two main cases why I don't like database procedures:

• database procedures are very difficult to debug
• having database procedures forces you to learn additional programming language

You are currently suggesting two options:

1. Add new BL function and new stored procedure (let's name it: getPersonIdByEmail) that will get personId from the database by passing the Person's Email and then use the existing function (getPersonDetailsById) to get the Person Details?

2. Change the existing SP (getPersonDetailsById) so it will get Email as additional parameter and pass null for PersonId.

If you are considering a solution out of only those two suggested options, then performance-wise it does not really matter which one you choose.

Although dynamic sql queries need to be recompiled every time, in your case you will only be choosing either one or second column and you should see very little to performance drop.

Dynamic sql queries are a little more susceptible to sql injection, that is if you allow the user to choose which column value should be inserted into the code calling the procedure containing the dynamic sql.

An example is a user choses a value from a drop-down and you take the selected value and pass it directly to the procedure.

Luckily, this is easy to fix, by limiting the value of the input to a specific ids, which represent a column in your database, and if a user passes an id which does couldn't be found, an error is returned.

With the first approach (creating a getUsersByEmail procedure), while the query itself cannot be heavily modified by nature, you are still required to have some decision in code, which chooses either one or the other procedure, which is pretty much the same as the second approach, where you limit the user to use only specific values.

Using the first approach, you get a slight benefit of performance increase at the cost of storing pretty much the same sql code twice in the database.

Using the second approach the performance will be slightly (really slightly) worse, you kind of avoid code duplication in the database procedure but you really should add additional checks in your code due to security issues.

So if you really only consider one of the two options, it does not really matter which one you choose, but if you decide to choose the dynamic sql version, make sure to add some security check in your code, so that at least the column names are passed as constants you define and not as strings the user can change.

---

If you are open to different suggestions, I'd suggest a third approach: Not having stored procedure at all and moving it to code.

As I said in the begining of this answer, I don't believe any business logic belongs to database and this seems like business logic (you're deciding in which way you want to query the database based on some rule).

At the very end, you'll have a dynamically created sql query and this will be hidden behind two facades, providing public interface, for either querying by id or email.

Here's a pseudo C# implementation:

internal class DynamicPersonDetails<T>
{
    private Database db;

    public DynamicPersonDetails(Database database)
    {
        this.db= database;
    }

    public DbResult GetByColumnName(string columnName, T value)
    {
        var sql =
            "SELECT * FROM person_detail WHERE "+columnName+"="value.toString();

        return this.db.DoQuery(sql).FetchSingle();
    }
}

public class EmailPersonDetails
{
    private DynamicPersonDetails<string> dpd;

    public EmailPersonDetails(DynamicpersonDetails<string> dpd)
    {
        this.dpd = dpd;
    }

    public DbResult GetPersonDetailsByEmail(string email)
    {
        return this.dpd.GetByColumnName("email", email);
    }
}

public class IdPersonDetails
{
    private DynamicPersonDetails<int> dpd;

    public EmailPersonDetails(DynamicpersonDetails<int> dpd)
    {
        this.dpd = dpd;
    }

    public DbResult GetPersonDetailsById(int id)
    {
        return this.dpd.GetByColumnName("id", id);
    }
}

The class where the sql is built should be hidden from the outside world and you should only expose to the world the classes with interfaces representing the available columns.

In reality you most likely wouldn't construct the query in DynamicPersonDetails the way it's done here, but have prepared statements instead (mainly to prevent SQLi)

Naturally you could even group both methods, GetPersonDetailsById and GetPersonDetailsByEmail to one class and not have them scattered between two as I am showing.

What you get by using this approach:

• code very easy to debug (you're debugging the code you're fluent in, be it C#, Java, PHP or C++)
• if you use prepared statements, you get performance increase on multi querries
• if you use prepared statements, you get security boost and pretty much deny sql injection

You get the benefit of no-code duplication (at the sql level) while also having code which is robust and secure to run against the database.

And most importantly, if you even decide to switch to an engine which does not support stored procedures, you can have the EmailPersonDetails and IdPersonDetails implement a IEmailPersonDetails interface or IIdPersonDetails respectively, make your application depend on this one instead and swap out the dependency when needed, being totally database-ignorant.

---

By the way, REST/SOAP APIs do exactly what I described, they hide database dependency behind an abstract web service layer - pretty much moving all procedures to code and making certain endpoints accessible.

Before web services became common, you could mimick the same behaviour by denying database users access to tables directly and granting them permission to only execute certain procedures.

But this didn't feel right, because then you couldn't have any cache layer in between the client of the database and the database server itself, which resulted in heavy load.

Now since web services became so popular, I can hardly imagine anyone going back to the old way.