Last Comment Bug 657561 - Invalid XMLRPC response generated if an optional custom int field is empty
: Invalid XMLRPC response generated if an optional custom int field is empty
Status: RESOLVED FIXED
[4.0 only; AUTOLOAD is gone in 4.2]
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 4.0.1
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: Peter Gyongyosi
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-17 01:38 PDT by Peter Gyongyosi
Modified: 2011-12-26 07:43 PST (History)
2 users (show)
LpSolit: approval4.0+
mkanat: blocking4.0.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
making Bug.pm's AUTOLOAD hande undefs better (433 bytes, patch)
2011-05-18 01:11 PDT, Peter Gyongyosi
LpSolit: review+
Details | Diff | Splinter Review

Description Peter Gyongyosi 2011-05-17 01:38:01 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.16) Gecko/20110323 Ubuntu/10.04 (lucid) Firefox/3.6.16
Build Identifier: 4.0.1

The bug that can be seen is similar to that in bug #477593 but with other fields: when we have some int-type custom fields which are optional, XMLRPC web service generates an empty <int/> tag in the response, which is considered invalid by most clients (including python's xmlrpclib, which I wanted to use) and cause them to whine.

After lots of digging, I managed to find the root of the problem. You see, XMLRPC handles it well if it receives "undef" in such a field and does not include it in the output, however, the underlying layers mess with it badly. The problem is that that at some point, _bug_to_hash() calls something like $bug->the_name_of_my_custom_and_empty_int_field, which goes to the magic AUTOLOAD thing in Bug.pm, which breaks things. The problem is with the following:

return $self->{$attr} if defined $self->{$attr};
[...]
return '';

Now, as $bug->the_name_of_my_custom_and_empty_int_field is explicitly set to "undef", it won't be returned with that value, but instead as an empty string. Which will bubble up to the XMLRPC layer, and cause it to include the invalid empty <int/> tag.

My proposed fix is to change the "if defined" condition to "if exists", which is what I think we want to do here.

I'm aware that this whole AUTOLOAD mess has been cleaned up in bug #600123, which is in the trunk now and proposed for 4.2, however, I don't have the resources now to test it with that code -- my change fixed the problem for me in a 4.0.1 and caused no other problems. So feel free to either include this fix in the 4.0 branch for a future 4.0.2 maintenance release (if you do those) or set this to a dup of #600123 after testing that that change also fixed this -- honestly, I'm a bit terrified of Perl so I couldn't figure it out only based on the code :)

Reproducible: Always

Steps to Reproduce:
1. create an int-type optional custom field
2. create a bug where it is not filled in
3. fetch this bug through the XMLRPC API

Actual Results:  
The response includes an empty <int/> tag for that custom field, which is considered invalid by most XMLRPC clients.

Expected Results:  
The whole custom field should be omitted from the results if it's blank, just as it happens for built-in fields, like dup_id.
Comment 1 Max Kanat-Alexander 2011-05-17 11:29:58 PDT
Wow, what an interesting bug, thank you for the report! Your fix sounds like a pretty good idea, do you think you could provide a patch? If not, we can just do it ourselves.

