1105433 - Bug.update_attachment fails if you try to update a pending request while marking the attachment as obsolete

Status: NEW

(Bugzilla :: WebService, defect)

Description

[Ed Morley [:emorley]]

bzexport is hitting this issue after switching from the legacy bzapi endpoint to the bzapi compatibility layer...

If you attempt to update an attachment, when the attachment has a pending review, and the update is to make the attachment obsolete, we get bzexport errors like:

Error: There is no flag with the id '868076'.
abort: Could not update attachment 8418132 [details]: HTTP Error 404: Not Found

Testcase coming up.

Comment 1

[Ed Morley [:emorley]]

Attachment: POC

$ ./bzapi-attachment-poc.py

Fetching attachment...

Making request to:
https://bugzilla-dev.allizom.org/bzapi/attachment/8418132?username=[e-mail]&password=REMOVED

Server: Apache
X-Backend-Server: web5.stage.bugs.scl3.mozilla.com
Vary: Accept-Encoding
Content-Type: application/json; charset=UTF-8
Strict-transport-security: max-age=31536000; includeSubDomains
Date: Wed, 26 Nov 2014 19:16:51 GMT
Keep-Alive: timeout=5, max=1000
X-xss-protection: 1; mode=block
Transfer-Encoding: chunked
Access-control-allow-origin: *
Etag: mAnTcTopPOMU6wHMYz5kkg
X-content-type-options: nosniff
Connection: close
X-frame-options: SAMEORIGIN
Access-control-allow-headers: origin, content-type, accept

{"attacher": {"name": "[e-mail]", "real_name": "Ed Morley", "ref": "https://bugzilla-dev.allizom.org/bzapi/user/[e-mail]"}, "bug_id": 1006956, "bug_ref": "https://bugzilla-dev.allizom.org/bzapi/bug/1006956", "content_type": "text/plain", "creation_time": "2014-11-26T18:11:14Z", "description": "Foo", "file_name": "file_1006956.txt", "flags": [{"creation_date": "2014-11-26T18:11:14Z", "id": 868076, "modification_date": "2014-11-26T18:11:14Z", "name": "review", "requestee": {"name": "[e-mail]", "ref": "https://bugzilla-dev.allizom.org/bzapi/user/[e-mail]"}, "setter": {"name": "[e-mail]", "ref": "https://bugzilla-dev.allizom.org/bzapi/user/[e-mail]"}, "status": "?", "type_id": 4}], "id": 8418132, "is_obsolete": false, "is_patch": true, "is_private": false, "last_change_time": "2014-11-26T19:05:02Z", "ref": "https://bugzilla-dev.allizom.org/bzapi/attachment/8418132", "size": 7, "update_token": "1417029411-aadfe4917fdedadf5259b71fa3361fef"}


Updating attachment...

Making request to:
https://bugzilla-dev.allizom.org/bzapi/attachment/8418132?username=[e-mail]&password=REMOVED

Server: Apache
X-Backend-Server: web1.stage.bugs.scl3.mozilla.com
Vary: Accept-Encoding
Content-Type: application/json; charset=UTF-8
Strict-transport-security: max-age=31536000; includeSubDomains
Date: Wed, 26 Nov 2014 19:16:51 GMT
Keep-Alive: timeout=5, max=1000
X-xss-protection: 1; mode=block
Transfer-Encoding: chunked
Access-control-allow-origin: *
Etag: dSXrDagHwlcPG+xJpeTCfg
X-content-type-options: nosniff
Connection: close
Set-Cookie: Bugzilla_login=373476; path=/; secure; HttpOnly
Set-Cookie: Bugzilla_logincookie=dmmLBQifZE; path=/; secure; HttpOnly
X-frame-options: SAMEORIGIN
Access-control-allow-headers: origin, content-type, accept

HTTP Error 404: Not Found
{"documentation":"http://www.bugzilla.org/docs/tip/en/html/api/","error":true,"code":51,"message":"There is no flag with the id '868076'."}

