Re: Patch for gnatsweb.pl

[Lars Henriksen]

On Mon, Sep 16, 2002 at 09:41:21AM -0400, Robert Lupton the Good wrote:

> (This is my `patch.3b')
[snip]
>       1/ requires exact matches for field names selected using
> scrolling lists.  For example, we have a "Scheduled" field that
> has entries "approved" and "notapproved"; without this patch
> selecting "approved" gets "notapproved" too.  The reason is that
> gnatsweb uses a ~ match in its query, and this seems reasonable.
> 
> This patch adds explicit "^" and "$" to the fields, and then
> adds code so that the user doesn't see them (Note: this doesn't
> work very elegantly with my "load stored query" patch that
> I sent to the list a few days ago.  This problem is fixed in
> a later patch that I'll send today).

This is a clever trick, but I believe there is more to it.

The query_page() and advanced_query_page() subroutines both use field names
as names of form parameters whose values are then taken by submitquery() to
construct a query expression. But query_page() uses dbconfig field names
(except for Synopsis) while advanced_query_page() uses lower case versions!
By lower case versions I mean those returned by field2param().

This affects the following piece of code in submitquery():

            # Most (?) people expect queries on enums to be of the
            # exact, not the substring type.
            if (fieldinfo($field, 'fieldtype') =~ "enum|multienum")
            {
              $subexp = appendexpr ($subexp, '|', "$ucfield==\"$sval\"");
            }
            else
            {
              $subexp = appendexpr ($subexp, '|', "$ucfield~\"$sval\"");
            }

Here $field is a form parameter, hence a dbconfig name if set by
query_page() and a lower case version if set by advanced_query_page();
$ucfield is the upper case version of $field. Now, when called with a lower
case field name, fieldinfo() will never return enum or multienum (because
there is no such field).

The upshot is that the if-clause is taken from query_page() (except for
Synopsis), the else-clause from advanced_query_page() whether the field
type is enumerated or not. If that was the intention, the comments and the
if-expression are misleading to put it mildly!

There are further complications. The advanced_query_page() sets two form
parameters with the same name for each field of enumerated type: one for
regular expression match and one for list selection. Hence we cannot use the
field type to distinguish between the two. The problem is sidestepped in the
patch from Robert Lupton the Good by having the form parameter values include
regular expression anchors in the list case.

In stead I believe the correction should coordinate the two query pages and
submitquery() in a better way:

1) Use dbconfig field names consistently for query form parameters, and
2) avoid upper/lower case conversion.
3) Use a recognizable form parameter name for enumerated type regexp
   matches (like the *_after and *_before date parameters).
4) Rewrite the query expression construction in submitquery().

This is also a move towards getting rid of the upper/lower case field name
mess. Patch follows.

Regards
Lars Henriksen

Index: gnatsweb.pl
===================================================================
RCS file: /cvsroot/gnatsweb/gnatsweb/gnatsweb.pl,v
retrieving revision 1.99
diff -u -r1.99 gnatsweb.pl
--- gnatsweb.pl 17 Sep 2002 16:26:56 -0000      1.99
+++ gnatsweb.pl 22 Sep 2002 18:43:12 -0000
@@ -1840,8 +1840,8 @@
                        -values=>['Originated by You'],
                        -defaults=>[]),
         "</td>\n</tr>\n",
-        "<tr>\n<td>Synopsis Search:</td>\n<td>",
-        $q->textfield(-name=>'synopsis',-size=>25),
+        "<tr>\n<td>$SYNOPSIS_FIELD Search:</td>\n<td>",
+        $q->textfield(-name=>$SYNOPSIS_FIELD,-size=>25),
         "</td>\n</tr>\n",
         "<tr>\n<td>Multi-line Text Search:</td>\n<td>",
         $q->textfield(-name=>'multitext',-size=>25),
@@ -1961,14 +1961,12 @@
       my $headerstr = $_." after";
       my $param_name = $headerstr;
       $param_name =~ s/ /_/;
-      $param_name =  field2param ($param_name);
       print "<tr>\n<td>$headerstr:</td>\n<td>",
           $q->textfield(-name=>$param_name, -size=>$width),
           "</td>\n</tr>\n";
       $headerstr = $_." before";
       $param_name = $headerstr;
       $param_name =~ s/ /_/;
-      $param_name = field2param ($param_name);
       print "<tr>\n<td>$headerstr:</td>\n<td>",
           $q->textfield(-name=>$param_name, -size=>$width),
           "</td>\n</tr>\n";
@@ -1991,16 +1989,20 @@
         "</tr>\n";
   foreach (@fieldnames)
   {
-    my $lc_fieldname = field2param($_);
-
     print "<tr valign=top>\n";
 
     # 1st column is field name
     print "<td>$_:</td>\n";
 
     # 2nd column is regexp search field
+    my $param_name = $_;
+    # Usually we want an exact match for enumerated types,
+    # so recognize the regexp query explicitly.
+    if (fieldinfo($_, 'fieldtype') =~ /enum/) {
+      $param_name = $_ . "_enum";
+    }
     print "<td>",
-          $q->textfield(-name=>$lc_fieldname,
+          $q->textfield(-name=>$param_name,
                         -size=>$width);
     print "\n";
     # XXX ??? !!! FIXME
@@ -2017,12 +2019,12 @@
 
     # 3rd column is blank or scrolling multi-select list
     print "<td>";
-    if (fieldinfo($_, 'fieldtype') =~ 'enum')
+    if (fieldinfo($_, 'fieldtype') =~ /enum/)
     {
       my $ary_ref = fieldinfo($_, 'values');
       my $size = scalar(@$ary_ref);
       $size = 4 if $size > 4;
-      print $q->scrolling_list(-name=>$lc_fieldname,
+      print $q->scrolling_list(-name=>$_,
                                -values=>$ary_ref,
                                -multiple=>1,
                                -size=>$size);
@@ -2245,63 +2247,53 @@
     $expr = appendexpr ('(! builtinfield:State[type]="closed")', '&', $expr);
   }
 
-  ### Construct expression for each param which specifies a query.
-  my $field;
-  foreach $field ($q->param())
-  {
-    my @val = $q->param($field);
+  ### Construct expression for each param which specifies a query, i.e.
+  # the param name is a (dbconfig) field name, possibly followed by
+  # "_after", "_before" or "_enum", see advanced_query_page().
+  my $name;
+  foreach $name ($q->param()) {
+    my @val = $q->param($name);
     my $stringval = join(" ", @val);
 
-    # Bleah. XXX ??? !!!
-    if ($stringval ne '')
-    {
-      my $ucfield = param2field ($field);
-      if (isvalidfield ($ucfield))
-      {
-        my $subexp = "";
-        my $sval;
-
-        # Turn multiple param values into ORs.
-        foreach $sval (@val)
-        {
-          if ($sval ne 'any' && $sval ne '')
-          {
-            # Most (?) people expect queries on enums to be of the
-            # exact, not the substring type.
-            if (fieldinfo($field, 'fieldtype') =~ "enum|multienum")
-            {
-              $subexp = appendexpr ($subexp, '|', "$ucfield==\"$sval\"");
-            }
-            else
-            {
-              $subexp = appendexpr ($subexp, '|', "$ucfield~\"$sval\"");
-            }
-          }
-        }
-        $expr = appendexpr ($expr, '&', $subexp);
-      }
-      elsif ($field eq 'text' || $field eq 'multitext')
-      {
-        $expr = appendexpr ($expr, '&', "fieldtype:$field~\"$stringval\"");
-      }
-      elsif ($field =~ /_after$/ || $field =~ /_before$/)
-      {
-        my $op;
-        # Waaah, nasty. XXX ??? !!!
-        if ($field =~ /_after$/)
-        {
-          $op = '>';
-        }
-        else
-        {
-          $op = '<';
+    if ($stringval eq '') { next };
+
+    # First the params we recognize as query params ...
+    # ... text ...
+    if ($name eq 'text' || $name eq 'multitext') {
+      $expr = appendexpr ($expr, '&', 'fieldtype:'.$name.'~"'.$stringval.'"');
+      next;
+    }
+    # ... dates ...
+    if ($name =~ /_after$/ || $name =~ /_before$/) {
+      my $op = ($name =~ /_after$/) ? '>' : '<';
+      # Remove the trailer.
+      $name =~ s/_[^_]*$//;
+      $expr = appendexpr ($expr, '&', $name.$op.'"'.$stringval.'"');
+      next;
+    }
+    # ... regexp query for enumerated types ...
+    if ($name =~ /_enum$/) {
+      $name =~ s/_[^_]*$//;
+      $expr = appendexpr ($expr, '&', $name.'~"'.$stringval.'"');
+      next;
+    }
+    # ... then those that are NOT query params ...
+    if (! isvalidfield($name)) { next };
+    # ... and finally query params with field names.
+    # For enumerated type queries we want an exact string match ...
+    if (fieldinfo($name, 'fieldtype') =~ /enum/) {
+      my $subexp = "";
+      # Turn multiple param values into ORs.
+      foreach (@val) {
+        if ($_ ne 'any' && $_ ne '') {
+            $subexp = appendexpr ($subexp, '|', $name.'=="'.$_.'"');
         }
-        # Whack off the trailing _after or _before.
-        $field =~ s/_[^_]*$//;
-        $field = param2field ($field);
-        $expr = appendexpr ($expr, '&', $field.$op.'"'.$stringval.'"');
       }
+      $expr = appendexpr ($expr, '&', $subexp);
+      next;
     }
+    # ... and for the rest a regexp match.
+    $expr = appendexpr ($expr, '&', $name.'~"'.$stringval.'"');
   }
 
   my $format="\"%s";