New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust accounts-twitter package to use id_str instead of id for Twitter users #629

Closed
timhaines opened this Issue Jan 23, 2013 · 7 comments

Comments

Projects
None yet
2 participants
@timhaines
Contributor

timhaines commented Jan 23, 2013

Twitter's moving to 64-bit Twitter User IDs this year. They strongly recommend using the id_str property for user ids in Javascript. https://dev.twitter.com/blog/64-bit-twitter-user-idpocalypse

If it's agreed this is a sensible change, I can submit a pull request to change the use of the Twitter User id field to id_str. I'm not quite sure how to handle the upgrade process for people who are already using the id field in production though. Should something be provided to help people migrate (if so, what?), or would it be fair to treat this as a breaking change?

@n1mmy

This comment has been minimized.

Member

n1mmy commented Jan 23, 2013

Ooh, good catch @timhaines. Thanks for volunteering!

We should definitely start using id_str. I'm not sure the exact right way to do this. I think the least confusing thing for meteor users would be to continue to use services.twitter.id – matching all the other auth services – but to have its value being the string version from twitter's id_str. Having the twitter package use services.twitter.id_str or populate both id and id_str just pushes the burden onto every meteor developer and will have people writing incorrect code.

I'm not sure how to handle migrations, either. Do twitter's APIs accept either integer or string ids, or do you have to specify id_str in queries to twitter? What would break if some users' id was a number and some was a string?

@timhaines

This comment has been minimized.

Contributor

timhaines commented Jan 23, 2013

I agree it should keep using the services.twitter.id field, but switch to using the field for Twitter's id_str.

None of Twitter's APIs take an id_str parameter - the encoding in the url or post parameters for a number or string representation of a number is the same. (right?)

I think the thing we need to consider for people upgrading is how they match logins to existing users. twitter_server.js specifies Twitter's serviceData. If we switch the id from a number to a string there, its going to create the problem of a returning user logging in not being matched to their existing user account (because the selector created in Accounts.updateOrCreateUserFromExternalService is trying to match a string to an integer).

Ideally when people upgrade to whichever release a patch is included in, all of their existing user's service.twitter.id fields are migrated from an integer to a string. I'm not sure how to handle this elegantly though - or even if it's an appropriate solution?

@n1mmy

This comment has been minimized.

Member

n1mmy commented Jan 24, 2013

Riiight... so posting to twitter shouldn't be a problem, just matching on login.

As I see it there are two basic paths here:

  1. make Accounts.updateOrCreateUserFromExternalService match either number or string (something like selector = {$or: [{services.serviceName.id: idAsString}, {services.serviceName.id: idAsNumber}]}). It's a little odd to have something like that for Twitter directly in accounts-base, but as a limited time migration measure with a clear comment it seems reasonable.

  2. do the database migration as you suggest. This has the upside that people's databases will all have one type, avoiding potential future bugs. We don't have an elegant pattern for this yet. One simple thing to do would be to run a query at startup time to select all the users with a numeric twitter id ({services.twitter.id: {$type: 1}}) and then convert each one. This is idempotent as next time it runs it won't find anything, so we could safely run it once every time the server loads.

This isn't particularly elegant, but should work. We could also record that the migration has run in the database somewhere, and query that before running the migration to avoid scanning the whole users collection each restart. Though the column does already have an index, so maybe this actually won't help much.

It's worth noting that in an ideal world where we had a general purpose database migration mechanism and we wanted to support live migrations such that users didn't have to downtime for update, we'd need to do both of these: code that accepts either new or old versions, and a migration that can be run to translate new to old. I don't think live update is a requirement at this time though, the translation will be pretty fast, and people don't have big databases of twitter users yet.

Number 1 is much simpler, but I do fear the potential future burden of dealing with some documents with integer id. Number 2 could be done simply, and I don't think would cause any performance issues, but would need some careful testing.

Thoughts?

@timhaines

This comment has been minimized.

Contributor

timhaines commented Jan 24, 2013

I'll have a think about it overnight.

One other option is that the meteor_accounts_loginServiceConfiguration documents gain a version number for each service. Then on startup, migrations (or code paths) could be dependent on the version number a service is at.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Feb 2, 2013

Hey @timhaines,

It was really nice to meet you at the Devshop yesterday! Thanks for coming by.

Chatted w/ @gschmidt about this today, and we're leaning towards option 1. It's much simpler, and should work pretty well. Old accounts will be able to login fine and, as you say, the encoding is the same so developers using the id field to make API calls should be fine.

What do you think? Is this something you're still interested in implementing?

@timhaines

This comment has been minimized.

Contributor

timhaines commented Feb 3, 2013

Yes. Pull request #661 implements option 1. I attached some suggestions for Release Notes in the PR. The third suggestion mentions a potential breaking change if apps are looking up users by their twitter user id. Perhaps it's worth making that more explicit than I did in the suggestion.

It was great to meet a few of your crew in the Devshop too. Thanks for hosting it! :-)

@n1mmy

This comment has been minimized.

Member

n1mmy commented Feb 5, 2013

Fixed in #661.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment