i've been trying to modify an sql command for postgresql on c#. The line in comment quotes is the one working, but i need to change the set from pre-defined to variable. Although after
using the same way to define it as a variable, with a text given by the user on another form, it doesnt seem to work. Which is the correct way to replace the "first_name" with a variable ?
String oldname = Actor.oldname;
String newname = Actor.newname;
String column = Actor.columnname;
try
{
string connstring = "Server=127.0.0.1; Port=5432; User Id=postgres; Password=72677267; Database=imdb;";
NpgsqlConnection connection = new NpgsqlConnection(connstring);
connection.Open();
//NpgsqlCommand command = new NpgsqlCommand("UPDATE actor SET first_name = " + newname + " WHERE first_name =" + oldname + "", connection);
NpgsqlCommand command = new NpgsqlCommand("UPDATE actor SET " + column + " = " + newname + " WHERE " + column + " =" + oldname + "", connection);
NpgsqlDataReader dataReader = command.ExecuteReader();
connection.Close();
return dataItems;
}
catch (Exception msg)
{
MessageBox.Show(msg.ToString());
throw;
}
You have a few issues with your code sample above that should prevent it from working. I'm a little shocked the commented code line works.
- You are not quoting the values you update
- Even if you did quote them, you should use parameters instead.
- The datareader is for reading. If you are executing DML, you should use
ExecuteNonQuery
. If you have returning values, there may be cause for a datareader, but in this case you don't appear to need it.
All that said, dynamic SQL is sometimes unavoidable, but I'd recommend every possible recourse before giving up and making dynamic SQL. If you have to, one way to mitigate this might be to have a finite number of options, so rather than letting them update ANY field, let them choose a field from a list of options.
This is still dynamic SQL, but it's at least parameterized, and the injection possibilities are at least limited to the method, which makes any injection very unlikely.
public enum ActorField
{
FirstName,
LastName,
Salutation
}
public void UpdateActorField(string OldName, string NewName, ActorField FieldId)
{
string sql = "update actor set {0} = :NEW_NAME where {0} = :OLD_NAME";
switch (FieldId)
{
case ActorField.FirstName
sql = string.Format(sql, "first_name");
break;
case ActorField.LastName
sql = string.Format(sql, "last_name");
break;
case ActorField.Salutation
sql = string.Format(sql, "salutation");
break;
}
using (NpgsqlCommand cmd = new NpgsqlCommand(sql, connection))
{
cmd.Parameters.AddWithValue("NEW_NAME", NewName);
cmd.Parameters.AddWithValue("OLD_NAME", OldName);
int updatedRows = cmd.ExecuteNonQuery();
}
}
There are cuter ways to do this with a single SQL statement and with no dynamic SQL, but they add a lot of complexity to what looks like it should be a simple problem.