1105122 - bzexport fails with KeyError: 'id' since switching to bmo's bzapi compatibility layer

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

As of this morning, trying to use latest bzexport on Windows I get failures with a stack that ends in:

  File "c:/Users/Brian/version-control-tools/hgext\bzexport\__init__.py", line 1041, in bzexport
    "attachment.cgi?id=" + result["id"] + "&action=edit")
KeyError: 'id'

Others on Linux have confirmed seeing the same behaviour.

At a glance, it seems that create_attachment is now returning an object with a single property, 'attachments' that has an array of attachments. Hence, the 'id' property it is looking for is not found.

This was working yesterday so seems to coincide with bug 1098342 taking effect.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

After working around this particular issue, other issues crop up:

  addReadyPromise2 uploaded as https://bugzilla.mozilla.org/attachment.cgi?id=8528192&action=edit
  Error: HTTP Error 301: Moved Permanently
  abort: Could not update attachment 8528194 [details] [diff] [review]: No JSON object could be decoded

Basically, bzexport is all sorts of fail after bug 1098342.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Workaround for first issue

For reference, here are my hacks to bzexport to work around the first issue.

Comment 3

[Nicholas Nethercote [inactive]]

I just hit this on Linux.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Birtles's patch doesn't actually lead to attachment upload succeeding, though.

It seems like the API described at:
https://wiki.mozilla.org/Bugzilla:BzAPI:Methods#Create_new_attachment_.28.2Fbug.2F.3Cid.3E.2Fattachment_POST.29
is now broken.  It:
 * fails to create the attachment
 * returns a JSON blob representing the list of current attachments on the bug, rather than the new attachment

Comment 6

[Ed Morley [:emorley]]

It seems this issue was mentioned in bug 1033394 comment 7.

Comment 7

[Ed Morley [:emorley]]

(Moving assignee from bug 1104906)

Comment 8

[Ed Morley [:emorley]]

I was able to upload the patch in bug 1033394 whilst using bugzilla.m.o/bzapi/, though given comment 4, perhaps this is just because that bug already had an attachment? (ie if we're just returning a json blob of the current attachments, then we won't get the key error if there was already one)

Comment 9

[:Gijs (he/him)]

(In reply to Ed Morley (moved to Treeherder team) [:edmorley] from comment #8)
> I was able to upload the patch in bug 1033394 whilst using
> bugzilla.m.o/bzapi/, though given comment 4, perhaps this is just because
> that bug already had an attachment? (ie if we're just returning a json blob
> of the current attachments, then we won't get the key error if there was
> already one)

Try marking all the existing ones obsolete and then try again to test that hypothesis? :-)

Comment 10

[Ed Morley [:emorley]]

(In reply to :Gijs Kruitbosch from comment #9)
> Try marking all the existing ones obsolete and then try again to test that
> hypothesis? :-)

I wasn't able to prove this on bugzilla-dev.allizom.org/bzapi/.

In fact, I wasn't able to repro at all using either bugzilla-dev.allizom.org/bzapi/ or bugzilla.mozilla.org/bzapi/.

