458853 - Add a "permissive" argument that allows Bug.get to return even if there are errors

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Noura Elhawary]

This is a request from one of employees at redhat, I also think that what he is asking for is quite reasonable. Please read below and let me know what you think:

Background info: using the old RHBZ web services to fetch information about
multiple bugs, we would do a MultiCall invocation of the getBug() method - one
call for each bug. If one or more of the bugs was missing or unreadable, we
would replace those results with an appropriate error value and return a list
of results.

Bugzilla 3.x has Bug.get(), which lets us get multiple bugs. But there's a
problem - if *any* of the bugs requested are missing/unreadable, we get an
XML-RPC Fault and *no* return values. I could parse the Fault message, remove
the bad bug ID, and retry the call - repeating until I finally get results -
but that seems horribly inefficient.

Instead, I think Bug.get() should return results for each bug requested. For
bugs that are missing or unreadable, a different result hash can be returned,
instead of raising a fault and ending the whole call. An example error result:

{'id': bug_id, 
 'alias': alias or '', 
 'faultCode': faultCode, 
 'faultString': faultString}

A 'fault_on_error' argument could be added to Bug.get() to make it keep the
current behavior, if there are things that rely on that.

This should probably apply to all the Bug methods that take multiple bug ids,
and not just Bug.get().

Noura

Comment 1

[Max Kanat-Alexander]

Please set the os/platform/severity fields when filing a bug.

The solution here is to add an argument to Bug.get, yes. I want to call that argument "permissive" (I want to use the same name everywhere else in the WebService, too, for similar features).

I think the format of the return hash sounds OK, but it will be tricky to get faultCode and faultString. alias should probably only be included if the bug was asked for by alias.

Comment 2

[Max Kanat-Alexander]

To be totally clear, without an argument Bugzilla will behave as it does (strictly) and with an argument it will behave "politely".

The theory is that APIs should be as strict as possible by default.

Comment 3

[Noura Elhawary]

Attachment: v1 of adding permissive arg to Bug.get to make it behave politely

Hi Max,

attached is a patch that adds a permissive argument to Bug.get to make the function behave politely in the case of invalid bug ids passed to it , so we can call it like this:

$call = $rpc->call('Bug.get', {ids => [1,-1,-2,2], permissive => 1});

and for invalid bug ids we would get something like this:

                    {
                      'faultString' => '\'-1\' is not a valid bug number.',
                      'id' => '-1',
                      'faultCode' => '100'
                    },
                    {
                      'faultString' => '\'-2\' is not a valid bug number.',
                      'id' => '-2',
                      'faultCode' => '100'
                    },

Noura

Comment 4

[Max Kanat-Alexander]

Comment on attachment 346617 [details] [diff] [review]
v1 of adding permissive arg to Bug.get to make it behave politely

