349256 - Make the webservice get_bug into a stable API

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

As it stands, Bug.get_bug just returns the internals of a Bugzilla::Bug object, which isn't a stable API (that is, it can't be relied on to stay the same between versions of Bugzilla).

So, basically, we need to re-write it in such a way that it can become a documented, stable API for accessing certain data about a bug.

If we were using true SOAP, we could return an object with methods, but we're not. Anyhow, I think that would get pretty complicated. We're just using XML-RPC, so we have to return a hash with a specified structure.

Comment 1

[Max Kanat-Alexander]

Attachment: v1 (use -p1)

Okay, here we go. This is a pretty good example of what a stable section of the API should look like, complete with documentation.

Why does the stable API provide so few bug fields? Well, it's because all of the other bug fields would be unstable. For example, we could return the current resolution as a string, but might we not want to return a Bugzilla::Resolution object instead? Probably not, but these are all discussable things. As such, I've left anything controversial entirely out of this patch.

Another thing you'll notice is that the API specifies the *types* of its returned values. This means that we have to guarantee that the data will actually be returned as those types. That's why I use "type" throughout the patch.

Bugzilla::WebService::Bug has a bit of documentation that should eventually go into Bugzilla::WebService. (The stuff that describes STABLE, EXPERIMENTAL, and UNSTABLE.)

Comment 2

[Frédéric Buclin]

Comment on attachment 234525 [details] [diff] [review]
v1 (use -p1)

>+++ mkanat/Bugzilla/WebService.pm	2006-08-18 16:34:36.000000000 -0700