==

Note: If instead of obsoleting the attachment, we change the description, the request succeeds.

Comment 2

[David Lawrence [:dkl]]

(In reply to Ed Morley (moved to Treeherder team) [:edmorley] from comment #1) 
> HTTP Error 404: Not Found
> {"documentation":"http://www.bugzilla.org/docs/tip/en/html/api/","error":
> true,"code":51,"message":"There is no flag with the id '868076'."}

Ed, can I get the PUT body you are using when doing the update?

Thanks dkl

Comment 3

[Ed Morley [:emorley]]

It's the same as the json dump in comment 1 from the GET, but with is_obsolete set to true.

Comment 4

[Ed Morley [:emorley]]

If it helps, add a "print(json.dumps(attachment))" at line 30 in the POC.

Comment 5

[Ed Morley [:emorley]]

I've just seen bug 946831 - looks like a dupe of that?

Comment 6

[Ed Morley [:emorley]]

This no longer blocks bug 1033394, since for obsoleting attachments only, bzexport now uses the native REST API instead. (In the future it will use the native API for all interactions, but that will do for now).

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/11abf3861fc4

Comment 8

[Gervase Markham [:gerv]]

edmorley: does that mean that this bug also does not block bug 1098342?

Gerv

Comment 9

[Ed Morley [:emorley]]

(In reply to Gervase Markham [:gerv] from comment #8)
> edmorley: does that mean that this bug also does not block bug 1098342?

This bug is still present, it just no longer blocks bzexport switching, since bzexport now uses the native REST API rather than the bzapi. 

That said, most consumers of bzapi are read-only - and even for the ones that are not, most are just leaving bug comments and not obsoleting attachments - so perhaps fixing bug 1098342 before this one might not break anything.

Comment 10

[David Lawrence [:dkl]]

Ok, so what is happening here internally is Bugzilla is setting the is_obsolete before flags and this will in return remove any flags that are set to '?' from the $attachment->{flags} list. Then when we go to update any flags or set new ones, it sees the review flag that is '?' and tries to match it up. Of course that flag was removed so you get the error. This is a bug in the native webservices and not to do with BzAPI. Moving to upstream.

dkl

Comment 11

[David Lawrence [:dkl]]

Attachment: 1105433_1.patch

Comment 12

[Frédéric Buclin]

Are this bug and bug 946831 the same issue?

Comment 14

[Frédéric Buclin]

I cannot reproduce this issue upstream, neither with 5.0 nor with master.

Comment 15

[Frédéric Buclin]

Comment on attachment 8562903 [details] [diff] [review]
1105433_1.patch

>+++ b/Bugzilla/WebService/Bug.pm

>         $attachment->set_all($params);
>         if ($flags) {
>-            my ($old_flags, $new_flags) = extract_flags($flags, $attachment->bug, $attachment);
>+            my ($old_flags, $new_flags)
>+                = extract_flags($flags, $attachment->bug, $attachment, $was_obsolete);
>             $attachment->set_flags($old_flags, $new_flags);
>         }

Simply call $attachment->set_all($params) after flags have been set. This will give a chance to the external application to set flags before they are dropped (e.g. if you want to r- a pending request and mark the patch as obsolete in one shot).



>+++ b/Bugzilla/WebService/Util.pm

>+            if (!$flag_obj) {
>+                # If attachment was made obsolete and flag is set to '?' then
>+                # skip as it was cleared earlier by attachment_>set_all
>+                next if $flag->{'status'} eq '?'
>+                        && !$was_obsolete && $attachment->isobsolete;
>+                ThrowUserError('object_does_not_exist',
>+                               { class => 'Bugzilla::Flag', id => $id });
>+            }

The problem with this workaround is that you have no idea if the flag ID was a valid one or not. So I much prefer my suggestion above.

Comment 16

[Frédéric Buclin]

Note that marking the attachment as obsolete while it has pending requests is not enough to trigger the error. You must also edit the flag itself at the same time, else things work nicely.