477593 - Bug._bug_to_hash generates invalid XMLRPC when dup_id is blank

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Rosie]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020410 Fedora/3.0.6-1.fc9 Firefox/3.0.6
Build Identifier: 

If bugs are not marked as duplicates dup_id is returned as an int with value ''. This is invalid XMLRPC and can break client programs.

Reproducible: Always

Comment 1

[Rosie]

Attachment: v1

Checks for blank dup_ids and does not return them in the hash

Comment 2

[Max Kanat-Alexander]

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

If you read the Bugzilla::WebService documentation, you'll see that we allow empty ints and consider them undef. However, this doesn't mean that we should send empty ints. Perhaps we could send <nil>.

Comment 3

[Rosie]

Attachment: v2

This sets item{'dupe_of'} to undef (which i assume sends nil) if dup_id isn't set - it's the same way that aliases are handled below

Comment 4

[Max Kanat-Alexander]

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

>+# Portions © Crown copyright 2009 - Rosie Clarkson ([e-mail]) for the Planning Portal

  I would really rather not have that in the code itself. I think it's fine to have it at the top of the patch--that preserves the fact that this came to us under original copyright from the Crown.

>@@ -440,7 +443,14 @@
>     $item{'priority'}         = $self->type('string', $bug->priority);
>     $item{'product'}          = $self->type('string', $bug->product);
>     $item{'component'}        = $self->type('string', $bug->component);
>-    $item{'dupe_of'}          = $self->type('int', $bug->dup_id);

>+    #passing an int with value '' is invalid so set to undef    

  Nit: Trailing spaces, and this should be a complete sentence that starts with a capital letter and ends with a period, ideally. :-)

>+    if($bug->dup_id ne ''){ 

  You should just check "if ($bug->dup_id)" (also note the spacing).

Comment 5

[Chris Eveleigh]

Hi Max,

in _this_ case we can remove the copyright but that's only because it's such a minor change.  we will still have to include it in the larger patches.

Comment 6

[Rosie]

Attachment: v3

Thanks for the review here's the corrected version

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> in _this_ case we can remove the copyright but that's only because it's such a
> minor change.  we will still have to include it in the larger patches.

  Hey Chris. You can keep it in the patch itself, which I'm pretty sure should be sufficient, since the license you grant seems to give us a license to re-license anything any way we want, and thus we can just say that we (the committers) re-licensed it to the MPL on commit, right?

Comment 8

[Max Kanat-Alexander]

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

Looks good to me. :-)

Comment 9

[Max Kanat-Alexander]

I think you may have to make another patch for 3.2 just to overcome conflicts.

Comment 10

[Chris Eveleigh]

