• Welcome to Simple Machines Community Forum. Please login or sign up.

ERROR Coalesce

Started by habakuk, February 13, 2009, 02:44:37 PM

Previous topic - Next topic

habakuk

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

DerEineDa

February 13, 2009, 07:05:04 PM #1 Last Edit: February 13, 2009, 07:34:40 PM by DerEineDa
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}))',

DerEineDa

February 13, 2009, 07:37:27 PM #2 Last Edit: February 13, 2009, 07:40:31 PM by DerEineDa
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:

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 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:

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.

habakuk

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
®

DerEineDa

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 ;)

habakuk

Thanks for the bugreport!

cheers
®


karlbenson

Thanks for reposting the contents/link to this in the bug reports.

Both isses are now on the bug tracker.

markf

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.

DerEineDa

LOL, I just got a notification about this thread via E-Mail. Funny to be reminded about my old posts like this.

markf

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*

margarett

Could you please attach your ManageNews.php, please?
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

markf

Sure, unchanged version attached as requested.
A diff of the fix is:

--- 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,


margarett

Thank you. Your fix will be included in the next update ;)
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

markf

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.

Kindred

April 11, 2015, 06:44:09 PM #14 Last Edit: April 13, 2015, 11:28:45 AM by Kindred
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...
Please do not PM, IM or Email me with support questions.  You will get better and faster responses in the support boards.  Thank you.

"Loki is not evil, although he is certainly not a force for good. Loki is... complicated."

margarett

Quote from: markf on April 11, 2015, 05:58:33 PM
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)
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

markf

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.

margarett

Quote from: 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").
Ups, my bad, sorry :P
Thank you again ;)
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

Advertisement: