Instagram oAuth backend #7412
Instagram oAuth backend #7412
Conversation
| return Map.of("access_token", longLivedAccessToken); | ||
| } | ||
|
|
||
| private String getLongLivedAccessToken(final String clientId, final String clientSecret, final String shortLivedAccessToken) throws IOException { |
There was a problem hiding this comment.
Shouldn't this be DRY with the instagram one?
Opportunity to introduce a FacebookOAuthFlow that factors the process for facebook related connectors?
| .put("airbyte/source-github", new GithubOAuthFlow(configRepository)) | ||
| .put("airbyte/source-google-ads", new GoogleAdsOAuthFlow(configRepository)) | ||
| .put("airbyte/source-google-analytics-v4", new GoogleAnalyticsOAuthFlow(configRepository)) | ||
| .put("airbyte/source-google-search-console", new GoogleSearchConsoleOAuthFlow(configRepository)) | ||
| .put("airbyte/source-google-sheets", new GoogleSheetsOAuthFlow(configRepository)) | ||
| // Instagram connector use Facebook Marketing API | ||
| .put("airbyte/source-instagram", new FacebookOAuthFlow(configRepository)) |
There was a problem hiding this comment.
Should this be instead?
| .put("airbyte/source-instagram", new FacebookOAuthFlow(configRepository)) | |
| .put("airbyte/source-instagram", new InstagramOAuthFlow(configRepository)) |
There was a problem hiding this comment.
No, see comment above
// Instagram connector use Facebook Marketing API
I figured it out when I started doing e2e test, so I commited InstagramOAuthFlow just in case we will need it in future.
There was a problem hiding this comment.
Could we switch to InstagramOAuthFlow here even if the implementation is exactly the same?
So if, in the future, there's actual implementation differences, we wouldn't even need to change the mapping in the factory?
There was a problem hiding this comment.
The factory should point to specific concrete implementation as i commented here https://github.com/airbytehq/airbyte/pull/7412/files#r738454432
and we try to move as much code as possible in the common abstract class
There was a problem hiding this comment.
Could we switch to InstagramOAuthFlow here even if the implementation is exactly the same?
No, cause Instagram connector use Facebook Marketing API, it wouldnt work with InstagramOAuthFlow class. Ive figured it out after Ive done work on InstagramOAuthFlow so just added it to PR in case we going to need it in the future.
InstagramOAuthFlow has different auth workflow and generates token that doesnt work with facebook marketing API.
There was a problem hiding this comment.
Then that's very confusing and it is probably worth making it more visible in the code...
What about renaming InstagramOAuthFlow to UnusedInstagramOAuthFlow or InstagramAPIOAuthFlow instead and explains in a comment why it is there.
Then we should have an InstagramOAuthFlow that extends the FacebookOAuthFlow just like how FacebookMarketingOAuthFlow does now.
Or should we have an InstagramWithFacebookMarketingApiOAuthFlow class?
The factory would map source-instagram to InstagramOAuthFlow without surprises and we can switch the behavior / implementation of the class in the future without ever touching the factory
There was a problem hiding this comment.
Instagram connector use Facebook Marketing API
InstagramOAuthFlow has different auth workflow and generates token that doesnt work with facebook marketing API.
If i follow here, the source-instagram connector is implemented to pull instagram data from FacebookMarketingAPi.
If source-instagram connector is refactored to use the pure instagram API, then it would need a different oauthflow implementation
There was a problem hiding this comment.
if Instagram oauth flow is unused imo we should remove it from the code
There was a problem hiding this comment.
Then we should have an InstagramOAuthFlow that extends the FacebookOAuthFlow just like how FacebookMarketingOAuthFlow does now.
Done
if Instagram oauth flow is unused imo we should remove it from the code
Removed it
| @Override | ||
| protected String formatConsentUrl(final UUID definitionId, final String clientId, final String redirectUrl) throws IOException { | ||
| try { | ||
| return new URIBuilder(AUTHORIZE_URL) |
There was a problem hiding this comment.
the only difference here for Instagram from the super.formatConsentUrl from Facebook flow is the AUTHORIZE_URL?
Can't we just override a getAuthorizeUrl method and keep the rest DRY?
There was a problem hiding this comment.
No, they have different grant_type and parameters list (for instagram you dont need client secret and response_type parameters)
airbyte-oauth/src/main/java/io/airbyte/oauth/flows/facebook/InstagramOAuthFlow.java
Outdated
Show resolved
Hide resolved
| * Following docs from | ||
| * https://developers.facebook.com/docs/facebook-login/manually-build-a-login-flow | ||
| */ | ||
| public class FacebookMarketingOAuthFlow extends BaseOAuthFlow { |
There was a problem hiding this comment.
can we keep FacebookMarketingOAuthFlow extends FacebookOAuthFlow
And we define the getScope method in here and leave it as abstract in FacebookOAuthFlow?
Every facebook connector would have to re-implement a concrete class where the getScope or whatever method that differ slightly would be overriden
What
Resolves #7115