573195 - Make Bug.get return all of a bug's standard and custom field information

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

Bug.get should be able to return all of a bug's basic field information, other than flags, attachments, comments, and history.

Comment 1

[Max Kanat-Alexander]

Attachment: v1 (Needs POD)

Okay, here we go. This adds everything to Bug.get that isn't already returned by the other functions (though Bug.attachments still needs to be able to return attachment data).

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 452440 [details] [diff] [review]
v1 (Needs POD)

Patch is no longer valid as group parts are already committed in a different bug. 

Dave

Comment 3

[Tiago Mello [:timello]]

Comment on attachment 452440 [details] [diff] [review]
v1 (Needs POD)

Yup. The patch needs to be updated.

Comment 4

[Max Kanat-Alexander]

Attachment: v2 (Needs POD)

Okay, here's a patch that applies and is a little nicer.

I'll do the POD once the code passes review.

Comment 5

[David Lawrence [:dkl]]

Sorry for the delay on this review. Timello, if you do not have time I can try to take a look at it now.

But it will need a new patch regardless as it conflicts with the recent patch checked in from bug 579514.

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/get-more.diff 
patching file Bugzilla/WebService/Bug.pm
Hunk #1 FAILED at 29.
Hunk #2 succeeded at 776 (offset 138 lines).
1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/WebService/Bug.pm.rej
patching file Bugzilla/WebService/Util.pm
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
3 out of 3 hunks ignored -- saving rejects to file Bugzilla/WebService/Util.pm.rej

Comment 6

[Max Kanat-Alexander]

Attachment: v3 (Needs POD)

Okay, here's a fixed patch.

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 458804 [details] [diff] [review]
v3 (Needs POD)

>+    if (filter_wants $params, 'assigned_to') {
>+        $item{'assigned_to'} = $self->type('string', $bug->assigned_to->login);
>+    }

I would really like to see this email filtered if client is not logged in.

>+    if (filter_wants $params, 'creator') {
>+        $item{'creator'} = $self->type('string', $bug->reporter->login);
>+    }

Same here

Also a section for qa_contact should be added if Bugzilla->params->{'useqacontact'}.

>+    # Timetracking fields are only sent if the user can see them.
>+    if (Bugzilla->user->is_timetracker) {
>+        $item{'estimated_time'} = $self->type('double', $bug->estimated_time);
>+        $item{'remaining_time'} = $self->type('double', $bug->remaining_time);
>+        $item{'deadline'}       = $self->type('dateTime', $bug->deadline);
>+    }

$item{'deadline'} causes DateTime->new to throw an error due to deadline being a
date only and missing hour/minute/second portion in the hash passed in.

The 'hour' parameter (undef) to DateTime::new was an 'undef', which is not one of the allowed types: scalar
 at /usr/lib64/perl5/DateTime.pm line 176
        DateTime::new(undef, 'HASH(0x35cdc70)') called at Bugzilla/Util.pm line 448
        Bugzilla::Util::datetime_from('2010-07-29', 'UTC') called at Bugzilla/WebService/Server.pm line 53
        Bugzilla::WebService::Server::datetime_format_outbound('Bugzilla::WebService::Server', '2010-07-29') called at Bugzilla/WebService.pm line 55
        Bugzilla::WebService::datetime_format_outbound('Bugzilla::WebService::Bug', '2010-07-29') called at Bugzilla/WebService.pm line 46
        Bugzilla::WebService::type('Bugzilla::WebService::Bug', 'dateTime', '2010-07-29') called at Bugzilla/WebService/Bug.pm line 879
        Bugzilla::WebService::Bug::_bug_to_hash('Bugzilla::WebService::Bug', 'Bugzilla::Bug=HASH(0x3516938)', 'HASH(0x30e7828)') called at Bugzilla/WebService/Bug.pm line 332
        Bugzilla::WebService::Bug::get('Bugzilla::WebService::Bug', 'HASH(0x30e7828)') called at /usr/lib/perl5/vendor_perl/5.10.0/SOAP/Lite.pm line 2797
        eval {...} called at /usr/lib/perl5/vendor_perl/5.10.0/SOAP/Lite.pm line 2782
        eval {...} called at /usr/lib/perl5/vendor_perl/5.10.0/SOAP/Lite.pm line 2748
        SOAP::Server::handle('Bugzilla::WebService::Server::XMLRPC=HASH(0x2e04a30)', '<?xml version="1.0" encoding="UTF-8"?><methodCall><methodName...') called at /usr/lib/perl5/vendor_perl/5.10.0/SOAP/Transport/HTTP.pm line 427
        SOAP::Transport::HTTP::Server::handle('Bugzilla::WebService::Server::XMLRPC=HASH(0x2e04a30)') called at /usr/lib/perl5/vendor_perl/5.10.0/SOAP/Transport/HTTP.pm line 580
        SOAP::Transport::HTTP::CGI::handle('Bugzilla::WebService::Server::XMLRPC=HASH(0x2e04a30)') called at /var/www/html/bugzilla/xmlrpc.cgi line 48

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> I would really like to see this email filtered if client is not logged in.

   Mmm, well, this patch is going into 4.0, and the email filtering changes in the WebService aren't happening until 4.2. So I think we should wait, for that.

