345194 - Add "is empty" and "is not empty" search operators to the boolean chart (ability to search for null and not null fields)

Status: RESOLVED FIXED

Description

[Florent Guiliani]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

It's not possible to search for bugs who have a null deadline. This feature would be nice (for me but perhaps for others people)


Reproducible: Always

Steps to Reproduce:
Something like "deadline" - "is null" - "" in the boolean chart would be great.
Actual Results:  
The is no possibility to search a field with a null value

Expected Results:  
The SQL query resulting should be able to have a "where deadline is null" statment.

Or if you don't want to include this feature in the main branch, I would apreciate if someone can provide a patch for this ;).

I've looked into the source. I will try to produce a patch for it (the code in Search.pm is just a little complicated)

Comment 1

[Frédéric Buclin]

It's a dupe, but I cannot find the original bug right now.

Comment 2

[Florent Guiliani]

(In reply to comment #1)
> It's a dupe, but I cannot find the original bug right now.
> 

I've checked up all bugs who containes deadline keyword without succes.

Comment 3

[Florent Guiliani]

Created attachment 229833 [details] [diff] [review]
add a "is null" item in boolean chart

This patch begin the job. It adds a "is null" item in the boolean chart. You must provide a value (who is not used) to bypass the test:
if ($f eq "noop" || $t eq "noop" || $v eq "") {
   next;
}
in Search.pm

Comment 4

[Frédéric Buclin]

(In reply to comment #2)
> I've checked up all bugs who containes deadline keyword without succes.

No, the bug I have in mind is more generic and involves any field, not only the deadline one.

joel, do you remember having seen this request already?

Comment 5

[Florent Guiliani]

(In reply to comment #3)
> Created an attachment (id=229833) [edit]
> add a "is null" item in boolean chart
> 
> This patch begin the job. It adds a "is null" item in the boolean chart. You
> must provide a value (who is not used) to bypass the test:
> if ($f eq "noop" || $t eq "noop" || $v eq "") {
>    next;
> }
> in Search.pm
> 

Or we can change this test like that (near line 1240 in Search.pm):
if ($f eq "noop" || $t eq "noop" || ($v eq "" && $t ne "isnull") ) {
    next;
}

Comment 6

[Florent Guiliani]

(In reply to comment #4)
> No, the bug I have in mind is more generic and involves any field, not only the
> deadline one.

My submited patch is generic (and the proposed code in comment#5 also)

Comment 7

[Frédéric Buclin]

To put your patch in our radar, set the flag to "review?" (you can see this flag when editing your attachment). The list of official reviewers is available here:
http://www.bugzilla.org/docs/reviewer-list.html

Comment 8

[Florent Guiliani]

Comment on attachment 229833 [details] [diff] [review]
add a "is null" item in boolean chart

I will upload another patch

Comment 9

[Florent Guiliani]

Created attachment 229945 [details] [diff] [review]
This patch add a "is null" item in the boolean chart

This patch was made with "cvs diff -u" against the last cvs revision.

Comment 10

[timeless]

Personally I'm not fond of "is null" as a description. I think I'd rather "is not set". Although that would have slightly different connotations.

The reason i don't like "is null" is that it's a description field which means real live humans who aren't programmers might be using it.

Comment 11

[Florent Guiliani]

(In reply to comment #10)
> Personally I'm not fond of "is null" as a description. I think I'd rather "is
> not set". Although that would have slightly different connotations.
> 

I'm ok with that.

Comment 12

[Frédéric Buclin]

(In reply to comment #10)
> Personally I'm not fond of "is null" as a description. I think I'd rather "is
> not set". Although that would have slightly different connotations.

"is empty" sounds better.

Comment 13

[Joel Peshkin]

This does assume that the user knows which fields are nullable.  Perhaps this should be donw by just making null equivalent to empty and then use the existing negation feature to permit a query to match a null value.

Comment 14

[Zach Lipton [:zach]]

Joel: was this marked as a security-sensitive bug by mistake? I don't see any security implications here...

Comment 15

[Joel Peshkin]

Yeah...  I think I got bitten by the automatic advance to the next field issue.

Comment 18

[Frédéric Buclin]

justdave, joel: could you look at this patch? It's here for a year now.

Comment 20

[Thomas Albright]

(In reply to comment #1)
> It's a dupe, but I cannot find the original bug right now.
> 

You might be talking about bug 204903, it is very similar.  It supposedly gave the ability to search for null values in fields, but apparently it did not work for the Deadlines field.  I'm not sure if there are other fields that do not work either.

Comment 21

[Frédéric Buclin]

jjclark, could you help? I'm with joel about his comment 13. We don't need any change in the UI, but a change in the backend to make no distinction between "" and NULL.

Comment 22

[Jesse Clark]

Treating "" the same as NULL would require changes to several search functions, and it's not immediately obvious what the correct behavior would be for fields that join a separate table on the search condition and then check whether the joined column is null.

A more feasible solution would be to let the user search for %null% the same way they can search for %reporter%.

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> A more feasible solution would be to let the user search for %null% the same
> way they can search for %reporter%.

The problem is that we cannot ask users to know the DB schema and do the right choice between NULL and "".

Comment 24

[Jesse Clark]

Yes, I think we also need a more unified search term parser, so that searching for empty quotes would mean '' or null, and in general terms could be quoted consistently between different search types.

Comment 25

[John L. Clark]

Created attachment 336103 [details] [diff] [review]
Updated patch for CVS HEAD

This patch is an updated version of the previous patch that was created against the latest CVS HEAD.  It also adds an "is not null" option.

On the question of naming, I like "is provided" and "is not provided" (instead of "is not null" and "is null", respectively).

Comment 26

[David Lawrence [:dkl]]

Comment on attachment 336103 [details] [diff] [review]
Updated patch for CVS HEAD

We are currently using a very similar patch here at Red Hat. Although it only seems to
work for certain bug attributes such as bug group but not others that have mapping
tables. I will have to further confirm what is or is not working with out patch.

>+sub _isnull {
>+    my $self = shift;
>+    my %func_args = @_;
>+    my ($ff, $q, $term) = @func_args{qw(ff q term)};
>+    
>+    $$term = "$$ff is null";
>+}
>+
>+sub _isnotnull {
>+    my $self = shift;
>+    my %func_args = @_;
>+    my ($ff, $q, $term) = @func_args{qw(ff q term)};
>+    
>+    $$term = "$$ff is not null";
>+}
>+

Nit: Why are you importing $q if you do not use it?

Dave

Comment 27

[David Lawrence [:dkl]]

(In reply to comment #25)
> On the question of naming, I like "is provided" and "is not provided" (instead
> of "is not null" and "is null", respectively).

What about "is empty" or "is not empty"?

Dave

Comment 28

[John L. Clark]

(In reply to comment #26)
> Nit: Why are you importing $q if you do not use it?

Heh.  Because I copy-and-pasted another function to derive these two?  :)  Good catch; I could easily provide an updated patch if anyone wants one.

(In reply to comment #27)
> What about "is empty" or "is not empty"?

Perhaps, but I tend to think that "empty" could mean either "" (a value equal to the empty string) or NULL (a value not provided).  And, in fact, that could be a useful test.  But I think we're rehashing earlier portions of this dicussion (e.g. comment #10, although I didn't understand what timeless meant when she asserted that it had a slightly different connotation).

So, to compile the list of options, we've got "is null", "is empty", "is not set", and "is not provided".  Where's the Bugzilla polling widget?  Or, even better, who can do some HCI testing?  :)

Comment 29

[John L. Clark]

Created attachment 355767 [details] [diff] [review]
Updated patch for CVS HEAD as of 2009-01-07

I updated my previous patch to apply cleanly to current CVS HEAD.  I also addressed David's nit, and changed the labeling to use "is not provided"/"is provided".

Comment 30

[Frédéric Buclin]

Don't forget to request review!

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 355767 [details] [diff] [review]
Updated patch for CVS HEAD as of 2009-01-07

Looks good to me. Except for the rewording to "is provided", etc. we have been using this same implementation on our production server for some time. We have noticed that it works for certain fields but not all.

Comment 32

[Teemu Mannermaa (:wicked)]

Comment on attachment 355767 [details] [diff] [review]
Updated patch for CVS HEAD as of 2009-01-07

John, you forgot to request a review for this patch. For future reference you should read https://wiki.mozilla.org/Bugzilla:Developers, especially step 9.

David, looking at your previous comment it seems you might be willing to make a formal review of this patch? Yes?

Comment 33

[Teemu Mannermaa (:wicked)]

Comment on attachment 229945 [details] [diff] [review]
This patch add a "is null" item in the boolean chart

Bitrotten and has had some comments against the wording presented to the user.

Comment 34

[Dan Wierenga]

Is there any particular guideline/reason why there can't be a fairly verbose description for something that's this complex? Something like:

Has some value (is not null)
Has no value (is null)

which makes it meaningful (IMHO) to laypersons and technical staff alike.

Comment 35

[Michael Thomas (Mockodin)]

Hmm one thought however, if a field has had a value and it has been removed, are we setting it back to null or as a empty string? If the later it will need to be taken into account. I see its been mentioned above but disappeared from the conversation. Seems we got into a discussion over description rather than issue.

Modified Snip from attachment 355767 [details] [diff] [review]
1914 sub _isnull { 
  1915     my $self = shift; 
  1916     my %func_args = @_; 
  1917     my ($ff, $term) = @func_args{qw(ff term)}; 
  1918  
  1919     $$term = "( $$ff is null OR $$ff = ''"; 
  1920 } 
  1921  
  1922 sub _isnotnull { 
  1923     my $self = shift; 
  1924     my %func_args = @_; 
  1925     my ($ff, $term) = @func_args{qw(ff term)}; 
  1926  
  1927     $$term = "( $$ff is not null OR $$ff <> ''"; 
  1928 } 
  1929  

It *might* be appropriate to include trim statements for nullifing white space, which would cause validation as not null. Which for general purposes and intents is empty, that's a very ify *might* however however as sometimes is puposeful.

Comment 36

[David Lawrence [:dkl]]

Comment on attachment 355767 [details] [diff] [review]
Updated patch for CVS HEAD as of 2009-01-07

>Index: Bugzilla/Search.pm
>===================================================================
>+sub _isnull {
>+    my $self = shift;
>+    my %func_args = @_;
>+    my ($ff, $term) = @func_args{qw(ff term)};
>+
>+    $$term = "$$ff is null";
>+}
>+
>+sub _isnotnull {
>+    my $self = shift;
>+    my %func_args = @_;
>+    my ($ff, $term) = @func_args{qw(ff term)};
>+
>+    $$term = "$$ff is not null";
>+}
>+

Sorry for the delay in reviewing. This is something we have done in our Bugzilla for a while now so I know that it works
for some cases. It doesn't however work for columns that are set to empty strings and not NULLs which you have stated. So you need to also check for the presence or lack of empty strings in addition to IS NULL and IS NOT NULL.

Also this does not work for all fields as well. This only works accurately for columns that are directly located in the 'bugs' table such as status_whiteboard, and other fields which can be blank including custom fields. It doesn't work in all cases for bug values that are located in mapping tables such as bug groups, cc, etc. For example "Group" "is not provided" will still list bugs that have groups checked. A different solution will need to be taken into account when the field is not a direct column in 'bugs'.

But I am not opposed to this going in with the caveat mentioned above as it does provide some benefit but is not the perfect solution.

Dave

Comment 38

[Peter Sone Koldkjær]

This issue/enhancement was born almost 3 years ago... what is the forecast for getting it in a release?
- it would help me/us in our internal processes (see bug #497326)

Comment 40

[Alexey Korsun]

(In reply to comment #38)
> This issue/enhancement was born almost 3 years ago... what is the forecast for
> getting it in a release?
> - it would help me/us in our internal processes (see bug #497326)

It would help in my process to. The same issue - Bug #505722. Voted for that bug.

Comment 41

[Pedro Zorzenon Neto]

Comment on attachment 355767 [details] [diff] [review]
Updated patch for CVS HEAD as of 2009-01-07

comment about Bugzilla-isnull_2009-01-07.diff that was reproved.

In the final patch, you also need to remember add the fields to list template, to show the search header correctly at the browser.

--- template/en/default/list/list.html.tmpl.orig        2010-08-31 11:56:39.000000000 -0300
+++ template/en/default/list/list.html.tmpl     2010-08-31 11:56:39.000000000 -0300
@@ -82,7 +82,7 @@
 [% SET shown_types = [
   'notequal', 'regexp', 'notregexp', 'lessthan', 'lessthaneq',
   'greaterthan', 'greaterthaneq', 'changedbefore', 'changedafter',
-  'changedfrom', 'changedto', 'changedby',
+  'changedfrom', 'changedto', 'changedby', 'isnotnull', 'isnull',
 ] %]
 <ul class="search_description">
 [% FOREACH desc_item = search_description %]

Comment 42

[Matt N]

Using the 'matches regular expression' or 'does not match regular expression' option under the Custom Search (or Advanced Boolean Charts) with a value of ^$ allows me to find bugs with null or not null fields. It may not work for all fields, but it works for the ones I tried and care about. For example:

1) Assigned or reopened bugs with not null URLs:

https://bugzilla.mozilla.org/buglist.cgi?bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=bug_file_loc&list_id=523276&product=Bugzilla&query_format=advanced&type0-0-0=notregexp&value0-0-0=^%24&query_based_on=&columnlist=bug_severity%2Cpriority%2Cop_sys%2Cassigned_to%2Cbug_status%2Cresolution%2Cshort_desc%2Cbug_file_loc

2) Assigned or reopened bugs with null URLs:

https://bugzilla.mozilla.org/buglist.cgi?list_id=523278&field0-0-0=bug_file_loc&columnlist=bug_severity%2Cpriority%2Cop_sys%2Cassigned_to%2Cbug_status%2Cresolution%2Cshort_desc%2Cbug_file_loc&query_format=advanced&bug_status=ASSIGNED&bug_status=REOPENED&type0-0-0=regexp&value0-0-0=^%24&product=Bugzilla

Comment 43

[Frédéric Buclin]

Now that Search.pm in 4.2 treats NULL the same way as 0 or undef, this makes things even worse. We now really need a way to include/exclude undefined values.

Comment 44

[Torsten Martinsen]

This regression also breaks other types of search: I have a saved search "Merge to HEAD" in Bugzilla 3.2.3 with the Boolean chart

Depends on   is not equal to    ""

This works fine, but in 4.2.4 (which I would like to upgrade to) the search no longer gives meaningful results.

Comment 45

[Torsten Martinsen]

FYI: The workaround in comment 42 does not work for neither 4.2.4, the 4.4 branch, or trunk.

Comment 46

[Byron Jones ‹:glob›]

Created attachment 724306 [details] [diff] [review]
patch v1

here's the changes i made to bmo/4.2 to support "is empty" and "is not empty".

Comment 47

[Frédéric Buclin]

Comment on attachment 724306 [details] [diff] [review]
patch v1

It looks mostly good, but there are two major problems:


>=== modified file 'Bugzilla/Search.pm'

>+sub _isempty {
>+    my ($self, $args, $join) = @_;

You don't use $join, so you can remove it from the list.


>+    $args->{term} = "$full_field IS NULL OR $full_field = " . $self->_empty_value($args->{field});

If the field you are querying against is outside the bugs table, e.g. CC, groups, flags, then isempty doesn't work at all because when _build_subselect() is called, it runs the subselect separately and it cannot find anything, and so returns 1=2. So the main query will return 0 bugs. If I replace:

"groups is empty" by "NOT groups is not empty" (to get public bugs), then it's working fine. But that's not something the average user will think about.


>+sub _isnotempty {
>+    my ($self, $args, $join) = @_;

Same as above, $join is unused.


>+sub _empty_value {
>+    my ($self, $field) = @_;
>+    return "''" unless $field =~ /^cf_/;

This line must go away. If you want bugs with e.g. no deadline, then you need to return EMPTY_DATETIME, not '', else all bugs are returned.

Comment 48

[Byron Jones ‹:glob›]

Created attachment 726707 [details] [diff] [review]
patch v2

Comment 49

[Jon Tarry]

I tried the workaround in comment 42, but that didn't work on our install of bugzilla (v4.0.1).  However, using 'does not match regular expression' and then just a . does work for the field I needed.

Comment 50

[Frédéric Buclin]

Comment on attachment 726707 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla/Search.pm'

>+    return $self->_multiselect_isempty($args)

>+sub _multiselect_isempty {
>+    my ($self, $args, $not) = @_;

$not is never defined as you only pass $args. Did you test your patch? ;)

Comment 51

[Byron Jones ‹:glob›]

Created attachment 729580 [details] [diff] [review]
patch v3

yes, i tested it.
looks like $not was accidentally lost during last minute refactoring.

Comment 52

[Frédéric Buclin]

Comment on attachment 729580 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla/Search.pm'

> sub _multiselect_term {

>+    return $self->_multiselect_isempty($args, $not)
>+        if $operator eq 'isempty' || $operator eq 'isnotempty';

Nit: you could combine both tests: if $operator =~ /^is(not)?empty$/.


>+sub _multiselect_isempty {

>+    $not = $operator eq 'isnotempty';

If you write: $not = ($operator eq 'isnotempty') ? 'NOT' : '', then you could write:

>+        return $not
>+            ? "keywords_$chart_id.bug_id IS NOT NULL"
>+            : "keywords_$chart_id.bug_id IS NULL";

as: return "keywords_$chart_id.bug_id IS $not NULL". One single line instead of 3 (two of which are mostly the same and prone to copy-paste errors). You could do the same for most return statements in this method. This would be a nice cleanup.


>+    elsif ($field eq 'longdesc') {

>+            extra => [ "longdescs_$chart_id.type = " . CMT_NORMAL ],

I don't understand why you set this restriction. This means that comments written when attaching or updating a file or when marking a bug as duplicate are excluded from the query. I would rather *exclude* CMT_HAS_DUPE, which is the single comment type which takes no comment, so that if we add more comment types in the future, we don't need to update this list all the time. Also, why don't you specify that the comment must not be private if the user is not in the insidergroup? That's the first part of my r-.


>+    elsif ($field eq 'longdescs.isprivate') {
>+        ThrowUserError('auth_failure', { action => 'search',
>+                                         object => 'bug_fields',
>+                                         field => 'longdescs.isprivate' })
>+            if !$self->_user->is_insider;

Nit: I would always throw an error here. I don't understand the meaning of "comment is private is empty". This doesn't mean anything. This also applies to all other boolean fields.


>+        push @$joins, {
>+            table => 'longdescs',
>+            as    => "longdescs_$chart_id",
>+            from  => 'bug_id',
>+            to    => 'bug_id',
>+            extra => [ "longdescs_$chart_id.type = " . CMT_NORMAL ],

Same comment here about CMT_NORMAL.



There is another problem with your patch when playing with the CC list. You must also fix the following line in _user_nonchanged() to understand is(not)empty correctly:

  my $is_negative = $operator =~ /^no/ ? 1 : 0;

It must be replaced by:

  my $is_negative = $operator =~ /^(?:no|isempty)/ ? 1 : 0;

With this fix, queries involving the CC list work fine. Else no bugs are returned. That's the 2nd part of my r-.


Otherwise looks good. We are pretty close now.

Comment 53

[Byron Jones ‹:glob›]

Created attachment 744493 [details] [diff] [review]
patch v4

Comment 54

[Frédéric Buclin]

Comment on attachment 744493 [details] [diff] [review]
patch v4

The patch doesn't apply cleanly:

Hunk #8 FAILED at 2746.

Comment 55

[Byron Jones ‹:glob›]

Created attachment 749711 [details] [diff] [review]
patch v5

Comment 56

[Frédéric Buclin]

Bugzilla 4.4 has been released. Enhancement bugs are retargetted to 5.0.

Comment 57

[Frédéric Buclin]

Comment on attachment 749711 [details] [diff] [review]
patch v5

>+    elsif ($field eq 'longdesc') {
>+        my @extra = ( "longdescs_$chart_id.type != " . CMT_DUPE_OF );

s/CMT_DUPE_OF/CMT_HAS_DUPE/


Otherwise looks good and works fine. r=LpSolit

Comment 58

[Byron Jones ‹:glob›]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Search.pm
modified template/en/default/global/field-descs.none.tmpl
modified template/en/default/list/list.html.tmpl
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 8659.

Comment 59

[Byron Jones ‹:glob›]

evidently the xt tests don't skip operators which don't have tests defined (instead they crash).

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 8660.

Comment 60

[Antoine]

Hi Everyone,

Whether this implemented in Bugzilla 4.5.5. Can any one update on this?

Comment 61

[Simon Green]

(In reply to Antoine from comment #60)
> Hi Everyone,
> 
> Whether this implemented in Bugzilla 4.5.5. Can any one update on this?

Yes.  This feature is implemented in the 4.5 branch currently.

Comment 62

[Antoine]

(In reply to Simon Green from comment #61)
> (In reply to Antoine from comment #60)
> > Hi Everyone,
> > 
> > Whether this implemented in Bugzilla 4.5.5. Can any one update on this?
> 
> Yes.  This feature is implemented in the 4.5 branch currently.

Hi Simon,

I have checked in the following files and not able to see is isnull, isnotnull in Search.pm. Also I have checked in below files

1. template/en/default/search/boolean-charts.html.tmpl
2. template/en/default/search/field-descs.none.tmpl

Could you please update the path? 

Bugzilla version I have used is bugzilla - 4.4.5

Comment 63

[Vitaly Fedrushkov]

(In reply to Antoine from comment #62)
> (In reply to Simon Green from comment #61)
> > Yes.  This feature is implemented in the 4.5 branch currently.
 
> Bugzilla version I have used is bugzilla - 4.4.5

It's in development branch 4.5, not in stable release 4.4.5

Comment 64

[Antoine]

(In reply to Vitaly Fedrushkov from comment #63)
> (In reply to Antoine from comment #62)
> > (In reply to Simon Green from comment #61)
> > > Yes.  This feature is implemented in the 4.5 branch currently.
>  
> > Bugzilla version I have used is bugzilla - 4.4.5
> 
> It's in development branch 4.5, not in stable release 4.4.5

Hi Vitaly,

Can I have path of development branch? 

Currently I am referring in this path "http://ftp.mozilla.org/pub/mozilla.org/webtools/"

Comment 65

[Vitaly Fedrushkov]

Please use http://www.bugzilla.org/download/#devel to find latest download available.  

Your URL is correct too, it contains http://ftp.mozilla.org/pub/mozilla.org/webtools/bugzilla-4.5.6.tar.gz

Comment 66

[Antoine]

Thank You Vitaly.....

Comment 67

[Frédéric Buclin]

Added to relnotes for 5.0rc1.