829273 - Certain webservice tests failing due to improper error being thrown for undef or empty bug id values

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[David Lawrence [:dkl]]

webservice_bug_add_attachment.t ........... 32/177 
#   Failed test 'JSON-RPC: Got correct fault for Bug.add_attachment'
#   at lib/QA/RPC.pm line 90.
#     'Invalid parameter passed to Bugzilla::Bug::_init: It must be numeric.'
#         =~
#     'You must enter a valid bug number'
webservice_bug_add_attachment.t ........... 115/177 
#   Failed test 'XML-RPC: Got correct fault for Bug.add_attachment'
#   at lib/QA/RPC.pm line 90.
#     'Invalid parameter passed to Bugzilla::Bug::_init: It must be numeric.'
#         =~
#     'You must enter a valid bug number'
webservice_bug_add_attachment.t ........... 177/177 # Looks like you failed 2 tests of 177.
webservice_bug_add_attachment.t ........... Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/177 subtests 
webservice_bug_add_comment.t .............. 21/141 
#   Failed test 'JSON-RPC: Got correct fault for Bug.add_comment'
#   at lib/QA/RPC.pm line 90.
#     'Invalid parameter passed to Bugzilla::Bug::_init: It must be numeric.'
#         =~
#     'You must enter a valid bug number'
webservice_bug_add_comment.t .............. 73/141

These are due to some recent refactoring in Bugzilla/Bug.pm for caching of bug objects.

Patch coming that fixes this for me.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to handle certain invalid bug id cases in Bug->new and Bug->check (v1)

Comment 2

[Frédéric Buclin]

Comment on attachment 700660 [details] [diff] [review]
Patch to handle certain invalid bug id cases in Bug->new and Bug->check (v1)

>=== modified file 'Bugzilla/Bug.pm'

>     if (!defined $param
>-        || (!ref($param) && $param =~ /\D/)
>-        || (ref($param) && $param->{id} =~ /\D/))
>+        || (!ref($param) && (!$param || $param =~ /\D/))
>+        || (ref($param) && (!$param->{id} || $param->{id} =~ /\D/)))

The right fix is to revert what glob did in bug 811280: =~ /\D/ should be !~ /^\d+$/ to correctly catch empty strings. Else your patch would change the behavior of Bugzilla::Bug->new(0): NotFound would become InvalidBugId, and we shouldn't change the error message at this point. In bug 509734, I will remove this error message entirely, but for now, let's try to not break the current behavior.

 

>=== modified file 'Bugzilla/WebService/Bug.pm'

>-    defined $ids || ThrowCodeError('param_required', { param => 'ids' });
>+    (defined $ids && scalar @$ids) 
>+        || ThrowCodeError('param_required', { param => 'ids' });

I don't see why this change is made in one method only. There are many places in this file where we do this check. We should be consistent.

Comment 3

[David Lawrence [:dkl]]

Attachment: 829273_1.patch

this change allows for the proper error to occur when doing Bugzilla::Bug->new("") (InvalidBugID) and also Bugzilla::Bug->new(0) (NotFound) and allows the webservice tests to pass properly.

dkl

Comment 4

[:glob ✱]

Comment on attachment 8490306 [details] [diff] [review]
829273_1.patch

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

r=glob

Comment 5

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   c07333d..917ede9  master -> master