Closed Bug 345194 Opened 18 years ago Closed 11 years ago

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

Categories

(Bugzilla :: Query/Bug List, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: fguiliani, Assigned: glob)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

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)
It's a dupe, but I cannot find the original bug right now.
Whiteboard: DUPEME
(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.
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
(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?
(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;
}
(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)
Summary: add a "deadline is null" request item → add a "is null" item in the boolean chart
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 on attachment 229833 [details] [diff] [review]
add a "is null" item in boolean chart

I will upload another patch
Attachment #229833 - Attachment is obsolete: true
This patch was made with "cvs diff -u" against the last cvs revision.
Attachment #229945 - Flags: review?
Attachment #229945 - Flags: review? → review?(bugreport)
Attachment #229945 - Flags: review?(justdave)
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.
(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.
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Group: webtools-security
Joel: was this marked as a security-sensitive bug by mistake? I don't see any security implications here...
Yeah...  I think I got bitten by the automatic advance to the next field issue.
Group: webtools-security
justdave, joel: could you look at this patch? It's here for a year now.
OS: Windows XP → All
Hardware: PC → All
Whiteboard: DUPEME
Assignee: query-and-buglist → fguiliani
(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.
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.
Priority: -- → P2
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%.
(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 "".
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.
Attached patch Updated patch for CVS HEAD (obsolete) — Splinter Review
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 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
(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
(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?  :)
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".
Attachment #336103 - Attachment is obsolete: true
Don't forget to request review!
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.
Assignee: fguiliani → jlc6
Summary: add a "is null" item in the boolean chart → Add "is null" and "is not null" search operators to the boolean chart
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?
Attachment #355767 - Flags: review?(dkl)
Attachment #229945 - Attachment is obsolete: true
Attachment #229945 - Flags: review?(justdave)
Attachment #229945 - Flags: review?(bugreport)
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.
Attachment #229945 - Flags: review-
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.
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 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
Attachment #355767 - Flags: review?(dkl) → review-
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)
(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.
Blocks: 506021
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 %]
Summary: Add "is null" and "is not null" search operators to the boolean chart → Add "is null" and "is not null" search operators to the boolean chart (ability to search for empty and not empty fields)
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.
Priority: P2 → P1
Target Milestone: --- → Bugzilla 4.4
See Also: → 760302
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.
FYI: The workaround in comment 42 does not work for neither 4.2.4, the 4.4 branch, or trunk.
Attached patch patch v1 (obsolete) — Splinter Review
here's the changes i made to bmo/4.2 to support "is empty" and "is not empty".
Assignee: jlc6 → glob
Attachment #355767 - Attachment is obsolete: true
Attachment #724306 - Flags: review?(LpSolit)
Summary: Add "is null" and "is not null" search operators to the boolean chart (ability to search for empty and not empty fields) → Add "is empty" and "is not empty" search operators to the boolean chart (ability to search for null and not null fields)
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.
Attachment #724306 - Flags: review?(LpSolit) → review-
Blocks: 763707
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #724306 - Attachment is obsolete: true
Attachment #726707 - Flags: review?(LpSolit)
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 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? ;)
Attached patch patch v3 (obsolete) — Splinter Review
yes, i tested it.
looks like $not was accidentally lost during last minute refactoring.
Attachment #726707 - Attachment is obsolete: true
Attachment #726707 - Flags: review?(LpSolit)
Attachment #729580 - Flags: review?(LpSolit)
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.
Attachment #729580 - Flags: review?(LpSolit) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #729580 - Attachment is obsolete: true
Attachment #744493 - Flags: review?(LpSolit)
Comment on attachment 744493 [details] [diff] [review]
patch v4

The patch doesn't apply cleanly:

Hunk #8 FAILED at 2746.
Attachment #744493 - Flags: review?(LpSolit)
Attached patch patch v5Splinter Review
Attachment #744493 - Attachment is obsolete: true
Attachment #749711 - Flags: review?(LpSolit)
Bugzilla 4.4 has been released. Enhancement bugs are retargetted to 5.0.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
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
Attachment #749711 - Flags: review?(LpSolit) → review+
Status: NEW → ASSIGNED
Flags: approval?
Keywords: relnote
Flags: approval? → approval+
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
evidently the xt tests don't skip operators which don't have tests defined (instead they crash).

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 8660.
Blocks: 901589
Blocks: 1027617
Hi Everyone,

Whether this implemented in Bugzilla 4.5.5. Can any one update on this?
(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.
(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
(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
(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/"
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
Thank You Vitaly.....
Added to relnotes for 5.0rc1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: