Simple Machines Community Forum

SMF Support => SMF 2.0.x Support => PostgreSQL and SQLite Support => Topic started by: habakuk on February 13, 2009, 02:44:37 PM

Title: ERROR Coalesce
Post by: habakuk on February 13, 2009, 02:44:37 PM
Hi all. Using rc1, but having seen this probelm in b4 too, I get this Error message, on step 2 of the newsletter functionality:

ERROR: COALESCE types integer and boolean cannot be matched
File: blah/Sources/ManageNews.php
Line: 365

Using postgreSQL and just one mod (Order Sticky Topics), I get the error after selecting "News and Newsletters" > "Newsletters" and selecting any (or none) of the groups to receive a newsletter and posting the form ("Next" button).

In ManageNews.php > ComposeMailing(), the line:

         AND (COALESCE(bg.expire_time, 1=1) OR (bg.expire_time > {int:current_time}))',

seems to throw up.

Any hint on that issue?

TIA
Title: Re: ERROR Coalesce
Post by: DerEineDa on February 13, 2009, 07:05:04 PM
Not using PostgreSQL, but I guess you could try to change that line to:

AND (bg.expire_time IS NULL OR (bg.expire_time > {int:current_time}))',
Title: Re: ERROR Coalesce
Post by: DerEineDa on February 13, 2009, 07:37:27 PM
Thinking a bit further... what purpose has the part "AND (COALESCE(bg.expire_time, 1=1) OR" anyway? This part will always evaluate to "true"! Coalesce will return the first not-null-value of the arguments. If "bg.expire_time" is not NULL, it will be returned, and MySQL will evaluate this part to TRUE, because the value is different from zero.

But if "bg.expire_time" is NULL, then "1=1" will be returned and this is also TRUE. No matter what happens, it's always like there would be written "...AND (TRUE OR...)". Taking this even further, you can see that the left side of this OR-operator is always TRUE, which means that the whole part after the AND will always be TRUE.

This means, that this:

Code: [Select]
WHERE (bg.cannot_access = {int:cannot_access} OR bg.cannot_login = {int:cannot_login})
AND (COALESCE(bg.expire_time, 1=1) OR bg.expire_time > {int:current_time})',
is semantically exactly the same as
Code: [Select]
WHERE (bg.cannot_access = {int:cannot_access} OR bg.cannot_login = {int:cannot_login})
AND TRUE,
. The "AND TRUE" at the end can be omitted.

As far as I understand this, what the programmers wanted to have is this:

Code: [Select]
SELECT DISTINCT mem.id_member
FROM {db_prefix}ban_groups AS bg
INNER JOIN {db_prefix}ban_items AS bi ON (bg.id_ban_group = bi.id_ban_group)
INNER JOIN {db_prefix}members AS mem ON (bi.id_member = mem.id_member)
WHERE (bg.cannot_access = {int:cannot_access} OR bg.cannot_login = {int:cannot_login})
AND (bg.expire_time IS NULL OR bg.expire_time > {int:current_time})

This would get a list of all currently banned member_id. According to the comment inside this file this is exactly what this query should return.

This bug can be found two times in the file ManageNews.php.
Title: Re: ERROR Coalesce
Post by: habakuk on February 13, 2009, 08:31:02 PM
Again, thanks much for the pointer. After reading about Coalesce, I didn't understand what is being tested there, as it indeed always returns true. But still being pretty new to smf, I usually expect it's me not understanding the code ... :)

And when trying to figure what goes on there, I removed the coalesce part and all went well...

Your code change certainly makes sense to me.

Now... is this bug already filed/known?

cheers
®
Title: Re: ERROR Coalesce
Post by: DerEineDa on February 13, 2009, 08:37:19 PM
I don't know if this bug is already known. Hope someone responsible will read this :)

I am quite sure that what I wrote is correct, because I am very confident in my programming/database skills ;)
Title: Re: ERROR Coalesce
Post by: habakuk on February 14, 2009, 07:25:47 PM
Thanks for the bugreport!

cheers
®

Title: Re: ERROR Coalesce
Post by: karlbenson on February 15, 2009, 08:16:22 AM
Thanks for reposting the contents/link to this in the bug reports.

