275788 - Provide a line of code that defines legal query formats for other scripts to use

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Colin Ogilvie [:cso]]

This comes from bug 274397 comment 19 - there should be a way to check whether a
query is in the required format.

I intend to look at this shortly.

Comment 1

[Colin Ogilvie [:cso]]

Nominating for Blocking 2.18 per Dave's comments in the other bug

Comment 2

[Colin Ogilvie [:cso]]

Attachment: Fix v1

Patch v1

The Patch adds a function to globals.pl (IsValidQueryType) which can have extra
types added into it, and changes userprefs.cgi and query.cgi to use this for
checking for valid query types.  I'm not convinced the Userprefs stuff is
perhaps the way to go, but it works for me, and at the moment I can't come up
with anything better :(

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 169438 [details] [diff] [review]
Fix v1

Since the point of separating this was to make it easy on customizers, we
should probably put the list in a qw() so it's easy to read/modify, and put a
comment in front of it explaining what it's for and why someone might want to
add to the list (like if they add a format for the query page and want a cookie
set when people visit it)

Also, we've been trying to avoid adding stuff to globals.pl, because globals.pl
is going to go away soon anyway.  I'm not sure which .pm file would work best
for this one though...

Comment 4

[Gervase Markham [:gerv]]

Why are we not finding the valid query types by parsing the names of templates
in the appropriate directory? Performance?

This would be self-extending, and easier for customisers.

Gerv

Comment 5

[Colin Ogilvie [:cso]]

(In reply to comment #4)
> Why are we not finding the valid query types by parsing the names of templates
> in the appropriate directory? Performance?
> 
> This would be self-extending, and easier for customisers.

As I understand it, there are only two types of query this bit needs to refer
to, yet there are 7 search-*.html.tmpl files in the search/ folder.

Comment 6

[Colin Ogilvie [:cso]]

Attachment: Bugzilla::Query

Bugzilla::Query

This contains the IsValidQueryType subroutine.

Comment 7

[Colin Ogilvie [:cso]]

Attachment: Patchv2 (Trunk)

Use the new module to check if its a valid query.

Comment 8

[Colin Ogilvie [:cso]]

Comment on attachment 169593 [details] [diff] [review]
Patchv2 (Trunk)

Note, the patch also actually applies to 2.18, with an offset.

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #4)
> Why are we not finding the valid query types by parsing the names of templates
> in the appropriate directory? Performance?

That's what we did originally, and it's the sole reason we released 2.18rc2 a
single week after 2.18rc1, because it was a fatal error.  The reports also use
query.cgi with search-* templates.  If you set a cookie when the reports page is
displayed, then you go back to the query page, you wind up with one of the
reports pages, and no way to get back because there's no tabs at the top.  See
bug 275788.

Comment 10

[Dave Miller [:justdave]]

you know, perhaps for this reason, it would be better to provide a list of
formats to exclude instead of include...

Comment 11

[Dave Miller [:justdave]]

(In reply to comment #9)
> See bug 275788.

Oops, make that bug 250881.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 169592 [details]
Bugzilla::Query

>package Bugzilla::Query;

  Why doesn't this just go inside of Bugzilla::Search? A "search" and a "query"
are the same thing, I believe... although maybe not in Bugzilla...

  Still, validation of Bugzilla::Search's input seems like something that
should be in Bugzilla::Search.

Comment 13

[Colin Ogilvie [:cso]]

(In reply to comment #12)
>   Why doesn't this just go inside of Bugzilla::Search? A "search" and a "query"
> are the same thing, I believe... although maybe not in Bugzilla...

That would be because I didn't see that... 

Comment 14

[Colin Ogilvie [:cso]]

(In reply to comment #12)
>   Still, validation of Bugzilla::Search's input seems like something that
> should be in Bugzilla::Search.

Something that occured to me just now: 

This is not validating Bugzilla::Search's input as such - neither of the files
that use this module make use of Bugzilla::Search (that I can see).

I'm quite happy to move this method in their though, if thats whats desired (and
I'll attach a patch shortly).

Comment 15

[Colin Ogilvie [:cso]]

Attachment: Branch Patch v2

As previous patches, but with the code in Bugzilla::Search instead.

Comment 16

[Colin Ogilvie [:cso]]

Attachment: Trunk Patch v3

Trunk patch with the code in Bugzilla::Search

Comment 17

[Vlad Dascalu]

Comment on attachment 169926 [details] [diff] [review]
Trunk Patch v3

+		 next if $comp !~ /query_format=(.*)/i;

Why the /i at the end? I've tested your patch on the trunk and hacked the URL
so that "Query_format" is used instead of "query_format".

It's not accounted for and it's not considered in any testing that I could
perform, so doing case insensitive searching here doesn't make sense.

Comment 18

[Vlad Dascalu]

Comment on attachment 169926 [details] [diff] [review]
Trunk Patch v3

Same here.

Comment 19

[Colin Ogilvie [:cso]]

Attachment: Trunk Patch v4

Removes the /i (why I put it there, I can't think)

Comment 20

[Colin Ogilvie [:cso]]

Attachment: Branch Patch v4

Branch Patch, with the /i fixed

Comment 21

[Colin Ogilvie [:cso]]

Attachment: Trunk Patch v5

After discussion with vladd, he suggested a way to make userprefs.cgi simpler -
so this implements it.

Comment 22

[Colin Ogilvie [:cso]]

Attachment: Branch Patch v5

As attachment 169945 [details] [diff] [review] but for the trunk

Comment 23

[Vlad Dascalu]

Comment on attachment 169945 [details] [diff] [review]
Trunk Patch v5

There is some duplication regarding $q->{'query'} .= '&query_format=advanced';
but I'm not sure how to remove it. Otherwise looks good.

Regarding Dave's suggestion about the include/exclude thing, I'm afraid neither
will work since newly created templates could be in both categories and each
one of them will have to be manually reviewed (and added, if suitable, to
IsValidQueryType).

Comment 24

[Vlad Dascalu]

Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.135; previous revision: 1.134
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.72; previous revision: 1.71
done

Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.126.2.4; previous revision: 1.126.2.3
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.58.2.3; previous revision: 1.58.2.2
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.57.2.6; previous revision: 1.57.2.5
done