(In reply to comment #7)
>   Hey Chris. You can keep it in the patch itself, which I'm pretty sure should
> be sufficient, since the license you grant seems to give us a license to
> re-license anything any way we want, and thus we can just say that we (the
> committers) re-licensed it to the MPL on commit, right?

Hi Max.  in theory, it would be sufficient not to include any copyright, source or re-use statement in both the patch and the patched file - it's up to the potential re-licenser to find out who the copyright holders are and get the required permissions.

however, i only have permission to contribute at all subject to an agreement i've reached with the Crown.  that includes some additional requirements as specified in our Information Asset Register (https://support.planningportal.gov.uk/wiki/index.php/Information_Asset_Register)

i'll certainly go back and see if we can adjust the agreement at some point in the future but _for now_ if the change is big enough to warrant us asserting our copyright then i have to follow that procedure.  i'm already bending it a bit to remove the re-use statement from the patched file.

in _this_ case i don't see the need for us to assert copyright over a single 'if'.  so, for consistency, both the patch and the patched file have no copyright or re-use statement.

hope that makes some sense!

and thanks for the reviews - we'll make a 3.2 (presumably 3.2.2?) patch tomorrow.  :-)

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> however, i only have permission to contribute at all subject to an agreement
> i've reached with the Crown.  that includes some additional requirements as
> specified in our Information Asset Register
> (https://support.planningportal.gov.uk/wiki/index.php/Information_Asset_Register)

  Okay. But you could contribute it and I could remove the copyright notice on checkin, couldn't I? :-)

  In any case, you'd probably do best to contact [e-mail] about all of this.

Comment 12

[Rosie]

Hi Max thanks for the quick review - I don't see where you would patch in 3.2 though - dup_id isn't returned anywhere.

Comment 13

[Max Kanat-Alexander]

Oh, I thought dup_id dated back to 3.2. Okay, then.

Comment 14

[Frédéric Buclin]

So removing approval3.2+.

Comment 15

[Max Kanat-Alexander]

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

Actually, I just thought about this some more, and on the trunk what we should be doing is fixing $self->type to do the right thing. We shouldn't be putting a band-aid on it here.

Comment 16

[Rosie]

Attachment: v4 - fixes problem in XMLRPC.pm

Rather than fixing the problem in type I've gone even further back and fixed it in Bugzilla::WebService::Server::XMLRPC::Serializer. There's fairly similar code in Deserializer where ints, doubles and dates are converted to undef on the way in. I've just copied the functions from the perl module but have added checks for undef values to return nil.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 364301 [details] [diff] [review]
v4 - fixes problem in XMLRPC.pm

>+            my($self, $value) = @_;
>+            if (!$value){
>+                return [ 'nil', {}, $value ];

  No, that's wrong for all integer types, because "0" is also false.

Comment 19

[Rosie]

Attachment: v5 - checks for undefined number values in XMLRPC.pm

Oops yes this one checks for undefined values instead... and should be a little less hacky than the last version too!

Comment 20

[Max Kanat-Alexander]

Comment on attachment 374048 [details] [diff] [review]
v5 - checks for undefined number values in XMLRPC.pm

>+# Here the XMLRPC::Serializer is extended to use the XMLRPC nil extension.
>+sub encode_object {
>+    my $self = shift;
>+    my @encoded = $self->SUPER::encode_object(@_);
>+
>+    return $encoded[0]->[0] =~ /^nil$/o

  Why don't you just use "eq"?

>+        ? ['value', {}, [@encoded]]

  I don't understand what you're doing here--why are you returning a 'value' object when you get a 'nil' object to encode?

>+sub BEGIN {
>+    no strict 'refs';
>+    for my $type (qw(double i4 int dateTime)) {
>+        my $method = 'as_' . $type;
>+        *$method = sub {
>+            my ($self, $value) = @_;
>+            if (!defined($value)) {
>+                return as_nil();
>+            }
>+            else {

>+                return eval('$self->SUPER::' . $method . '($value)');

  Why are you wrapping that in a string eval? You can actually call methods like: $self->$some_string($value) in Perl.

>+sub as_nil {
>+    return ['nil', {}];
>+}

  XMLRPC::Lite doesn't already have that?

>-nil is implemented by XMLRPC::Lite, in XMLRPC::Deserializer::decode_value.
>+nil is implemented by XMLRPC::Lite, in XMLRPC::Deserializer::decode_value in the 
>+CPAN SVN since 14th Dec 2008 (http://rt.cpan.org/Public/Bug/Display.html?id=20569)
>+and in fedora's perl-SOAP-Lite package in versions 0.68-1 and above.

  You don't need to note that, because checksetup.pl already checks that we have the appropriate version of SOAP::Lite.

Comment 21

[Rosie]

Attachment: v6

Thanks again for the quick review!

> >+        ? ['value', {}, [@encoded]]
> 
>   I don't understand what you're doing here--why are you returning a 'value'
> object when you get a 'nil' object to encode?

it's nesting the <nil/> in a <value> - we think it's required by the spec

> >+sub as_nil {
> >+    return ['nil', {}];
> >+}
> 
>   XMLRPC::Lite doesn't already have that?

No - not that we can find anyway!

> >-nil is implemented by XMLRPC::Lite, in XMLRPC::Deserializer::decode_value.
> >+nil is implemented by XMLRPC::Lite, in XMLRPC::Deserializer::decode_value in the 
> >+CPAN SVN since 14th Dec 2008 (http://rt.cpan.org/Public/Bug/Display.html?id=20569)
> >+and in fedora's perl-SOAP-Lite package in versions 0.68-1 and above.
> 
>   You don't need to note that, because checksetup.pl already checks that we
> have the appropriate version of SOAP::Lite.

checksetup appears to blacklist "versions (0.70 -> 0.710.05)".  So it's certainly worth noting (somewhere) that the patch for the deserializer isn't in _any_ released version - not even the latest 0.710.08 - but went into SVN on the noted date.  but also note that the patch _is_ in some versions of the fedora package.  who knows if the patch is in other linux distributions' packages?

Comment 23

[Max Kanat-Alexander]

(In reply to comment #21)
> it's nesting the <nil/> in a <value> - we think it's required by the spec

  But won't that create things like <value><value<nil/></value></value>? I should test it, I guess??

> checksetup appears to blacklist "versions (0.70 -> 0.710.05)".  So it's
> certainly worth noting (somewhere) that the patch for the deserializer isn't in
> _any_ released version - not even the latest 0.710.08 - but went into SVN on
> the noted date.  but also note that the patch _is_ in some versions of the
> fedora package.  who knows if the patch is in other linux distributions'
> packages?

  Wait, what? 0.68 and above are absolutely released. Are you telling me that this code requires some unreleased version of SOAP::Lite in order to work? Just on the client side, or what? Have you looked into how many common language clients support <nil>? Most people are writing clients in Python or Java, I'd look at those first. However, we aren't writing for some specific client, so we don't have to put in any comments that have to do with what version of a client you should be using--particularly as *client* users aren't going to be reading through our source code to know what client version to use.

  However, if you're saying that the *server* needs some patch to SOAP::Lite that was never released in order to function, then that won't work.

Comment 24

[Chris Eveleigh]

(In reply to comment #23)
>   But won't that create things like <value><value<nil/></value></value>? I
> should test it, I guess??
> 

:-)

have a look at the encode_object sub here: http://cpansearch.perl.org/src/MKUTTER/SOAP-Lite-0.710.08/lib/XMLRPC/Lite.pm

it takes things from SOAP and adds <value> tags around XMLRPC things that require it.  we're just extending that function to also add <value> tags around <nil />.

>   Wait, what? 0.68 and above are absolutely released. Are you telling me that

no, sorry - i probably should have raised this as a separate bug.  it's just something we found out whilst working on this bug.

the situation with the 'nil' in XMLRPC::Lite seems to be a bit messy - and certainly more complicated than is implied by that note.

it would appear that someone patched XMLRPC::Lite on fedora to add support for nil.  however, they only added it to the deserializer half so that a server would understand nils sent from a client.

that patch was only added to XMLRPC::Lite SVN recently.  So yes, 0.68 and above are released but they don't contain the deserializer patch - it's only in SVN or on fedora (or other similarly patched package).

this doesn't affect bug_to_hash etc - it needs nil support in the _serializer_ only - which is what our patch adds to bugzilla.

do you want me to split that out into a separate documentation bug?  more than happy to do so - particularly if getting the wording right in the note means delaying the actual code patch.

Comment 25

[Max Kanat-Alexander]

(In reply to comment #24)
> (In reply to comment #23)
> have a look at the encode_object sub here:
> http://cpansearch.perl.org/src/MKUTTER/SOAP-Lite-0.710.08/lib/XMLRPC/Lite.pm

  Ah, thank you! :-)

> do you want me to split that out into a separate documentation bug?  more than
> happy to do so - particularly if getting the wording right in the note means
> delaying the actual code patch.

  No, actually, since you wrapped it in private, it's OK.

Comment 26

[Max Kanat-Alexander]

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

Okay, with all those explanations, this looks good! :-) Thank you!

Comment 27

[Max Kanat-Alexander]

It needed some unrotting for HEAD and a slight porting for 3.4 (no code changes, though). I'll attach the final patches here.

tip:

Checking in Bugzilla/WebService/Server/XMLRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/XMLRPC.pm,v  <--  XMLRPC.pm
new revision: 1.4; previous revision: 1.3
done

3.4:

Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.17.2.1; previous revision: 1.17
done
Checking in Bugzilla/WebService/Server/XMLRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/XMLRPC.pm,v  <--  XMLRPC.pm
new revision: 1.1.2.1; previous revision: 1.1
done

Comment 28

[Max Kanat-Alexander]

Attachment: Checked-In Version: HEAD

Comment 29

[Max Kanat-Alexander]

Attachment: Checked-In Version: 3.4