> Also a section for qa_contact should be added if
> Bugzilla->params->{'useqacontact'}.

  Oh, I think qa_contact should be added unconditionally, actually. I don't really want parameters to control how the API works, anywhere that I can avoid it.

> $item{'deadline'} causes DateTime->new to throw an error due to deadline being
> a
> date only and missing hour/minute/second portion in the hash passed in.

  Ahhh, right, I will have to fix that.

Comment 9

[David Lawrence [:dkl]]

(In reply to comment #8)
> (In reply to comment #7)
> > I would really like to see this email filtered if client is not logged in.
> 
>    Mmm, well, this patch is going into 4.0, and the email filtering changes in
> the WebService aren't happening until 4.2. So I think we should wait, for that.

Thats fine. We (RH) will have to add the checks into the 4.0 code as a customization when we port to it as it is our policy to not expose email addresses to non-logged in users through any API. When I do the work for 4.0, I will create a patch for it to be added into 4.2.

Dave

Comment 10

[Max Kanat-Alexander]

Attachment: v4

All right, this fixes the deadline problem (by fixing datetime_from), and adds qa_contact. I also renamed the cclist_accessible and reporter_accessible fields to start with "is" and more accurately match what I recommended for BzAPI.

Comment 11

[Max Kanat-Alexander]

Gerv: This patch provides full bug data, so you might just want to make sure that we are more-or-less in sync with what you're doing in BzAPI.

Comment 12

[Max Kanat-Alexander]

Attachment: v5

Also document include_fields and exclude_fields support.

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 463476 [details] [diff] [review]
v5

>+    if (filter_wants $params, 'qa_contact') {
>+        $item{'qa_contact'} = $self->type('string', $bug->qa_contact->login);
>+    }

This generates an error if qa_contact is not defined.

Can't call method "login" on an undefined value at Bugzilla/WebService/Bug.pm line 855.

>+    # And now custom fields
>+    my @custom_fields = Bugzilla->active_custom_fields;
>+    foreach my $field (@custom_fields) {
>+        my $name = $field->name;
>+        next if !filter_wants $params, $name;
>+        if ($field->type == FIELD_TYPE_BUG_ID) {
>+            $item{$name} = $self->type('int', $bug->$name);
>+        }
>+        elsif ($field->type == FIELD_TYPE_DATETIME) {
>+            $item{$name} = $self->type('dateTime', $bug->$name);
>+        }

This generates an error for any undefined dateTime value because of below

>+    # Timetracking fields are only sent if the user can see them.
>+    if (Bugzilla->user->is_timetracker) {
>+        $item{'estimated_time'} = $self->type('double', $bug->estimated_time);
>+        $item{'remaining_time'} = $self->type('double', $bug->remaining_time);
>+        $item{'deadline'}       = $self->type('dateTime', $bug->deadline);
>+    }

This generates an error if deadline is undefined.

Can't call method "iso8601" on an undefined value at Bugzilla/WebService/Server.pm line 55.

Dave

Comment 14

[Frédéric Buclin]

dkl: so how can this be r+ with all these errors being thrown?

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 463476 [details] [diff] [review]
v5

Sorry. I meant r- :)

Comment 16

[David Lawrence [:dkl]]

(In reply to comment #13)
> Comment on attachment 463476 [details] [diff] [review]
> v5
> 
> >+    if (filter_wants $params, 'qa_contact') {
> >+        $item{'qa_contact'} = $self->type('string', $bug->qa_contact->login);
> >+    }
> 
> This generates an error if qa_contact is not defined.
> 
> Can't call method "login" on an undefined value at Bugzilla/WebService/Bug.pm
> line 855.

Also, should part be omitted completely if not Bugzilla->params->{'useqacontact'}?

Dave

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> Also, should part be omitted completely if not
> Bugzilla->params->{'useqacontact'}?

  No--parameters should only control the UI, not the backend. The general idea is that we have field-controlling parameters only because they make for a cleaner UI.

Comment 18

[Max Kanat-Alexander]

Attachment: v6

Okay, this should fix the errors!

Comment 19

[David Lawrence [:dkl]]

As I was working on something similar for our Bug.get and playing around with adding this patch as it is now to our implementation, I have come up with some questions and concerns.

1. The data fields that would execute additional SQL are wrapped in filter_wants conditionals, but if the user doesn't specify anything, then they will still get all of the fields even though it can be slower due to the extra database calls. Shouldn't it be that we just return the basic fields unless the client explicitly asks for the extra fields? This way the basic call is as fast as it can be by default.

2. Users in Red Hat have asked for attachments, comments, and flags to be returned by Bug.get if they are explicitly asked for. If I add them in using the filter_wants method used in this patch, the user will get them even if they do not specify anything specific. To not get them with a basic call to Bug.get, they would have to know to add the field names to 'exclude_fields' in the Bug.get params. And if I code it to have them not returned by default unless listed in 'include_fields', then if the client passes those values in, they *only* get those fields and not those + the default basic fields as expected.

3. The way I would understand the 'include_fields' term it would mean to me to 'include' the fields to the default list of returned fields. So if I had say 'attachments' off by default, passing in 'include_fields' => ['attachments'] then I should get the standard set of fields PLUS the 'attachments' data. Then 'exclude_fields' should also do as it says and exclude fields from the default set.

4. Maybe we should have a 'fields' or 'only_fields' that does what 'include_fields' currently does and have it specify the exact fields to return. Similar to how 'columnlist' works for buglist.cgi. The filter_wants() could be redone to look at each of the three params to determine whether to return true/false for a given field.

Just some food for thought, feel free to ignore me if I am off base :)

Dave

Comment 20

[Gervase Markham [:gerv]]

Dave: a while back, Max and I bashed out a spec for how fields should be included and excluded, and the BzAPI conforms to it (and I think the WebService may now, too). However, experience with it on the BzAPI end has shown it to be a bit cumbersome in some common cases - like having the default fields and then adding some. So it may be worth revisiting, along the lines you have suggested.

Should we take this to the newsgroup?

Gerv

Comment 21

[Max Kanat-Alexander]

(In reply to comment #19)
> 1. The data fields that would execute additional SQL are wrapped in
> filter_wants conditionals, but if the user doesn't specify anything, then they
> will still get all of the fields even though it can be slower due to the extra
> database calls. Shouldn't it be that we just return the basic fields unless the
> client explicitly asks for the extra fields? This way the basic call is as fast
> as it can be by default.

  I thought about this.

  For some fields, it would currently require breaking our API on a stable call, so we're probably not going to do it for the existing fields, at the very least.

  For the rest of the fields, I think that it's simpler if we just return everything (except comments and attachments) and tell people in the POD that if they want things to be faster, they should use include_fields and exclude_fields.

  I'm open to discussion on it, but I was kind of leaning towards making things easy for people by default, and then if they want better performance, they can choose that.

> 2. Users in Red Hat have asked for attachments, comments, and flags to be
> returned by Bug.get if they are explicitly asked for.

  Ah, yes, there's a planned method to add those, called "extra_fields".

  I've actually implemented it once, in another situation, and then didn't attach the patch.

  We'll do it in a future patch.


  Questions 3 + 4 will be obsoleted by extra_fields.

Comment 22

[Max Kanat-Alexander]

(In reply to comment #20)
> However, experience with it on the BzAPI end has shown it to be
> a bit cumbersome in some common cases - like having the default fields and then
> adding some.

  Yeah, I think that the "_default" and "_custom" stuff we talked about is probably too cumbersome and shouldn't be done.

  I think extra_fields, include_fields, and exclude_fields will be enough by themselves.

Comment 23

[Gervase Markham [:gerv]]

OK... Can you write up a brief yet comprehensive spec, in the wiki or somewhere, and I'll implement it for the next BzAPI release?

I'm assuming that it'll be something like:

- Each call has a documented default set of fields
- In some cases, that set may be "all fields"; in others, it may be "no fields"
- include_fields starts from scratch and adds fields
- exclude_fields starts from default and removes fields
- extra_fields starts from default and adds fields
  (so if default is nothing, it's the same as include_fields, and if default is 
   everything, it has no effect)
- If you specify more than one of the three, results are undefined.
  (Or do we want to allow people to specify both exclude_fields and 
   extra_fields? That's the only pair which makes any sense at all.)

The only trouble is, there's no way of saying "everything ya got, whatever it is" for calls where the default is not everything. Do we still keep the "_all" special value for include_fields (and, presumably, extra_fields too)?

Gerv

Comment 24

[Max Kanat-Alexander]

(In reply to comment #23)
> - In some cases, that set may be "all fields"; in others, it may be "no fields"

  It's never "no fields".

> - include_fields starts from scratch and adds fields
> - exclude_fields starts from default and removes fields

  Right--these work as documented now--no change.

  However, you could do:

  extra_fields => ['attachments'], exclude_fields => ['attachments.data']

> - extra_fields starts from default and adds fields

  More or less, yeah. It adds them based on the names of other methods within the same class. So for Bug.get, there will be "attachments" and "comments".

  Each function will document which extra_fields items it supports.

  Anything that goes into extra_fields can also go into include_fields.

> - If you specify more than one of the three, results are undefined.

  No, they have a very definite precedence order: exclude_fields, include_fields, extra_fields.

>   (Or do we want to allow people to specify both exclude_fields and 
>    extra_fields? That's the only pair which makes any sense at all.)

> The only trouble is, there's no way of saying "everything ya got, whatever it
> is" for calls where the default is not everything. Do we still keep the "_all"
> special value for include_fields (and, presumably, extra_fields too)?

  Well, I'm not sure that there's a need to say "everything ya got, whatever it is", because we don't validity-check the *_fields values, so they are backwards and forwards compatible. So you could just ask every version of Bugzilla for "extra_fields => ['attachments']" and it simply wouldn't do anything in older versions.

Comment 25

[Gervase Markham [:gerv]]

(In reply to comment #24)
> > - If you specify more than one of the three, results are undefined.
> 
>   No, they have a very definite precedence order: exclude_fields,
> include_fields, extra_fields.

That leads to some confusion about the starting point, though, because the different parameters are documented as starting from different sets ("nothing" or "default").

E.g. default is fields A, B, C. There also exist optional fields D, E.

1) exclude_fields = A, include_fields = D, extra_fields = E --> B, C, D, E

2)                     include_fields = D, extra_fields = E --> D, E

3)                                         extra_fields = E --> A, B, C, E

I.e. between 1) and 2), removing "exclude_fields = A" from the query results in the removal from the output of fields B and C! And between 2) and 3), removing "include_fields = D" from the query results in the removal of D but the addition of fields A, B and C!

That's not particularly intuitive IMO.

>   Well, I'm not sure that there's a need to say "everything ya got, whatever it
> is", 

I've used this several times. One example is a Bugzilla backup application. There should be no need to update the application every time new fields get added. And how do you ask for e.g. custom fields you don't even know the name of?

If every time someone wants to make an API call, they have to make a call to another API (in BzAPI, "/configuration") to get information about what to ask for, that makes the API much more cumbersome. I think there is still a good case for _all and even for _custom.

Gerv

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> 1) exclude_fields = A, include_fields = D, extra_fields = E --> B, C, D, E

  No, that combination will result in only D and E being shown.

