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:
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?
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.