DapperLib / Dapper

Dapper - a simple object mapper for .Net
https://www.learndapper.com/
Other
17.58k stars 3.68k forks source link

Dapper will not throw when a param is missing but has the same name as a column. #291

Closed ofirgeller closed 9 years ago

ofirgeller commented 9 years ago

When a query has a param that shares the name of one of the columns, even when the param is missing the query runs without throwing, I think this is a dangerous thing to do.

example:

      var args = new List<ParamClass> { new ParamClass { bad_name_id = 1, name = "mono" } };

       conn.Execute("DELETE FROM test WHERE id = @id AND name = @name",args);

Here records that match the second predict will be deleted because the first predicate ends up being " id = id "and not " id = And " (bad syntax exception, correct thing to do IMHO).

Is there a use case I am missing here for indicating a column name this way?

juliomurta commented 9 years ago

Hello!

I tested your case using the following source:

   public class Test
   {
        public int BadName { get; set; }
        public string Name { get; set; }
   }

    public void TestMissingParam()
    {
        var args = new List<Test> { new Test { BadName = 1, Name = "mono" } }   ;

        int i = connection.Execute("DELETE FROM test WHERE id = @id AND name = @name", args);
    }

And I got the following exception:

     "Must declare the scalar variable \"@id\"."

Are you sure about this?

Cheers

ofirgeller commented 9 years ago

I am sure that it happens yes. I have a piece of running code where the name of the column is item_id and the correct name for the parameter is ItemId, if I write " update table_name where item_id = @item_id " i get no exceptions and the query works. it updates all the records. meaning that the query ends up being item_id = item_id.

mgravell commented 9 years ago

What is your ADO.NET provider here? I strongly suspect this is an issue in your chosen provider (and backend). Dapper doesn't do anything "clever" here at all - it basically does the equivalent of:

cmd.CommandText = "DELETE FROM test WHERE id = @id AND name = @name";
cmd.Parameters.AddWithValue("name", ((object)param.name) ?? DBNull.Value);

(it doesn't add bad_name_id because it knows that isn't in the query)

What happens next is kinda up to the provider and backend. Dapper cannot attempt to sanity check queries, because it works for all flavors of ADO.NET provider - which technically might not even be using SQL at the command language.

ofirgeller commented 9 years ago

You might be right, I am using npgsql.

On Mon, Jun 1, 2015 at 12:13 PM, Marc Gravell notifications@github.com wrote:

What is your ADO.NET provider here? I strongly suspect this is an issue in your chosen provider (and backend).

Reply to this email directly or view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/291#issuecomment-107377167 .

mgravell commented 9 years ago

I'm not sure I can do much about this in the general case; I can't parse every variant of SQL, and even if I could, things like:

declare @foo int = 42;

is not a parameter (it is a local variable, although it could also represent a table-variable), and

where table.Bar = 42 -- @bar - note: now hard coded, see v1.2 release notes

exists (kind of) in the SQL, but does not need to be added to the parameters (because it is in a comment)

I'm open to ideas, but I can't see how dapper could do anything different here.

NickCraver commented 9 years ago

@ofirgeller Did you have any luck here on the current version? I'd like to close this out since it's not a dapper side issue, but wanted to get an update for anyone else stumbling upon this and using npgsql.

ofirgeller commented 9 years ago

haven't upgraded dapper in a while and I moved on by making sure never to use the same name for a param as one of the columns.

NickCraver commented 9 years ago

@ofirgeller I'll go ahead and close this out, but I'm happy to re-open if someone else can repro with details. I was unable to repro the break on my end so not an awesome use of time to track something down which may have been fixed in the many changes since this was opened.

ofirgeller commented 9 years ago

@NickCraver Makes perfect sense to me.

HarveyTaylor commented 5 months ago

Hello @NickCraver , im seeing this issue again in 2.1.44

var somethingelse = 2;
await connection.QueryAsync<int>("select id from metadata where customerid = @CustomerId", new { Other = somethingelse })).ToList();

This returns all metadata id's regardless of customer id's, i would expect it to throw a none matching param error but it doesn't complain.

mgravell commented 5 months ago

Are you sure you have customerid = @CustomerId and not customerid = CustomerId ? because ... if not, that sounds like a broken SQL engine; what is the backend database here?

HarveyTaylor commented 5 months ago

Im sure, and postgres:14.2

Also if you change the sql to "select id from metadata where customerid = @CuuuustomerId" It throws an error. which is what i expect

mgravell commented 5 months ago

Something sounds very weird here; do you have a runnable repro?