Skip to content
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

Bind API version with omniauth-facebook version #190

Closed
kenn opened this issue Mar 26, 2015 · 13 comments

Comments

@kenn
Copy link

commented Mar 26, 2015

In README, it is suggested that we can specify an API version like v2.2, but it's not clear what is the best way actually.

client_options: {
  site: 'https://graph.facebook.com/v2.2',
  authorize_url: 'https://www.facebook.com/v2.2/dialog/oauth' }

What I want to do is use the latest Facebook API version that is tested with this gem's version, not using unversioned API endpoints as it can break in the future.

So, say, if omniauth-facebook-2.0.1 is tested with Facebook API v2.2, I want to use v2.2. Is it possible?

facebook_api_version: :tested    # equivalent to 'v2.2' in this gem version
@simi

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2015

Actually omniauth-facebook is not tested against real API endpoint via automated tests.

@kenn

This comment has been minimized.

Copy link
Author

commented Mar 29, 2015

Here's what happened to me: I had been using the default setting, which is the unversioned API endpoints.

At a later time, I started to receive an exception that I hadn't seen, and the error was "unique constraint error for a user_id with multiple Facebook User IDs associated with the same user".

Turns out, Facebook updated the API at some point and now they return app_scoped_user_id instead of a real user ID, so our database now contains mixed data. We couldn't fix this issue until we change the database schema and rewrite the data model.

By nature, unversioned API endpoints will bring in incompatible changes in the future, and I believe libraries like omniauth-facebook should offer a better default than this.

So, even though it's not really tested with a real API endpoint, I suggest that we have a required (not optional) config of the API version in string like facebook_api_version: 'v2.2'. We could even make the unversioned optionally available as facebook_api_version: :unversioned, but the point is, it's a choice that you consciously make. That way, the responsibility is delegated to the user of this library, and it won't break on production at a random time.

How does that sound?

@simi

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2015

I think current behaviour is standard across omniauth-oauth(2) based gems. Also it is written in README.

OmniAuth Facebook uses unversioned API endpoints by default. You can configure custom endpoints via client_options hash passed to provider.

We can add some shortcut to select API version, but I think it will be good to keep unversioned API as default one.

@kenn

This comment has been minimized.

Copy link
Author

commented Apr 8, 2015

I think current behaviour is standard across omniauth-oauth(2) based gems.

I don't think that's true, for instance, omniauth-twitter hardcodes v1.1 path and omniauth-github hardcodes v3.

Also it is written in README.

It's my fault for sure that I missed it in README, but I learned it the hard way, by corrupting the integrity of the database. Considering how disastrous the consequence was, I don't see any reason that we'd ever want to use the unversioned API on production. The risk outweighs any potential benefits IMO.

@simi

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2015

OK, are you able to provide PR for this?

@semaperepelitsa

This comment has been minimized.

Copy link

commented Apr 13, 2015

I don’t think it is a responsibility of an application to decide the API version at all. The library is making API calls, not my app. I guess, the problem is that there are no automated tests with the real API.

@simi

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2015

let's decide what we need to close this

  1. test against real API
  2. lock to tested API

@dixalex

This comment has been minimized.

Copy link

commented Apr 23, 2015

Testing against real API would be better, but if no time for this - locking to tested API is also an option. The issue just needs to be resolved as soon as possible.

@simi simi added the new-feature label Jul 17, 2015

@simi simi self-assigned this Jul 17, 2015

@mkdynamic

This comment has been minimized.

Copy link
Owner

commented Jun 27, 2016

Next version will use versioned APIs.

@mkdynamic mkdynamic closed this Jun 27, 2016

@kenn

This comment has been minimized.

Copy link
Author

commented Jun 27, 2016

Awesome, thanks Mark! 👍

@svoop

This comment has been minimized.

Copy link

commented Jul 26, 2016

@mkdynamic Facebook keeps nagging due to the Aug 8 deadline for unversioned API access. What would you say, better switch to the RC of version 4 now or is version 4 about to be released within the next couple of days anyways?

@mkdynamic

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

@svoop Just released 4.0.0, since no bug reports from the RC. Let me know how it goes.

@svoop

This comment has been minimized.

Copy link

commented Nov 15, 2016

Cleaning my inbox, I come across this old issue. @mkdynamic Yep, worked like a charm, thanks a bunch for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.