> >   Well, I'm not sure that there's a need to say "everything ya got, whatever it
> > is", 
> 
> I've used this several times. One example is a Bugzilla backup application.
> There should be no need to update the application every time new fields get
> added. And how do you ask for e.g. custom fields you don't even know the name
> of?

  Ah, okay, fair enough. :-) We could implement _all. It's not that hard--we just have to modify filter_wants.

> If every time someone wants to make an API call, they have to make a call to
> another API (in BzAPI, "/configuration") to get information about what to ask
> for, that makes the API much more cumbersome. I think there is still a good
> case for _all and even for _custom.

  I can see that. People would want to insert _custom into exclude_fields, perhaps?

Comment 27

[Gervase Markham [:gerv]]

(In reply to comment #26)
> (In reply to comment #25)
> > 1) exclude_fields = A, include_fields = D, extra_fields = E --> B, C, D, E
> 
>   No, that combination will result in only D and E being shown.

The fact that I got that wrong shows, IMO, that there are multiple mental models of how this could work, and it's not obvious which one is the right one. This is going to lead to confusion.

Why does that combination result in only D and E being shown? If, as you say in comment 24, exclude_fields has priority over include_fields, then surely exclude_fields gets to define the starting point (i.e. you start with the default set), rather than include_fields getting to define it (i.e. you start with nothing)?

Or is the precedence order actually include_fields, exclude_fields, extra_fields?

This is what I mean by combinations being confusing. It's not clear whether you start with the default set or the empty set. How would you articulate the rules for that, under your scheme?

>   Ah, okay, fair enough. :-) We could implement _all. It's not that hard--we
> just have to modify filter_wants.

