1012506 - Allow a bug to have multiple aliases

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Simon Green]

This is a feature that has been in Red Hat Bugzilla for more than five years. I would like to upstream it if you want it. The major regression is that RPC calls (e.g. Bug.get) change so that they return an array rather than a string for the array value.

If the needinfo is accepted, then I'll submit a patch. If not, feel free to close the bug.

From the original brc bug:
The Security Response Team would like the ability to have multiple 'aliases' for a single bug report. Currently they use the alias field to record the CVE identifier of the security issue and have too create several bugs that each have a different CVE identifier as the alias. If a bug could have multiple aliases then they could create one bug report with all of the relevant CVE identifiers attached to it. The multiple aliases would still need to be unique and each one cannot be attached to more than one bug report for the alias process to still work.

Comment 1

[Simon Green]

At this months Bugzilla meeting ( https://wiki.mozilla.org/Bugzilla:Meetings:2014-06-25 ), we decided to implement this feature

Comment 2

[Simon Green]

Attachment: alias-v1.patch

Note: After checksetup is run with this patch, the database cannot be used for non patched code without some hackery, since this drops the bugs.alias field.

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8450769 [details] [diff] [review]
alias-v1.patch

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

Searching with "Alias" "contains all of the strings" "foo1 foo2" when I have a bug with both foo1 and foo2 set as aliases, does not show the bug. 

Will do further functional testing when the below are addressed.

::: Bugzilla/Bug.pm
@@ +2882,5 @@
> +sub add_alias {
> +    my ($self, $alias) = @_;
> +    return if !$alias;
> +
> +    $alias = trim($alias);

Do this instead:

sub add_alias {
    my ($self, $alias) = @_;
    return if !$alias;
    my $aliases = $self->_check_alias($alias);
    $alias = $aliases->[0];
    my $bug_aliases = $self->alias;
    push(@$bug_aliases, $alias) if !grep($_ eq $alias, @$bug_aliases);
}

::: Bugzilla/DB/Schema.pm
@@ +363,5 @@
> +                                         COLUMN => 'bug_id',
> +                                         DELETE => 'CASCADE'}},
> +        ],
> +        INDEXES => [
> +            bugs_bug_id_indx        => ['bug_id'],

s/bugs_bug_id_indx/bugs_bug_id_idx/

::: Bugzilla/WebService/Bug.pm
@@ +2169,5 @@
>  
>  =item C<alias>
>  
> +C<array> of C<string>s The unique aliases of this bug. An empty array will be
> +returned  if this bug has no aliases.

single space after returned

@@ +2799,5 @@
>  
>  =item C<alias>
>  
> +C<array> of C<string>s The unique aliases of this bug. An empty array will be
> +returned  if this bug has no aliases.

single space after returned

@@ +3759,5 @@
>  =item C<alias>
>  
> +(array) - A brief alias for the bug that can be used instead of a bug number
> +when accessing this bug. Must be unique in all of this Bugzilla. Specifying
> +an aliases overwrites any aliases this bug already has.

We be able to still take 'alias' as single string value and optionally take 'alias' as a hash (similar to dependencies) that has add/remove/set keys. Otherwise this will break compatibility. We can look at dropping the string value in v2 of the API.

So in update() we need to make the following changes and update the documentation to describe it:

1) To maintain backwards compatibility, 'alias' can be a single string need to convert to
{ alias => { set => ['<new_alias>'] } } for set_all
2) If 'alias' is a reference to a hash then pass in to set_all unchanged. The documentation should explain
how to use add/remove/set.

