airbyte icon indicating copy to clipboard operation
airbyte copied to clipboard

Provide correct grant_type in Apple Ads Oauth

Open olagjo opened this issue 1 year ago • 2 comments

Based on our testing of this source, it seems like an explicit specification of grant_type: client_credentials is necessary in order not to crash the DeclarativeOauth2Authenticator.

When no grant type is specified, it expects grant type refresh token, but we have no refresh token and attempt to use client credentials

This fixes https://github.com/airbytehq/airbyte/issues/46879

What

Avoid OAuth ValueError "OauthAuthenticator needs a refresh_token parameter when grant_type is set to refresh_token when initializing a new Apple Search Ads source.

How

Explicitly specify that grant_type is client_credentials (this is already reflected in the url that we try to use)

Review guide

  1. Follow the installation steps for the Apple Search ads extension at main (import the yaml into your builder)
  2. Test the source
  • This should crash with the above-mentioned error
  1. Apply the fix from this PR (explicitly set grant_type to client_credentials in the auth-yaml) and retest
  • You should now receive data 🎉

User Impact

Now it should be possible to set up new Apple Search Ads sources again 🎉

Can this PR be safely reverted and rolled back?

  • [x] YES 💚
  • [ ] NO ❌

olagjo avatar Oct 10 '24 13:10 olagjo

@olagjo is attempting to deploy a commit to the Airbyte Growth Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Oct 10 '24 13:10 vercel[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 10 '24 13:10 CLAassistant

Thanks for attempting to fix this @olagjo. Can you bump the version, and update the changelog to proceed with this.

PS: Since the manifest only version has been shipped to cloud, I'll suggest forking the connector in cloud builder, make your changes, test that the fix worked as expected, and contribute back to the repo. Makes sense? Let me if you need me to clarify anything. Cheers!

topefolorunso avatar Oct 31 '24 19:10 topefolorunso

Hi @topefolorunso thank you for following up! I have tried to bump the version and the changelog :)

I'm also not fully following your suggestion – in order to make this fix in the first place, the first thing I did was use the manifest-only version of the source in my airbyte installation, and then tweak the one line in the manifest. The source then started working in my airbyte installation, so I decided to contribute the oneline fix upstream. Is this fundamentally different than using the cloud builder?

olagjo avatar Nov 01 '24 14:11 olagjo

@olagjo Thanks for submitting this contribution, and apologies for not catching this sooner! I had to make a few additional changes to the connector's metadata to appease our CI, so for expediency I threw in your fix in a fresh PR (link above). It was merged earlier today, and you should be good to go with the new version (0.2.1). Cheers!

ChristoGrab avatar Nov 12 '24 21:11 ChristoGrab

Thank you!

olagjo avatar Nov 12 '24 21:11 olagjo