>+sub datetime_format {

>+    warn "String: $date_string Time: $time DateTime: $iso_datetime";

debug code here...

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Okay, I've updated this patch to use named parameters, as we discussed on the mailing list.

What I'm thinking for the future of this API is that it will have an "include" parameter, which will be a pointer to an array of fields to include. That will include other fields beyond just the default fields, particularly for things like comments and so on.

Comment 4

[Marc Schumann [:Wurblzap]]

Hmmm... This is no real review yet, but a request for discussion: do we want one stable API and that's it? I think a single stable API is pretty unrealistic at this time because there is a demand for information from clients at the same time as there is a lot of fluctuation in Bugzilla's code, so I'm in favour of something else which closely resembles two threads of development: one thread following the current agreed-on bug object very semi-stablily, and one thread trying to be as stable as possible. So, I think we should have two get_bug functions (plus ones which supply attachments and comments).

Comment 5

[Max Kanat-Alexander]

I'm in favor of having one stable API, but part of the return value is unstable. That is, the "internals" param is unstable. I think maintaining two APIs would just be nightmarish as far as maintenance and code complexity go.

We don't need a separate function to return attachments and comments--those can be returned as part of this function if they're specified in "include".

Comment 6

[Marc Schumann [:Wurblzap]]

Sounds good to my ears.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 234607 [details] [diff] [review]
v2

This needs to be updated to use error codes. And anyhow, I want the create() stuff to go in first.

Comment 8

[Mads Bondo Dydensborg]

Looking at the patch, I can't help but thinking that it is a very limited set of information that is returned in a STABLE way.

Personally I would very much like to see product, component, status and assignee as part of the returned result.

Comment 9

[Mads Bondo Dydensborg]

(In reply to comment #7)
> (From update of attachment 234607 [details] [diff] [review] [edit])
> This needs to be updated to use error codes. And anyhow, I want the create()
> stuff to go in first.
> 

I do not know if this is related to your comment about error codes, but if I try to call an unsupported method in current cvs, the fault response is malformed. This is an example:

<?xml version="1.0" encoding="UTF-8"?><methodResponse><fault><value><struct><member><name>faultString</name><value><string>Failed to locate method (create) in class (Bugzilla::WebService::Bug) at /usr/share/perl5/SOAP/Lite.pm line 2545.
</string></value></member><member><name>faultCode</name><value><string>Client</string></value></member></struct></value></fault></methodResponse>

The type for faultCode should, in all cases, be <int> (and the value some numeric, obviously). My toolkit refuses to parse the faultCode in this case.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #8)
> Looking at the patch, I can't help but thinking that it is a very limited set
> of information that is returned in a STABLE way.

  You're certainly right.

  You are also free to use the XML format of a bug to get as much or as little information about a bug as you'd like.

(In reply to comment #9)
> I do not know if this is related to your comment about error codes, 
> [snip]

  It is. :-)

Comment 11

[Mads Bondo Dydensborg]

(In reply to comment #10)
> (In reply to comment #8)
> > Looking at the patch, I can't help but thinking that it is a very limited set
> > of information that is returned in a STABLE way.
> 
>   You're certainly right.

;-)

> 
>   You are also free to use the XML format of a bug to get as much or as little
> information about a bug as you'd like.

You mean, doing an ordinary http request at http://example.org/show_bug.cgi?ctype=xml&id=<someid>
and parsing the output? Obviously I can do that - I just thought the point of the webservice API was to not have to do stuff like that.

Do you have objections to adding the other information to the bug? I am trying to understand here, not argue ;-)

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> You mean, doing an ordinary http request at
> http://example.org/show_bug.cgi?ctype=xml&id=<someid>
> and parsing the output? Obviously I can do that - I just thought the point of
> the webservice API was to not have to do stuff like that.

  Yeah. You're right, that is the point of the API. But the API won't be totally complete for 3.0, it'll just be very basic.

> Do you have objections to adding the other information to the bug? I am trying
> to understand here, not argue ;-)

  The problem is that the format of every other field is debateable. Should we return product names or product ids? Well, probably product ids. But then we have to have a Product.get_product. What about resolutions? It wouldn't be sensible at this point to return ids for resolutions, and maybe it never would. But what if we change something about resolutions in the future so that there can be multiple resolutions with the same name?

  Basically, I left everything controversial out of this patch, so that it could be granted review.

Comment 13

[Mads Bondo Dydensborg]

(In reply to comment #12)
> > Do you have objections to adding the other information to the bug? I am trying
> > to understand here, not argue ;-)
> 
>   The problem is that the format of every other field is debateable. Should we
> return product names or product ids? Well, probably product ids. 

I think ids should always be returned. After all, they are the unique identifier for the information returned.

However, I think, as a guideline, in the most common case, when the overhead is low, and it eliminates a roundtrip to the server, information that is suitable for user consumption should also be returned.

So, I would advocate that _both_ id and name is returned. 
Likewise for the component, status, resolution, version, priority, and severity.

Consider the case, where I would like to present this information to a user. I would have to get the bug, then make 6 calls (6 roundtripds) to translate between id and names. Unless, of course, I had already retrieved 6 lists of the mappings between e.g. product id and names.

Alternatively, _extended methods (or similar) should be provided for get_bug, that returns the information as both ids and strings... perhaps a better solution? Or, a parameter to get_bug? (and related methods). Thats probably the best solution.

However: my use cases all involve user consumption, so I may be a bit biased...

> But then we
> have to have a Product.get_product. 

There is a get_product in Product.pm - I shall have a look at it.

>   Basically, I left everything controversial out of this patch, so that it
> could be granted review.

Well, by returning names, you would make it considerably more useful. 

Comment 14

[Mads Bondo Dydensborg]

(In reply to comment #13)
> (In reply to comment #12)
> >   Basically, I left everything controversial out of this patch, so that it
> > could be granted review.

One final comment: Could we make the parameter called "id" instead of "bug_id"? 

That would make it easier to make "generic" parameters in clients that use languages that are less flexible than perl ;-)


> 
> Well, by returning names, you would make it considerably more useful. 
> 

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> One final comment: Could we make the parameter called "id" instead of "bug_id"? 

  Yes, I think that's an excellent idea.

Comment 16

[Max Kanat-Alexander]

This is a blocker, in the sense that this function needs to be made stable or removed, for 3.0.

Comment 17

[Max Kanat-Alexander]

Attachment: v3

Okay, in the spirit of our other patches, this is now get_bugs, and I've updated it to take appropriate parameters and return appropriate things.

Comment 18

[Mads Bondo Dydensborg]

Comment on attachment 245357 [details] [diff] [review]
v3

>+        if (Bugzilla->params->{'usebugalias'}) {

This needs to be 'usebugaliases'

>+Note that it's possible for aliases to be disabled in Bugzilla, in which
>+case you will be told that you have specified an invalid bug_id if you
>+try to specify an alias.

this descriptions seems to go a bit against this:

>+=item 100 (Invalid Bug Alias)
>+
>+If you specified an alias and either: (a) the Bugzilla you're querying
>+doesn't support aliases or (b) there is no bug with that alias.
>+
>+=item 101 (Invalid Bug ID)
>+
>+The bug_id you specified doesn't exist in the database.

I have given it -, because I do not know to what limits that commiter can be asked to change stuff. 

But, apart from the +es on the bugalias, this passes my tests. (Note, that I am uncertain about authentication tests, but I reckon this should be handled by ValidateBugId)

Comment 19

[Max Kanat-Alexander]

Attachment: v4

Okay, fixed your comments. :-)

Comment 20

[Frédéric Buclin]

Comment on attachment 245409 [details] [diff] [review]
v4

>Index: Bugzilla/WebService.pm

>+sub datetime_format {

>+    warn "String: $date_string Time: $time DateTime: $iso_datetime";


Hum, I suppose |warn| is debug stuff and should go away?

Comment 21

[Mads Bondo Dydensborg]

Comment on attachment 245409 [details] [diff] [review]
v4

The warn line must be removed by the committer:
>Index: Bugzilla/WebService.pm

>+sub datetime_format {

>+    warn "String: $date_string Time: $time DateTime: $iso_datetime";

Otherwise, this should go in, I reckon ;-)

Comment 22

[Max Kanat-Alexander]

Okay. Removed the line on checkin.

Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.4; previous revision: 1.3
done
Checking in contrib/bz_webservice_demo.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bz_webservice_demo.pl,v  <--  bz_webservice_demo.pl
new revision: 1.6; previous revision: 1.5
done

Comment 23

[Frédéric Buclin]

test script: webservice_bug_get_bugs.t