sqlkata / querybuilder

SQL query builder, written in c#, helps you build complex queries easily, supports SqlServer, MySql, PostgreSql, Oracle, Sqlite and Firebird
https://sqlkata.com
MIT License
3.13k stars 502 forks source link

Query should not automatically treat null as where null #174

Closed ahmad-moussawi closed 5 years ago

ahmad-moussawi commented 5 years ago

currently the query will treat Where(col, null) as WhereNull(col) but this can lead to unsafe query execution. for example


var name = getName();

// getName() returned null instead of the real name

// users that does not have names will get deleted instead
db.Query("Table").Where("name", name).Delete();
Shadetheartist commented 5 years ago

This seems like perfectly normal behavior to me. The responsibility is on the consuming code to ensure that the input is not null. But, if really don't like that, you can certainly fork the repo and change the behavior for your own personal use.

ceastwood commented 5 years ago

@Shadetheartist

You do realize @ahmad-moussawi is the repo owner? 😆

Shadetheartist commented 5 years ago

Oh no i didn't, just saw he was a 'contributor'. I guess he wouldn't need to fork it lol. Even so i think the behavior makes sense as it is.

ahmad-moussawi commented 5 years ago

@Shadetheartist makes sense, I will keep it open to get more feedback :)

tlaguz commented 5 years ago

I would expect this:

db.Query("Table").Where("name", null).Delete();

to be compiled as:

DELETE FROM "Table" WHERE "name" IS NULL;

As I understand correctly this library is meant to be a SQL builder, so it should introduce minimum abstraction.

for7raid commented 5 years ago

@tlaguz I'm not agree with you in this case. While null not equals null in DB, it should not be equal in builder. When I want to delete null value rows in db, I tell to DB exactly where column is NULL command. Any abstraction layer over DB should not take any side effects with it. Going back to main sample, lets translate it in to stored procedure in T-SQL

create procedure drop_user (@username nvarchar(255))
as
begin
     delete Users where username = @username
end

and call it

declare @username nvarchar(255)
select top 1 @username = name from orders where orderid = -1 --this should set @username = null
exec drop_user @username

This code do not delete all users without username.

tlaguz commented 5 years ago

@for7raid Agreed - SQL have special clause for dealing with nulls. I now see that compiling to:

DELETE FROM "Table" WHERE "name" = NULL;

would be more natural because operator = is the default one.

However this may lead to confusion in this case:

db.Query("Table").Where("name", null).Select();

so this is not only delete problem. However I would like where's behavior to be consistent for all types of requests, regardless if it converts = to is or not.

for7raid commented 5 years ago

@tlaguz

However I would like where's behavior to be consistent for all types of requests, regardless if it converts = to is or not.

Fully argeed)

I'm confused, today I've found that NHibernate has the same issue The query

var query = _documentFileRepository.GetAll()
                .Where(x => x.Document.Id == param.DocumentId && x.DataId == param.DataId)
                .OrderBy(x => x.Name);

in case when param.DataId is null will be compiled to

where DocumentId = @p1 and DataId is null
tlaguz commented 5 years ago

knex.js: https://github.com/tgriesser/knex/blob/master/src/query/builder.js#L275 doctrine2 (different case, but somehow connected): https://stackoverflow.com/questions/16718841/doctrine2-symfony2-how-to-ignore-null-in-query-builder

In EF this code also translates to is null:

context.Customers.Where(c => c.Name == null) ....

If Where were to compile null blindly to column = null then similar method (or just argument) that would compile null to is null would be handy.

for7raid commented 5 years ago

Very sad to hear this news about other utils. I have to make a revision of my application to remove all code with this side effects. But in this way I think sqlkata should be in the same stream like big ORM and force treat null as is null command.

for7raid commented 5 years ago

If Where were to compile null blindly to column = null then similar method (or just argument) that would compile null to is null would be handy.

There are thees methods in sqlkata: WhereIsNull and WhereIsNotNull