601093 - Invalid bug statuses are ignored on search rather than being treated as literals

Status: NEW

(Bugzilla :: Query/Bug List, defect)

Description

[Gervase Markham [:gerv]]

4.0: https://landfill.bugzilla.org/bugzilla-4.0-branch/buglist.cgi?bug_status=Phooey&bug_id=1

=> Zarro boogs.

Tip: https://landfill.bugzilla.org/bugzilla-tip/buglist.cgi?bug_status=Phooey&bug_id=1

=> One bug found. 

Gerv

Comment 1

[Gervase Markham [:gerv]]

To summarise the change: searching for unknown statuses used to return no results (because there were no bugs with that status). Now, the unknown status is just ignored as if it were not present.

LpSolit, mkanat: do you have an opinion on the correct behaviour here? My view is that the new behaviour is undesirable. We don't have an "ignore unknown values" policy for other choices from lists, like version, OS or Platform.

Gerv

Comment 2

[Gervase Markham [:gerv]]

In fact, it seems that "ignore unknown values" has also recently become the policy for the "keywords" field. But it remains not the policy for version, OS, platform and many others. So we should decide what to do about "keywords" too.

I would like to work this out soon, so I can either fix Bugzilla or the BzAPI test suite to pass against the tip.

Gerv

Comment 3

[Frédéric Buclin]

Personally, something which doesn't exist should return no data rather than being ignored. So if there is no bug with field = foo, and that's exactly what you asked for, then nothing should be returned.

Comment 4

[Gervase Markham [:gerv]]

I agree. mkanat?

Gerv

Comment 5

[Max Kanat-Alexander]

I've sort of thought about this on and off as I've been working on Search.pm.

My personal policy has been to validate things where we have to anyway, and where it won't slow down Search.pm.

Specifically for statuses, I do believe there is a place where I could throw an error, and so we probably should. So for now consider this a bug in Bugzilla unless I say otherwise (and "otherwise" would only happen if I realize that there's some big complexity involved in validation here).

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's the patch for Status; I can't see how to fix keywords. mkanat: can you give me a hint?

Thanks,

Gerv

Comment 7

[Gervase Markham [:gerv]]

mkanat: ping? It would be good to fix this before 4.2 comes out, so then behaviour will be consistent across released versions.

Gerv

Comment 8

[Max Kanat-Alexander]

Comment on attachment 559624 [details] [diff] [review]
Patch v.1

Review of attachment 559624 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, this isn't quite right, but you've pointed me in the right direction to fix this, and I do see how we could. I'll actually take this bug myself and work on it.

Comment 9

[Max Kanat-Alexander]

For statuses, these are the situations this has to deal with:

(1) User passes an invalid status, and no other status.
(2) There are N statuses. User passes N - 1 valid statuses and one invalid status.
(3) User passes multiple statuses with the same name but different amounts of whitespace, some valid and some invalid.
(4) User passes entirely valid statuses.
(5) User passes some combination of __open__, __closed__, and __all_, combined with the above cases.

And all various combinations of those cases. There might be a case I'm missing.

Comment 10

[Max Kanat-Alexander]

So due to the complexity, I'm not going to take this right now. So feel free to keep working on it, if you have a solution that accounts for at least the above cases. You'll also want to do this with resolutions.

Keywords should be dealt with in a separate bug.

Comment 11

[Gervase Markham [:gerv]]

(In reply to Max Kanat-Alexander from comment #9)
> For statuses, these are the situations this has to deal with:
> 
> (1) User passes an invalid status, and no other status.
> (2) There are N statuses. User passes N - 1 valid statuses and one invalid
> status.
> (3) User passes multiple statuses with the same name but different amounts
> of whitespace, some valid and some invalid.
> (4) User passes entirely valid statuses.
> (5) User passes some combination of __open__, __closed__, and __all_,
> combined with the above cases.
> 
> And all various combinations of those cases. There might be a case I'm
> missing.

Is it really that complicated? Whitespace-trim the submitted statuses, and delete any which are invalid. Use the rest. If that makes the list empty, then treat the list as empty. (We already have code to catch the case where a user searches for every bug in the database.)

Is that an unreasonable algorithm?

Gerv

Comment 12

[Max Kanat-Alexander]

(In reply to Gervase Markham [:gerv] from comment #11)
> Whitespace-trim the submitted statuses, and delete any which are invalid. 
> Use the rest. If that makes the list empty,
> then treat the list as empty. (We already have code to catch the case where
> a user searches for every bug in the database.)

  That's what we're doing *now*. Your "delete any which are invalid" step is the reason there is no error being thrown.

Comment 13

[Gervase Markham [:gerv]]

Max: sorry, you are quite right. What I meant to say was:

Whitespace-trim the submitted statuses. (If the list is empty, treat it as empty - that's fine.) Expand any __foo__ statuses. Then search for the resulting list. That deals with cases (2), (3), (4) and (5).

There are plenty of ways to do a search which returns every bug in the database if you really want to - searching for ?status=Foopy is just one more. So I'm not worried about case (1).

Gerv

Comment 14

[Gervase Markham [:gerv]]

Keywords is now bug 803481.

Gerv