Awesome.

>   I can see that. People would want to insert _custom into exclude_fields,
> perhaps?

Well, are custom fields ever going to be part of the default set for any call?

Gerv

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> The fact that I got that wrong shows, IMO, that there are multiple mental
> models of how this could work, and it's not obvious which one is the right one.
> This is going to lead to confusion.

  I can see that. I'd guess, though, that that's probably mostly because extra_fields doesn't exist yet, and so it doesn't have docs. include_fields and exclude_fields do have docs, here:

  http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService.html#Limiting_What_Fields_Are_Returned

> Or is the precedence order actually include_fields, exclude_fields,
> extra_fields?

  It's in the docs.

> >   I can see that. People would want to insert _custom into exclude_fields,
> > perhaps?
> 
> Well, are custom fields ever going to be part of the default set for any call?

  Yes, they're part of the default set for Bug.get right now, in the patch, on this bug.

Comment 29

[Gervase Markham [:gerv]]

> Yes, they're part of the default set for Bug.get right now, in the patch, on
> this bug.

I clearly misunderstood the word "basic" in the bug title, then :-)

As for the precedence order question, I think we are approaching it from opposite ends. I am sort of talking about "execution order", i.e. "process parameter X, then parameter Y", and you are talking about precedence order, as in "whatever's in parameter Y takes precedence over X". So I think we agree on the model. But I still think the addition of extra_fields causes some combinatorial problems.