Do we think that it was perhaps the redirect that is causing issues? ie: if the POST was flipped to a GET (after bug 1036802 I'll believe anything lol) we would get a list of existing attachments, which would seem to explain the rest of the behaviour, and why I cannot repro.

Comment 11

[Ed Morley [:emorley]]

Attachment: bzexport debugging patch

The debugging patch I was using, in case it helps.

Example request (though wasn't able to repro with this):

[/c/src/vctools]$ hg bzexport
saved edited form in .hg/last_bzexport.txt
Requesting review from [e-mail]

Request URL:
https://bugzilla-dev.allizom.org/bzapi/bug/1006954/attachment?username=emorley%40SNIP.com&password=REMOVED

Request headers:
{'Content-Type': 'application/json', 'Accept': 'application/json'}

Request body:
{"description": "Foo", "encoding": "base64", "file_name": "2014-11-26_11-26-10_r1542+.diff", "is_patch": true, "comments": [{"text": "Test"}], "flags": [{"requestee": {"name": "[e-mail]"}, "status": "?", "name": "review", "type_id": 4}], "content_type": "text/plain", "data": "SNIP"}

Response headers:
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 14:22:22 GMT
Keep-Alive: timeout=5, max=1000
X-xss-protection: 1; mode=block
Transfer-Encoding: chunked
Access-control-allow-origin: *
Etag: f+QLO67xq6ynOhjAr0Mz5A
X-content-type-options: nosniff
Connection: close
Set-Cookie: Bugzilla_login=REMOVED; path=/; secure; HttpOnly
Set-Cookie: Bugzilla_logincookie=REMOVED; path=/; secure; HttpOnly
X-frame-options: SAMEORIGIN
Access-control-allow-headers: origin, content-type, accept

Response body:
{"ref": "https://bugzilla-dev.allizom.org/bzapi/attachment/8418124", "id": "8418124"}

2014-11-26_11-26-10_r1542+.diff uploaded as https://bugzilla.mozilla.org/attachment.cgi?id=8418124&action=edit

Comment 12

[Ed Morley [:emorley]]

Note "Request headers" in comment 11 are only what was passed to urllib2.Request() and doesn't include whatever other default headers it adds (should you wish to convert that to cURL).

Comment 13

[Kendall Libby [:fubar] (he/him)]

The redirect traffic script wasn't changing the method, but it does look like it was stripping the query string. :-(

You can still test against the redirect by adding an /etc/host entry that points api-dev.b.m.o to 63.245.215.123.

Comment 14

[Kendall Libby [:fubar] (he/him)]

(In reply to Kendall Libby [:fubar] from comment #13)
> The redirect traffic script wasn't changing the method, but it does look
> like it was stripping the query string. :-(

Or not? Poor documentation is poor. http.getPath strips the query string, but http.setPath says it preserves any existing query string unless the replacement contains a '?'. >.<

Here's the traffic script in question, fwiw:

$path = http.getPath();

if(
   string.startsWith( $path, "/latest/") ||
   string.startsWith( $path, "/1.3/") ||
   string.startsWith( $path, "/1.2/")
   ) {
   $newpath = string.regexsub( $path, "^/(latest|1.2|1.3)/", "/bzapi/" );
   http.setPath( $newpath );
   http.changesite( "https://bugzilla.mozilla.org" );
} else {
      connection.discard();
}

Comment 15

[Ed Morley [:emorley]]

(In reply to Kendall Libby [:fubar] from comment #13)
> You can still test against the redirect by adding an /etc/host entry that
> points api-dev.b.m.o to 63.245.215.123.

Yeah I can now repro when using the redirect.

Comment 16

[David Lawrence [:dkl]]

(In reply to Ed Morley (moved to Treeherder team) [:edmorley] from comment #15)
> (In reply to Kendall Libby [:fubar] from comment #13)
> > You can still test against the redirect by adding an /etc/host entry that
> > points api-dev.b.m.o to 63.245.215.123.
> 
> Yeah I can now repro when using the redirect.

Ok So we have evidence that the redirect itself is munging the request. We can see if we can get a header dump going in and after the redirect to see what is being changed. If in fact the POST is being changed to a GET somehow that would explain what is happening when the client is getting a list of attachments on the bug instead the new attachment id returned.

dkl

Comment 17

[Ed Morley [:emorley]]

Ok so as far as I can tell, this turns out to be due to RFC2616, or more various client's interpretations of it:
http://tools.ietf.org/html/rfc2616.html#section-10.3.2

10.3.2 301 Moved Permanently
...
   If the 301 status code is received in response to a request other
   than GET or HEAD, the user agent MUST NOT automatically redirect the
   request unless it can be confirmed by the user, since this might
   change the conditions under which the request was issued.

      Note: When automatically redirecting a POST request after
      receiving a 301 status code, some existing HTTP/1.0 user agents
      will erroneously change it into a GET request.

This affects Python's urllib2:
https://docs.python.org/2/library/urllib2.html#urllib2.HTTPRedirectHandler.redirect_request
"The default implementation of this method does not strictly follow RFC 2616, which says that 301 and 302 responses to POST requests must not be automatically redirected without confirmation by the user. In reality, browsers do allow automatic redirection of these responses, changing the POST to a GET, and the default implementation reproduces this behavior."

So basically the spec says don't redirect if !(GET or HEAD) (which would help anyway), and browsers, cURL and urllib2 ignore the spec and redirect anyway, but change the request to a GET.

Seems like we either cannot redirect legacy BMO for POST requests, or are going to have add handling on the bzapi comp side to ignore the request type and try to derive it from the content?

Comment 18

[Ed Morley [:emorley]]

s/which would help/which would not help/

Comment 19

[David Lawrence [:dkl]]

Hmm. I would rather not hack the code to look in the request body to determine whether it is GET or POST. 

What if we too twiddle DNS to just make all requests to api-dev.bugzilla.mozilla.org and the use mod_rewrite in .htaccess to rewrite the path?

Example:
RewriteRule ^(latest|1\.2|1\.3)/(.*)$ bzapi/$1 [NE] 

dkl

Comment 20

[:Gijs (he/him)]

(In reply to David Lawrence [:dkl] from comment #19)
> Hmm. I would rather not hack the code to look in the request body to
> determine whether it is GET or POST. 
> 
> What if we too twiddle DNS to just make all requests to
> api-dev.bugzilla.mozilla.org and the use mod_rewrite in .htaccess to rewrite
> the path?
> 
> Example:
> RewriteRule ^(latest|1\.2|1\.3)/(.*)$ bzapi/$1 [NE] 
> 
> dkl

This is one option - or we could make the redirect script to do actual proxying of the request (which of course could lead to serious issues if there was a problem with how it did that or what requests it made or how our firewall setup there works etc.).

Comment 21

[Ed Morley [:emorley]]

Attachment: Foo

Comment 22

[Ed Morley [:emorley]]

Comment on attachment 8529126 [details] [diff] [review]
Foo

Sorry used the wrong URL, didn't mean for this to attach.

Comment 23

[Ed Morley [:emorley]]

(In reply to David Lawrence [:dkl] from comment #19)
> Hmm. I would rather not hack the code to look in the request body to
> determine whether it is GET or POST. 
> 
> What if we too twiddle DNS to just make all requests to
> api-dev.bugzilla.mozilla.org and the use mod_rewrite in .htaccess to rewrite
> the path?
> 
> Example:
> RewriteRule ^(latest|1\.2|1\.3)/(.*)$ bzapi/$1 [NE] 

Ah of course this is much simpler.

Either way, I think we can close this bug out now - discussions about a non-301-redirect solution can occur in bug 1098342. I'd suggest we switch bzexport over to the new endpoint regardless, since it will reduce the risk of bug 1098342 when it finally lands - but there's already bug 1033394 filed for that (and it looks like there may be another bzapi compatibility layer bug lurking - bug 1033394 comment 7).