>Index: Bugzilla/WebService/Bug.pm
> [snip]
>+            eval { $bug = Bugzilla::Bug->check($bug_id); };
>+            if ($@) {
>+                my $faultstring = $@;
>+                push(@return, {id => $bug_id,
>+                               faultString => $faultstring->{_faultstring},
>+                               faultCode => $faultstring->{_faultcode},

  There should be better ways of getting that information than accessing $@ and using internal variables.

>-=item C<ids>
>+=item ids

 Why did you remove the C<>?

>+=item permissive
>+
>+C<boolean> An optional boolean argument, if set to 0 or ommitted then 
>+Bug.get will behave strictly by returning only an error message in the case
>+of any invalid bug ids or aliases passed to it, and if set to 1 then Bug.get
>+will return bug information for valid bug ids and faultstrings for invalid
>+bug ids.

 C<boolean> Normally, if you request any inaccessible or invalid bug ids, Bug.get will throw an error. If this parameter is True, instead of throwing an error we return a hash with a C<faultString> and C<faultCode> for each bug that fails, and return normal information for the other bugs that were accessible.

Comment 5

[Max Kanat-Alexander]

Actually...for the sake of consistency, should we return the faults in a separate return item called "faults"? Then every method could return the same way. But it might get confusing what exactly was faulting...probably best to just do it the way you're doing it, for now. (But let me know if you have any thoughts on this.)

Comment 6

[David Lawrence [:dkl]]

(In reply to comment #4)
> >+            eval { $bug = Bugzilla::Bug->check($bug_id); };
> >+            if ($@) {
> >+                my $faultstring = $@;
> >+                push(@return, {id => $bug_id,
> >+                               faultString => $faultstring->{_faultstring},
> >+                               faultCode => $faultstring->{_faultcode},
> 
>   There should be better ways of getting that information than accessing $@ and
> using internal variables.

I thought there should be a way to get at the SOAP::Fault->faultstring but it is not stored anywhere when the eval returns so I can only see getting this from $@. SOAP::Transport::HTTP doesn't seem to support a on_fault() handler that can be used to add the faultcode/faultstring to a storage place. on_fault() only works for client code from what I can tell.

We could possible create a wrapper for SOAP::Fault that allows us to store the values before dying.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch that overrides SOAP::Fault to store faultstring/faultcode for later retrieval

I have attached a patch that builds on Noura's work that wraps SOAP::Fault::faultcode and SOAP::Fault::faultstring so that we can store the values somewhere before dying. Then Bugzilla::WebService::Bug::get can look at Bugzilla->faultstring to see if it is populated and then use the values instead of having to access $@ directly. It works well in my testing. I just threw the values in Bugzilla.pm but can be located somewhere else like Bugzilla::WebService instead if it causes confusion.

Let me know if this looks doable.
Dave

Comment 8

[Max Kanat-Alexander]

Um, $@ is a SOAP::Fault. You should be able to call methods on it just fine.

Comment 9

[David Lawrence [:dkl]]

(In reply to comment #8)
> Um, $@ is a SOAP::Fault. You should be able to call methods on it just fine

Right, my bad then. I thought you said you didn't want to use the $@ or other internal variables so I was trying to figure out a way todo it without them. If $@ is ok then disregard my patch.

Comment 10

[Max Kanat-Alexander]

Yeah, $@ is fine, I reconsidered that part a little bit later, when I wasn't on the computer. Unfortunately $@ is the way we throw exceptions in Perl. :-(

Comment 11

[Noura Elhawary]

Attachment: v2 for adding permissive arg to Bug.get to make it behave politley

Hi Max,

Okay i found another way to parse $@ error returned by eval , which is to use the variable $EVAL_ERROR using the English module. please take a look at this solution and let me know what you think, if that doesn't look good , I am not really sure I would be able to find any other solutions.

Thanks,
Noura

Comment 12

[Max Kanat-Alexander]

Comment on attachment 346804 [details] [diff] [review]
v2 for adding permissive arg to Bug.get to make it behave politley

Oddly enough, Perl programmers never "use English", so $EVAL_ERROR is confusing, and $@ is more recognizable.

Also, you need to undef $@ at the bottom of your if, I'm pretty sure.

Comment 13

[Max Kanat-Alexander]

Also, dkl, do you have any thoughts on what I said in comment 5? I think it'd be easier to write a framework around the Bugzilla API if we did this more consistently.

Comment 14

[Noura Elhawary]

Attachment: v3 of adding permissive arg to Bug.get

Okay here is another patch that uses $@

Noura

Comment 15

[Max Kanat-Alexander]

This looks fine, but I'm waiting for dkl to respond to comment 13. Also, Igor and Mik are likely consumers of an API like this--what would you prefer? See comment 5, and also the existing patch.

Comment 16

[David Lawrence [:dkl]]

(In reply to comment #5)
> Actually...for the sake of consistency, should we return the faults in a
> separate return item called "faults"? Then every method could return the same
> way. But it might get confusing what exactly was faulting...probably best to
> just do it the way you're doing it, for now. (But let me know if you have any
> thoughts on this.)

I vote for leaving the fault in place of where the bug would have been. I think that would be as people would expect it if they were looping through the results in their client.

Dave

Comment 17

[David Lawrence [:dkl]]

(In reply to comment #16)
> (In reply to comment #5)
> > Actually...for the sake of consistency, should we return the faults in a
> > separate return item called "faults"? Then every method could return the same
> > way. But it might get confusing what exactly was faulting...probably best to
> > just do it the way you're doing it, for now. (But let me know if you have any
> > thoughts on this.)
> 
> I vote for leaving the fault in place of where the bug would have been. I think
> that would be as people would expect it if they were looping through the
> results in their client.

Hmm, after thinking about this a bit more, since the API is still evolving, we could still make a change such as this without worrying too much about what people are expecting.

So maybe returning a separate 'faults' list is not a bad idea. So for example:

Bug.get returns { bugs => \@bugs, faults => \@faults }
User.get returns { users => \@users, faults => \@faults }
Component.get returns { components => \@components, faults => \@faults }

and so on which I assume is similar to what you were suggesting. 

After this patch is committed, we could create a new bug that addresses converting all of the other *.get methods to also eval {} and return a faults
list.

Dave

Comment 18

[Max Kanat-Alexander]

Yeah, I've decided that I want a separate "faults" argument, so that the API can behave consistently.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 346810 [details] [diff] [review]
v3 of adding permissive arg to Bug.get

See my comment above.

Comment 20

[Noura Elhawary]

Attachment: v4 for adding permissive argument to Bug.get to return invalid bug ids

attached is a patch that will return separate item for the faults as suggested by Dave, Please review and let me know what you think.

Thanks,
Noura

Comment 21

[Max Kanat-Alexander]

Comment on attachment 360640 [details] [diff] [review]
v4 for adding permissive argument to Bug.get to return invalid bug ids

Redirecting review in the hope that it can be completed before the freeze.

Comment 22

[David Lawrence [:dkl]]

Comment on attachment 360640 [details] [diff] [review]
v4 for adding permissive argument to Bug.get to return invalid bug ids

Patch looks good to me and works as expected.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 360640 [details] [diff] [review]
v4 for adding permissive argument to Bug.get to return invalid bug ids

On checkin, please note that the faults item and the permissive argument are B<UNSTABLE>.

In the future I want a "permissive" utility function that returns the faults of a block of code, to make this easier.

Comment 24

[David Lawrence [:dkl]]

Noura. I went ahead and made the adjustment and checked this in for you.

tip:
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.31; previous revision: 1.30
done

Comment 25

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.