Both isses are now on the bug tracker.
Title: Re: ERROR Coalesce
Post by: markf on April 10, 2015, 11:41:12 AM
i know this is an old topic, but it's still happening in 2.09 on postgres.
Is there a link to a bug tracker where I might be able to upvote this or is it internal?
Either way, any chance it'll get fixed? I can manually do it for now, but hate having to hack stuff and having to track changes during upgrades.

EDIT: ... and in editing the Sources/ManageNews.php it's now obvious the first version of the COALESCE was fixed, but the one on line 367 has same issue.
Title: Re: ERROR Coalesce
Post by: DerEineDa on April 10, 2015, 11:52:53 AM
LOL, I just got a notification about this thread via E-Mail. Funny to be reminded about my old posts like this.
Title: Re: ERROR Coalesce
Post by: markf on April 10, 2015, 12:57:33 PM
Ya, I googled the coalesce problem, hit this thread, and saw the fix, assumed it wasn't in yet, but it is... only there's an almost identical block of the original code about 20 lines later with exactly the same mistake in it causing the same error *sigh*
Title: Re: ERROR Coalesce
Post by: margarett on April 10, 2015, 08:24:57 PM
Could you please attach your ManageNews.php, please?
Title: Re: ERROR Coalesce
Post by: markf on April 11, 2015, 05:57:28 AM
Sure, unchanged version attached as requested.
A diff of the fix is:

Code: [Select]
--- Sources/ManageNews.php.001  2015-04-10 16:45:45.655751299 +0100
+++ Sources/ManageNews.php      2015-04-10 16:49:28.026751555 +0100
@@ -364,7 +364,7 @@
                FROM {db_prefix}ban_items AS bi
                        INNER JOIN {db_prefix}ban_groups AS bg ON (bg.id_ban_group = bi.id_ban_group)
                WHERE (bg.cannot_access = {int:cannot_access} OR bg.cannot_login = {int:cannot_login})
-                       AND (COALESCE(bg.expire_time, 1=1) OR bg.expire_time > {int:current_time})
+                       AND (bg.expire_time IS NULL OR bg.expire_time > {int:current_time})
                        AND bi.email_address != {string:blank_string}',
                array(
                        'cannot_access' => 1,
Title: Re: ERROR Coalesce
Post by: margarett on April 11, 2015, 08:48:35 AM
Thank you. Your fix will be included in the next update ;)
Title: Re: ERROR Coalesce
Post by: markf on April 11, 2015, 05:58:33 PM
well, full credit to @DerEineDa for the original fix, but thanks for taking it in.
might want to check any other "1=1" logic, as they'll all break on postgresql, just haven't been hit yet.
Title: Re: ERROR Coalesce
Post by: Kindred on April 11, 2015, 06:44:09 PM
1=1 is just bad lazy coding (IMO) anyway


-edit - and yes, I am aware that SMF uses this method throughout several different files and queries.  That does not change my opinion that it should not actually be used and there are better ways to format a WHERE...
Title: Re: ERROR Coalesce
Post by: margarett on April 14, 2015, 04:57:07 AM
might want to check any other "1=1" logic, as they'll all break on postgresql, just haven't been hit yet.
Not ALL 1=1 WHERE clauses will break, or your forum will become unusable :P
Eg: to list the boards you can see being administrator, query_see_board *IS* 1=1 instead of FIND_IN_SET(yabadabadaba)
Title: Re: ERROR Coalesce
Post by: markf on April 14, 2015, 05:19:47 AM
My comment about breaking was everywhere different types are used as in the coalesce case (i.e. "integer and boolean cannot be matched").

I'm completely aware that you can do "where 1=1" to do every row selects, and in a dynamically built query, this is clearly being used.

Though personally, my style of coding would test the conditions in code before building the select and not add a where statement if not required, rather than it falling through in this way. In which case this error wouldn't have occurred and you wouldn't be at the whim of the database vendor. But that's a style thing.
Title: Re: ERROR Coalesce
Post by: margarett on April 14, 2015, 05:25:18 AM
My comment about breaking was everywhere different types are used as in the coalesce case (i.e. "integer and boolean cannot be matched").
Ups, my bad, sorry :P
Thank you again ;)