Closed Bug 955325 Opened 10 years ago Closed 10 years ago

Update Twitter to API v1.1

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

(Whiteboard: [1.4-wanted])

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1890 at 2013-03-08 21:58:00 UTC ***

Starting March 5, 2013 [1], Twitter has started testing disabling of API v1. We should update to v1.1 [2]. For the most part this should be a s/1/1.1/, but somethings, like the search API, have changed a bit; mostly for the better.

[1] https://dev.twitter.com/blog/planning-for-api-v1-retirement
[2] https://dev.twitter.com/docs/api/1.1/overview
Whiteboard: [1.4-wanted]
Attached patch Twitter v1.1 patch (obsolete) — Splinter Review
*** Original post on bio 1890 as attmnt 2256 at 2013-03-09 01:59:00 UTC ***

This seems to work expect for when I send a tweet with an @ or !, e.g.:

What gets sent is:
Warning: sending request to https://api.twitter.com/1.1/statuses/update.json (POSTData = status=Test%20sending%20message%20from%20%40instantbird%20with%20Twitter%20API%20v1.1!)

And I get an error:
Error: 401 - {"errors":[{"message":"Could not authenticate you","code":32}]}

I'm at a loss about what could cause this. :(

Changes in this patch:
 - s/1/1.1/
 - Stop sending include_entities=1, this is now the default.
 - Switch to using an authenticated search.
 - Requests 100 tweets when requesting a search.
 - Switch userstream to 1.1 instead of 2 (why was this 2? :-S)
 - Switched to user id_str wherever possible (it seems that the user stream still sends integers though.
Attachment #8354021 - Flags: feedback?
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 1890 at 2013-03-09 12:48:22 UTC ***

Some initial comments

- It seems like it should be possible to remove the special-case code from searchResultsReceived, but maybe it's safer to keep it for now. At any rate, the bug reference in http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#608 should be to bug 954486 (bio 1052), so that should be fixed if the code is kept.

- The strange error above is a nice test case for the parseError method and shows that there is a bug.
At lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#200,
  error = data.errors.split("\n")[0];
should be
  error = data.errors[0].message;
as far as I can tell from https://dev.twitter.com/docs/error-codes-responses. It's not clear from that page if there have been any recent changes there.
Comment on attachment 8354021 [details] [diff] [review]
Twitter v1.1 patch

*** Original change on bio 1890 attmnt 2256 at 2013-03-09 12:58:33 UTC ***

The error you found is discussed here: https://dev.twitter.com/discussions/12378

"'!' needs to be encoded as '%21' in the POST request body (application/x-www-form-urlencoded) with v1.1 ... '!()*+ all need to be escaped"

I could not reproduce any problems with @, but it's certainly listed here https://tools.ietf.org/html/rfc3986#section-2.2, so I probably just didn't find the right example.
Attachment #8354021 - Flags: feedback? → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1890 as attmnt 2259 at 2013-03-09 15:13:00 UTC ***

(In reply to comment #2)
> - It seems like it should be possible to remove the special-case code from
> searchResultsReceived, but maybe it's safer to keep it for now. At any rate,
> the bug reference in
> http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#608
I removed this entire function, if we don't need to do it anymore...we shouldn't do it.

> - The strange error above is a nice test case for the parseError method and
> shows that there is a bug.
> At
> lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#200,
>   error = data.errors.split("\n")[0];
> should be
>   error = data.errors[0].message;
I made this change, thanks!

(In reply to comment #3)
> Comment on attachment 8354021 [details] [diff] [review] (bio-attmnt 2256) [details]
> The error you found is discussed here:
> https://dev.twitter.com/discussions/12378
> 
> "'!' needs to be encoded as '%21' in the POST request body
> (application/x-www-form-urlencoded) with v1.1 ... '!()*+ all need to be
> escaped"
aleth provided a patch to http.jsm that fixes this. I'm unsure if it's the proper thing to do, but it seems to work. We'll need someone (mconley?) to look at this to ensure it doesn't break FileLink.
Attachment #8354024 - Flags: review?(florian)
Comment on attachment 8354021 [details] [diff] [review]
Twitter v1.1 patch

*** Original change on bio 1890 attmnt 2256 at 2013-03-09 15:13:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354021 - Attachment is obsolete: true
Comment on attachment 8354024 [details] [diff] [review]
Patch v2

*** Original change on bio 1890 attmnt 2259 at 2013-03-09 15:25:52 UTC ***

This would be r+ if not for the way percentEncode is duplicated across two files here.

I couldn't find the correct and complete RFC for HTTP POST data (https://en.wikipedia.org/wiki/Percent-encoding#The_application.2Fx-www-form-urlencoded_type). We should only make the change to http.jsm if it's the right change also outside the Oauth context. Twitter seems to claim requiring the percent encoding here is the correct thing to do independently of Oauth ("Our OAuth and HTTP standards are a bit stricter in API v1.1"). The error seen above arises because of the mismatch between the POSTData and the Oauth'ed data.

I'd also suggest considering:
- replacing encodeURIComponent with percentEncode throughout twitter.js for consistency.
- If we do end up adding percentEncode to http.jsm, it can be exported and removed from twitter.js.
Attachment #8354024 - Flags: review-
*** Original post on bio 1890 at 2013-03-09 15:29:31 UTC ***

(Afaik, the argument for the twitter interpretation of POST would be that www-form-urlencoded suggests using RFC3986 as linked above.)
*** Original post on bio 1890 at 2013-03-11 12:21:56 UTC ***

(In reply to comment #1)

>  - Switch userstream to 1.1 instead of 2 (why was this 2? :-S)

Because it's what's documented at https://dev.twitter.com/docs/api/2/get/user

The version number is apparently '2' for the v1 version, and '1.1' for the v1.1 version (https://dev.twitter.com/docs/api/1.1/get/user). Great job twitter!
Comment on attachment 8354024 [details] [diff] [review]
Patch v2

*** Original change on bio 1890 attmnt 2259 at 2013-03-15 21:53:30 UTC ***

We discussed this character encoding issue on IRC, here is what I said:

"Space characters are replaced by `+', and then reserved characters are escaped as described in [RFC1738], section 2.2: Non-alphanumeric characters are replaced by `%HH', a percent sign and two hexadecimal digits representing the ASCII code of the character." http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.1

http://www.ietf.org/rfc/rfc1738.txt defines the reserved characters as:
reserved       = ";" | "/" | "?" | ":" | "@" | "&" | "="


Here's a suggestion: ignore the specs that are all confusing, and instead send through Firefox in a POST form the same data to a web page. Look with wireshark how it's encoded, and do the same for http.jsm. If that doesn't make twitter work, then we need twitter specific hacks in twitter.js.
Attachment #8354024 - Flags: review?(florian) → review-
Attached patch Patch v3Splinter Review
*** Original post on bio 1890 as attmnt 2286 at 2013-03-16 18:17:00 UTC ***

This takes the function from [1] (along with [2] this seems to provide a description) and exports it form http.jsm.

This does not replace encodeURIComponent everywhere (as aleth suggested somewhere), because I'm unsure if that's appropriate...and it was unnecessary for me to get things to work.

[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/encodeURIComponent#Description
[2] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/encodeURI#Description
Attachment #8354051 - Flags: review?(florian)
Comment on attachment 8354024 [details] [diff] [review]
Patch v2

*** Original change on bio 1890 attmnt 2259 at 2013-03-16 18:17:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354024 - Attachment is obsolete: true
Comment on attachment 8354051 [details] [diff] [review]
Patch v3

*** Original change on bio 1890 attmnt 2286 at 2013-03-17 16:15:57 UTC ***

I haven't tested this again, but it looks good to me.

I wanted to add a f?=mconley for the http.jsm changes, but he doesn't have editbugs on BIO.
Attachment #8354051 - Flags: review+
Comment on attachment 8354051 [details] [diff] [review]
Patch v3

*** Original change on bio 1890 attmnt 2286 at 2013-03-17 16:20:42 UTC ***

(In reply to comment #10)
> Comment on attachment 8354051 [details] [diff] [review] (bio-attmnt 2286) [details]
> Patch v3
> I wanted to add a f?=mconley for the http.jsm changes, but he doesn't have
> editbugs on BIO.

He does now. ;)

mconley, can you just take a look at the changes to http.jsm and see if they seem like they'd be compatible w/ FileLink.
Attachment #8354051 - Flags: feedback?(bugzilla)
Comment on attachment 8354051 [details] [diff] [review]
Patch v3

*** Original change on bio 1890 attmnt 2286 at 2013-03-19 23:01:12 UTC ***

Looks good to me, r=me assuming:
- mconley confirms the changes to http.jsm don't break FileLink
- someone has tested sending a tweet containing all the potentially annoying characters with the patch applied. Or even better, all the printable ascii characters!
Attachment #8354051 - Flags: review?(florian) → review+
*** Original post on bio 1890 at 2013-03-19 23:07:27 UTC ***

(In reply to comment #12)
> - someone has tested sending a tweet containing all the potentially annoying
> characters with the patch applied. Or even better, all the printable ascii
> characters!

https://twitter.com/clokep/status/314151085424144385
*** Original post on bio 1890 at 2013-03-19 23:10:50 UTC ***

(In reply to comment #13)
> (In reply to comment #12)
> > - someone has tested sending a tweet containing all the potentially annoying
> > characters with the patch applied. Or even better, all the printable ascii
> > characters!
> 
> https://twitter.com/clokep/status/314151085424144385

Generated from:

> let x = "";
> for (let i = 0x20; i < 0x7E; ++i)
>   x += String.fromCharCode(i);
>   
> print(x)
Comment on attachment 8354051 [details] [diff] [review]
Patch v3

*** Original change on bio 1890 attmnt 2286 by mconley AT mozilla.com at 2013-03-21 19:00:48 UTC ***

I had a chance to try the changes to http.jsm under mail/base/modules.

The Filelink tests all pass, and manual testing didn't show anything unusual, so I think it's good to go. :)
Attachment #8354051 - Flags: feedback?(bugzilla) → feedback+
*** Original post on bio 1890 at 2013-03-21 19:12:15 UTC ***

Thanks for looking at this Mike!
Whiteboard: [1.4-wanted] → [1.4-wanted][checkin-needed]
*** Original post on bio 1890 at 2013-03-21 22:32:56 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/743ce473ff0a

We might want to get this into Thunderbird soon (and maybe backport to Aurora, which I think is the next ESR?)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.4-wanted][checkin-needed] → [1.4-wanted]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.