I'm slightly worried about changing anything regarding the AUTOLOAD on a stable branch, though. It's caused strange regressions before. Particularly if we start returning "undef", we might start generating a lot of warnings. So we may not be able to fix the problem that way on the 4.0.x branch.
Comment 2 Peter Gyongyosi 2011-05-18 01:09:49 PDT
(In reply to comment #1)
> Wow, what an interesting bug, thank you for the report! Your fix sounds like
> a pretty good idea, do you think you could provide a patch? If not, we can
> just do it ourselves.

Certeanly, I'll attach a patch against the 4.0 head in bzr in a minute. Consider it having a Signed-off-by: Peter Gyongyosi or whatever licensing thingie you need :)


> I'm slightly worried about changing anything regarding the AUTOLOAD on a
> stable branch, though. It's caused strange regressions before. Particularly
> if we start returning "undef", we might start generating a lot of warnings.
> So we may not be able to fix the problem that way on the 4.0.x branch.

You're indeed right about worrying about this. My first intuition was to change the default-fallback of returning an empty string to return undef, but that, while fixing the XMLRPC interface, triggered errors in the Web UI. This patch, however, seems to work properly, I haven't seen any warnings ever since I made the change.
Comment 3 Peter Gyongyosi 2011-05-18 01:11:17 PDT
Created attachment 533200 [details] [diff] [review]
making Bug.pm's AUTOLOAD hande undefs better
Comment 4 Max Kanat-Alexander 2011-05-18 21:54:48 PDT
Comment on attachment 533200 [details] [diff] [review]
making Bug.pm's AUTOLOAD hande undefs better

This looks good to me, but LpSolit, do you think this will cause regressions on the 4.0 branch?
Comment 5 Frédéric Buclin 2011-05-29 08:14:06 PDT
(In reply to comment #4)
> This looks good to me, but LpSolit, do you think this will cause regressions
> on the 4.0 branch?

Hard to say without some more testing (won't happen today), but from what I can see in AUTOLOAD, if $self->{$attr} is undefined and $attr doesn't point to a custom multi-select field, we return an empty string. With this patch, we would suddenly return undef instead. I guess there are some places where the caller doesn't expect to get an undefined value and could either throw warnings or die (for instance, trick_taint() explicitly dies if it gets undef). This makes me nervous.

Also, this would need to be tested against 4.0.2pre as it may interact with bug 653263.
Comment 6 Frédéric Buclin 2011-06-24 15:50:44 PDT
With this patch applied, webservice_bug_search.t throws:

"Use of uninitialized value in string eq at webservice_bug_search.t line 147."

Line 147 is:

            ok(!grep($_->{alias} eq $private_bug->{alias}, @$bugs),
               'Result does not contain the private bug');

mkanat: as far as I can tell, the alias can be undefined, as NULL is a valid value in the DB. So this test script should be fixed. I saw no other errors due to this patch.
Comment 7 Frédéric Buclin 2011-07-05 19:04:57 PDT
Comment on attachment 533200 [details] [diff] [review]
making Bug.pm's AUTOLOAD hande undefs better

I think this change makes sense, and the QA script mentioned above will need to be fixed. r=LpSolit
Comment 8 Frédéric Buclin 2011-07-05 19:08:29 PDT
We need to relnote this for devs using our WebServices. E.g. alias is not undefined rather than empty, as mentioned in comment 6. Also, bug ID custom fields are also undefined rather than empty when not set.
Comment 9 Frédéric Buclin 2011-07-05 19:10:06 PDT
(In reply to comment #8)
> We need to relnote this for devs using our WebServices. E.g. alias is not
> undefined rather than empty

I meant "... alias is now* undefined ..."
Comment 10 Peter Gyongyosi 2011-07-06 00:28:58 PDT
Thanks for reviewing and merging this. Frédéric, in comment #8, you've assigned this bug to me. Is this only for administrative reasons or I am expected to do something?
Comment 11 Frédéric Buclin 2011-07-06 02:46:58 PDT
(In reply to comment #10)
> assigned this bug to me. Is this only for administrative reasons or I am
> expected to do something?

Only for administrative reasons (so that we can easily know who fixed the bug). I will commit your patch in the source code later today. :)
Comment 12 Frédéric Buclin 2011-07-06 02:59:54 PDT
I did some more testing, and with this patch, Bugzilla 4.0.2 now behaves exactly the same way as Bugzilla 4.2, i.e. we return undef rather than an empty string when a field is undefined.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Bug.pm
Committed revision 7614.
Comment 13 Frédéric Buclin 2011-07-06 03:41:07 PDT
For the record, I also fixed webservice_bug_search.t:

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
modified t/webservice_bug_search.t
Committed revision 191.
Comment 14 Max Kanat-Alexander 2011-07-19 15:02:37 PDT
I'm sorry I didn't comment on this earlier, but it's not okay to change the API of a stable WebServices function that way on a stable branch. There could be implementations which depend on receiving that hash element and may fail when it is suddenly not present at all. (This is more likely in strictly-typed languages but could happen anywhere.)

In fact, if this behavior changed in 4.2, it should be added to History. You can keep this patch, but don't break the WebServices API.
Comment 15 Max Kanat-Alexander 2011-07-19 15:04:11 PDT
Actually, thinking about this further, I agree that it's better to generate valid XML-RPC than to retain a stable API for a very minor thing. So we will simply relnote it as LpSolit mentioned.
Comment 16 Frédéric Buclin 2011-12-26 07:43:54 PST
Already added to relnotes for 4.0.2.

Note You need to log in before you can comment on or make changes to this bug.