302599 - Saved searches using "days since bug changed" fail with "Can't use (to_days(now()) - to_days(bugs.delta_ts)) as a field name" error

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Marc Schumann [:Wurblzap]]

This is a regression of bug 280499 (based on bug 237862).

Steps to reproduce:
1. On a pre-280499 install, create a saved search which contains a boolean
   chart referring to the "Days since bug changed" field.
2. Upgrade to a post-280499 version.
3. Run the saved search.

This fails because the "Days since bug changed" field has changed from
"(to_days(now()) - to_days(bugs.delta_ts))" to
"(TO_DAYS(NOW()) - TO_DAYS(bugs.delta_ts))" in the meantime, so that Search.pm
cannot find it any more.

Comment 1

[A. Karl Kornel]

*** Bug 302651 has been marked as a duplicate of this bug. ***

Comment 2

[Frédéric Buclin]

*** Bug 313487 has been marked as a duplicate of this bug. ***

Comment 3

[Frédéric Buclin]

Attachment: possible fix, v1

Marc, does it fix the problem for you? I hacked the URL to test it and it seems to work correctly. But maybe this introduces other regressions?

Comment 4

[Frédéric Buclin]

Comment on attachment 200605 [details] [diff] [review]
possible fix, v1

joel, could you have a look at this fix as well? Maybe it introduces other regressions?

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 200605 [details] [diff] [review]
possible fix, v1

I think this is the right way to do it. I'll need to look into it in detail, though.

Comment 6

[Joel Peshkin]

Comment on attachment 200605 [details] [diff] [review]
possible fix, v1

This should be pretty safe

Comment 7

[Max Kanat-Alexander]

Comment on attachment 200605 [details] [diff] [review]
possible fix, v1

I disagree with this change -- I think that we can just manually fix the old Saved Searches in checksetup.pl.

I don't want to have to remember to "lc" the field names forever.

Comment 8

[Dave Miller [:justdave]]

WTF are we using SQL for a field name anyway?  That sucks.

We should get it over with and give it a real field name, and do the SQL conversion on the back end after decoding the URL params/saved query params.

Although we probably need to do something stupid like the lc() test for 2.20

Comment 9

[Frédéric Buclin]

I'm not going to play with field names or with Search.pm. Reassigning to default owner.

Comment 10

[Marc Schumann [:Wurblzap]]

It seems to me comment 8 makes this to be related to bug 289602. What do you think?

Comment 11

[Frédéric Buclin]

Another good reason to fix this bug quickly is that it blocks bug 301141, a PostgreSQL-specific one, when my patch v2 is applied (this doesn't mean my patch is wrong, this means we are inserting a stupid column name):

checksetup.pl: DBD::Pg::db do failed: ERREUR:  Value too long for type character varying(64)
checksetup.pl:  [for Statement "INSERT INTO fielddefs
checksetup.pl:                                (name, description, mailhead, sortkey)
checksetup.pl:                    VALUES (?, ?, ?, ?)"] at ./checksetup.pl line 1664
checksetup.pl:       main::AddFDef('(TO_CHAR(NOW()::date, \'J\')::int - TO_CHAR(bugs.delta_ts::da...', 'Days since bug changed', 0) called at ./checksetup.pl line 1715

Comment 12

[Max Kanat-Alexander]

It's true that comment 8 is the right way to fix this, but I doubt we will be doing that for 2.20 or 2.22. It actually could be a significant architectural change. For now there is a much simpler workaround to fix this, which is simply to fix the saved queries.

Comment 13

[Frédéric Buclin]

Attachment: patch, v2

it wasn't that difficult after all. ;)

Tested on both MySQL and PostgreSQL. Saved searches are correctly fixed, even when the case is different from the one expected. After running checksetup.pl, I can still run and edit saved searches correctly. The field name in the 'fielddefs' table has been renamed to 'days_elapsed'.

Comment 14

[Frédéric Buclin]

Comment on attachment 202609 [details] [diff] [review]
patch, v2

my patch affects Search.pm, so asking joel too.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 202609 [details] [diff] [review]
patch, v2

OH yeah, you're right.

My only thought is -- shouldn't the $old_field_name be surrounded by \Q \E in the regex?

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 202609 [details] [diff] [review]
patch, v2

Yes, it should, and I'm going to go ahead and deny review on this instead of saying "fix it on checkin" because that's a fairly big deal.  *never* use unescaped variables in a regexp unless you expect the contents of the variable to actually contain a regexp that you plan to use.

Comment 17

[Dave Miller [:justdave]]

ok, another stupid (or maybe not?) question...  what about saved chart series?

Comment 18

[Frédéric Buclin]

Attachment: patch, v3

justdave is right, saved chart series have to be fixed too.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 202920 [details] [diff] [review]
patch, v3

r=mkanat by inspection of interdiff.

Comment 20

[Frédéric Buclin]

tip:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.450; previous revision: 1.449
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.118; previous revision: 1.117
done


2.20:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412.2.10; previous revision: 1.412.2.9
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.99.2.8; previous revision: 1.99.2.7
done