::: process_bug.cgi
@@ +301,5 @@
>      # for one bug at a time.
> +    if (defined $cgi->param('newalias') || defined $cgi->param('removealias')) {
> +        my @alias_add = $cgi->param('newalias');
> +
> +        # We came from bug_form which uses a select box to determine what cc's

s/cc's/aliases/

::: template/en/default/bug/edit.html.tmpl
@@ +191,5 @@
>        [% END %]
>       </span>
> +
> +    <div id="summary_input">
> +      <table id="summary">

This looks bad after alias is removed in that the summary input field is the full width of the page and is below "Save Changes" when expanded. I would just remove the table and use <div>s for the label and input.

@@ +857,5 @@
> +    [% INCLUDE "bug/field-label.html.tmpl"
> +       field = bug_fields.alias, editable = 1
> +    %]
> +    <td>
> +      <label for="bugaliaslist">

do not see this being linked anywhere with the id="bugaliaslist". Just remove as not needed.

@@ +865,5 @@
> +          None
> +        [% END %]
> +      </label>
> +      [% IF bug.check_can_change_field('alias', 0, 1) %]
> +      <span id="alias_edit_area_showhide_container" class="bz_default_hidden">

indent

@@ +868,5 @@
> +      [% IF bug.check_can_change_field('alias', 0, 1) %]
> +      <span id="alias_edit_area_showhide_container" class="bz_default_hidden">
> +        (<a href="#" id="alias_edit_area_showhide">edit</a>)
> +      </span>
> +      <br>

not needed

@@ +876,5 @@
> +            <label for="aliases">
> +              <b>Add</b>
> +            </label>
> +          </div>
> +            <input name="newalias" id="newalias" size="20">

This should allow for comma/space separated alias names similar to CC. If I do "foo1, foo2" I get an error.

@@ +886,5 @@
> +            [% END %]
> +          </select>
> +          <br>
> +          <input type="checkbox" id="removealias" name="removealias">
> +          [%%]<label for="removealias">Remove selected aliases</label>

I do not see where [%%] is necessary as when I remove it locally it looks the same without.

@@ +887,5 @@
> +          </select>
> +          <br>
> +          <input type="checkbox" id="removealias" name="removealias">
> +          [%%]<label for="removealias">Remove selected aliases</label>
> +          <br>

this <br> is also not needed.

Comment 4

[Simon Green]

Attachment: bug1012506-v2.patch

Comment 5

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #3)
> Comment on attachment 8450769 [details] [diff] [review]
> alias-v1.patch
> 
> Review of attachment 8450769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Searching with "Alias" "contains all of the strings" "foo1 foo2" when I have
> a bug with both foo1 and foo2 set as aliases, does not show the bug. 
> 
> Will do further functional testing when the below are addressed.

This is intentional, and is the same reason why the following returns no results either (using the above logic, it should return this bug):

https://bugzilla.mozilla.org/buglist.cgi?f1=cc&o1=substring&query_format=advanced&v1=justdave%20dkl&product=Bugzilla

Searching 'contains the string' expects the value to be in a single thing, not across multiple things. I think this is the way it should work, even though it may be counter intuitive some times.

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8470512 [details] [diff] [review]
bug1012506-v2.patch

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

Sorry that I did not protest earlier, but we need to worry with API compatibility on this one and
add some changes to allow old clients to still work properly. Meaning we still need to return 'alias'
as a single string with one alias in the v1 or the API. We can add a new key where needed called 'alias_list'
or similar that will return all aliases for those that need it. Then for v2 when we go through and make
sweeping API improvements, we can go back to a single 'alias' key with the an array of aliases instead.

Thoughts on a better way to handle this issue?

::: Bugzilla/WebService/Bug.pm
@@ +467,4 @@
>          # alias is returned in case users passes a mixture of ids and aliases
>          # then they get to know which bug activity relates to which value  
>          # they passed
> +        $item{alias} = [ map { $self->type('string', $_) } @{ $bug->alias } ];

We can leave this as a string for backward compat and have it return the first alias if more than one as it will still get the user back to the bug. optionally we could also add 'alias_list' for the full list similar to comments below. API v2 would just be 'alias' as a list.

@@ +1195,5 @@
>      # First we handle any fields that require extra SQL calls.
>      # We don't do the SQL calls at all if the filter would just
>      # eliminate them anyway.
> +    if (filter_wants $params, 'alias') {
> +        $item{alias} = [ map { $self->type('string', $_) } @{ $bug->alias } ];

In thinking about this some more, we cannot really do this and some other similar changes as this breaks compatibility with clients who still expect the alias to to be a string. 

What I propose is we still return the first alias (oldest for those migrated) as 'alias' and create a new key called 'alias_list' that has all of the aliases as an array. 

When we move to API v2, we can drop the 'alias_list' and make 'alias' the array.

::: template/en/default/bug/edit.html.tmpl
@@ +194,5 @@
> +    <div id="summary_input">
> +      [% INCLUDE "bug/field-label.html.tmpl"
> +        field = bug_fields.short_desc
> +        editable = 1
> +        accesskey = "s"

Add tag_name = 'span' here and line up the attributes. Otherwise it still adds <th> tags and creates broken html.

@@ +196,5 @@
> +        field = bug_fields.short_desc
> +        editable = 1
> +        accesskey = "s"
> +      %]
> +      [% PROCESS input inputname => "short_desc" size => "80" colspan => 2

Nit: remove colspan => 2 as it is not needed here anymore.

@@ +875,5 @@
> +            <select id="alias" name="alias" multiple="multiple" size="5">
> +              [% FOREACH a = bug.alias %]
> +                <option value="[% a FILTER html %]">[% a FILTER html %]</option>
> +              [% END %]
> +            </select>

Add <br> below <select>

Comment 7

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #6)
> Comment on attachment 8470512 [details] [diff] [review]
> bug1012506-v2.patch
> 
> Review of attachment 8470512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry that I did not protest earlier, but we need to worry with API
> compatibility on this one and
> add some changes to allow old clients to still work properly. Meaning we
> still need to return 'alias'
> as a single string with one alias in the v1 or the API. We can add a new key
> where needed called 'alias_list'
> or similar that will return all aliases for those that need it. Then for v2
> when we go through and make
> sweeping API improvements, we can go back to a single 'alias' key with the
> an array of aliases instead.
> 
> Thoughts on a better way to handle this issue?

Apologies on this as it was pointed out to me, we did in fact discuss this in a past 
Bugzilla meeting and found it to be acceptable to make this change for the 5.0 release
and beyond without worry of breaking the API.

Please fix the remaining issues and I will do a quick review once it is ready.

dkl

Comment 8

[Simon Green]

Attachment: bug1012506-v3.patch

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8472082 [details] [diff] [review]
bug1012506-v3.patch

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

r=dkl

Comment 10

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   6dbcec0..424b21e  master -> master

Comment 11

[Frédéric Buclin]

The bugs.alias column is recreated every time you run checksetup.pl. Reopening!

Comment 12

[Frédéric Buclin]

Attachment: addl. fix

You must remove code which brings the column back.

Comment 13

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #11)
> The bugs.alias column is recreated every time you run checksetup.pl.
> Reopening!

Ugh. Good catch on that one. I ran the checksetup.pl to test the migration/conversion but did not run it more than that to see that the old column was being added back.

dkl

Comment 14

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   41d7aaa..7e1bdaa  master -> master

Comment 15

[Frédéric Buclin]

Added to relnotes for 5.0rc1.

Comment 16

[Frédéric Buclin]

You forgot to fix the Bugzilla DTD to allow multiple aliases:

To ssh://[e-mail]/bugzilla/bugzilla.git
   446656c..691e8bc  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   3db189f..34eb8a1  5.0 -> 5.0