Would you be so kind as to spec how you think extra_fields should interact with the other two?

Gerv

Comment 30

[Max Kanat-Alexander]

(In reply to comment #29)
> Would you be so kind as to spec how you think extra_fields should interact with
> the other two?

  I think the simplest way to do that would be to file a bug that requires extra_fields (like "allow Bug.get to return comments and attachments") and then I will simply write the code and the POD that will be the spec.

Comment 31

[Max Kanat-Alexander]

Dave: Can I get a review on the patch as it exists? The other stuff will be handled by future patches.

Comment 32

[Gervase Markham [:gerv]]

(Dave: this discussion should not hold up review of these patches.)

Max: I would much rather discuss it before you implement it rather than after. It's significantly easier to change plans than it is to change code, and if code is not written, people can't get upset that you are suggesting they rework it. :-) Given the complexities we have uncovered, I would much rather know exactly what you plan to do, and comment on it, before you actually do it. Is that reasonable?

Gerv

Comment 33

[David Lawrence [:dkl]]

Comment on attachment 472576 [details] [diff] [review]
v6

Something that no longer works after your patch is applied is the ability to pass login information to Bug.get instead of relying on a cookie being set.

The following worked before the patch:

$call = $rpc->call( 'Bug.get', { ids => \@bugs, Bugzilla_login => $username, Bugzilla_password => $password } );

But when the patch is applied I get the following error:

Can't call method "iso8601" on an undefined value at Bugzilla/WebService/Server.pm line 59.

If I change the following line in Bugzilla::WebService::Server::datetime_format_outbound:

-    return undef if !defined $date;
+    return undef if !$date;

then it works again. Some reason with Bugzilla_login/Bugzilla_password, $date is defined but empty.

Dave

Comment 34

[Max Kanat-Alexander]

Attachment: v1

The problem was the "deadline" field--because of the way that the AUTOLOAD works in Bugzilla::Bug, it was returning an empty string when the field was NULL.

Comment 35

[David Lawrence [:dkl]]

Comment on attachment 478965 [details] [diff] [review]
v1

Looks good and works as expected. r=dkl

Comment 36

[Max Kanat-Alexander]

Woo hoo!!

Comment 37

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Util.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Server.pm
Committed revision 7496.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/Util.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Server.pm
Committed revision 7424.

Comment 38

[Frédéric Buclin]

Looks like this patch doesn't return the expected data:

#   Failed test 'Admin correctly gets time-tracking fields'
#   in webservice_bug_get_bugs.t at line 17.
not ok 29 - Admin correctly gets time-tracking fields
not ok 68 - Admin correctly gets time-tracking fields