Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqlBuilder OR clause #573

Closed
rafakwolf opened this issue Jul 26, 2016 · 5 comments
Closed

SqlBuilder OR clause #573

rafakwolf opened this issue Jul 26, 2016 · 5 comments

Comments

@rafakwolf
Copy link

The SqlBuilder seems to be adding "OR" clause the wrong way...

are adding "AND" instead of "OR".

builder.Where("MyField = @Param",paramObj);
builder.OrWhere("MyField2 = @Param2",paramObj2); 

results this statement:
WHERE MyField = @Nome AND MyField2 = @Param2

Thanks

@ghost
Copy link

ghost commented Jul 30, 2016

This does seem to be an issue prevalent within the code base. See line 146 of SqlBuilder.cs:

AddClause("where", sql, parameters, " AND ", "WHERE ", "\n", true);

NickCraver added a commit that referenced this issue Sep 6, 2016
Fix #573 - OrWhere yielding AndWhere statement
@smax48
Copy link

smax48 commented Sep 14, 2016

I would disagree that it was really a bug - with the previous code, if you call both Where and OrWhere multiple times, the final result was like this:
(WhereCond1 AND WhereCond2 AND ...) AND (OrWhereCond1 OR OrWhereCond2 OR OrWhereCond3 OR ...)
From my perspective, that makes sense.
With this new update we have a breaking change.

@ghost
Copy link

ghost commented Oct 24, 2016

I'm inclined to disagree, the function is literally called 'OrWhere', not 'AndWhere'. Pretty sure you can still use the regular .Where function to achieve what you wanted.

@smax48
Copy link

smax48 commented Oct 26, 2016

It doesn't really matter if I can achieve the same effect using the regular .Where().
The problem here is that so far we've had consistent behaviour (maybe a bit strange, but it worked).
The proposed change is a BREAKING change, so literally it will break ALL existing code that relies on the current behaviour. Dapper is a very popular library and we cannot just break things.
If you need a different behaviour, it is a lot more better to add a new function (or a parameter to the existing one), than to change what we have atm.

@CrescentFresh
Copy link

I guess no one bothered to check the original test case. It is still not "fixed":

var sql = new SqlBuilder();
var tpl = sql.AddTemplate("select * from foo /**where**/");

sql.Where("MyField = @Param");
sql.OrWhere("MyField2 = @Param2");

@rafakwolf expected:

select * from foo WHERE  ( MyField = @Param OR MyField2 = @Param2 ) 

However both the previous and fixed SqlBuilder produces the following:

select * from foo WHERE MyField = @Param AND  ( MyField2 = @Param2 ) 

This is because only the first call to one of Where() or OrWhere() is the one that defines the joiner variable. That is, the variable that dictates what string to place between clauses. If Where() is called first the joiner will be "AND" but if OrWhere() is called first the joiner will be "OR".

Consider this: with the "fix" we get the following behavior (imagine these are in separate functions that don't know about each other):

sql.Where("a = @a");

sql.OrWhere("b = @b1");
sql.OrWhere("b = @b2");

result:

select * from foo WHERE a = @a AND (b = @b1 OR b = @b2)

But switch the order of the calls:

sql.OrWhere("b = @b1");
sql.OrWhere("b = @b2");

sql.Where("a = @a");

result:

select * from foo WHERE a = @a OR (b = @b1 OR b = @b2)

And that is what I would call a bug.

As a side comment, with the previous code it was at least consistent in that it always produced the first result irrespective of order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants