✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

cmm-airbyte · 2024-05-16T21:35:52Z

What

The Source Facebook Marketing Connector does not allow for choosing between providing an access token or using oauth, it only allows for users to define a source connection by following an oauth flow. This does not allow for use-cases in which a large organization may provision an access token to be used to authenticate to the Facebook Marketing data, rather than giving a user's personal Facebook account access.

How

The spec defines a credentials field that is a union between newly created OAuthCredentials and ServiceAccountCredentials. This allows for the user to select between using the Oauth flow and inserting Service Account Credentials.

Review guide

spec.py
config.py
source.py
sample_config.json
metadata.yaml
pyproject.toml
request_builder.py
test_ads_insights_action_product_id.py
test_videos.py
test_source.py

User Impact

The user is able to select between using the Oauth flow and inserting Service Account Credentials when setting up the Facebook Marketing Source.

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

Source PR here: #37625

…anges to solve build issues, and changes to solve unit testing issues.

…h 11 warnings

…e config. Updated version to reflect Airbyte versioning standard (this use case is a major version upgrade due to configuration of credentials needed to be changed)

…ebook-marketing-selectable-auth

…ebook-marketing-selectable-auth' into JohnMikkelsonOvative/feature-facebook-marketing-selectable-auth

…eting-selectable-auth

vercel · 2024-05-16T21:35:56Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		May 31, 2024 6:24pm

CLAassistant · 2024-05-16T21:35:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 4 committers have signed the CLA.

✅ JohnMikkelsonOvative
✅ marcosmarxm
❌ cristina.mariscal
❌ cmm-airbyte

cristina.mariscal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…irbyte into cmm-airbyte/fb_oauth

bazarnov · 2024-05-21T15:38:23Z

@cmm-airbyte We need to update the config in the GSM for the change to work, I believe.

maxi297 · 2024-05-21T18:45:02Z

We need to update the config in the GSM for the change to work, I believe.

I haven't check the change but it shouldn't be if the change is non-breaking with the config migration

maxi297

I'll test manually tomorrow morning but I don't see obvious issues with the code

maxi297 · 2024-05-21T18:45:43Z

airbyte-integrations/connectors/source-facebook-marketing/metadata.yaml

@@ -44,6 +44,9 @@ data:
              - "ads_insights_region"
              - "ads_insights_dma"
              - "ads_insights_action_product_id"
+      3.0.0:


What makes it breaking if we have the config migration? Does that mean that the user will have to opt-in on that change?

no, I was just guessing this because the PR cannot be directly reverted. I can do only a 2.2.0 version increment. WDYT?

I think 2.2.0 makes sense, yes! We should still document that the config format changes but there is a migration for backward compatibility but none for forward compatiblity

great. Thanks Max!

maxi297 · 2024-05-21T18:47:06Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+        """
+        credentials = config.get("credentials", None)
+        return credentials is None or (
+            "access_token" not in credentials and not ("client_id" in credentials and "client_id" in credentials)


This condition feels hard to maintain. If something adds a new auth method, they will need to know that they have to update an old migration script

mmm.. I think if there is a new change, an iteration over this migration will be required. isn't it?

Let's take the following scenario: on the 2025-01-04, a developer is doing the work to add username/password as an authentication mechanism. To do so, they update the spec to have credentials.username and credentials.password and configure the authenticator properly in the source.py. Now, this condition will always be triggered which means that the GSM secrets and the database entries will be updated on every operation of the connector. For the source in practice, I don't think it changes that much but I fear that those updates on the platform or GSM side could create issues in the long run. Hence, I would try to make the condition specific to this update. Is there a case where only credentials not being present wouldn't be enough to trigger this migration?

Note if we keep the condition as-is: the "client_id" in credentials and "client_id" in credentials part of the condition should probably be "client_id" in credentials and "client_secret" in credentials

maxi297 · 2024-05-21T18:50:51Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+    Starting from `3.0.0`, the `client_id`, `client_secret` and `access_token` will be placed at `credentials` path.
+    """
+
+    message_repository: MessageRepository = InMemoryMessageRepository()


This field is useless right now. We can just do print(create_connector_config_control_message(migrated_config).json(exclude_unset=True))

maxi297 · 2024-05-21T18:56:49Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+    message_repository: MessageRepository = InMemoryMessageRepository()
+
+    @classmethod
+    def should_migrate(cls, config: Mapping[str, Any]) -> bool:


It feels like this should be private as it is only used internally. What do you think? The same comment applies to transform, modify_and_save and emit_control_message

maxi297 · 2024-05-21T19:27:50Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+    def transform(cls, config: Mapping[str, Any]) -> Mapping[str, Any]:
+        # transform the config
+        config["credentials"] = {}
+        if "client_id" in config:


Set the type here so we can distinguish which type of authentication

If client_id and client_secret are not defined, we should use the auth Service

maxi297 · 2024-05-21T19:28:52Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/spec.py

+        title = "Authenticate via Facebook Marketing (Oauth)"
+        discriminator = "auth_type"
+
+    auth_type: Literal["Client"] = Field("Client", const=True)


This probably needs an access_token

…m-airbyte/fb_oauth

…irbyte into cmm-airbyte/fb_oauth

…m-airbyte/fb_oauth

maxi297

Can you confirm that the case I mentioned in the _should_migrate method is covered or not?

maxi297 · 2024-05-30T15:16:25Z

airbyte-integrations/connectors/source-facebook-marketing/integration_tests/spec.json

-        "airbyte_secret": true,
-        "type": "string"
+      "credentials": {
+        "title": "authentication credentials",


Why does this differ from spec.py? https://github.com/airbytehq/airbyte/pull/38304/files#diff-465a935c5dc0fc41f783e4f82ca3ba4bacf4d0dd97065fa273d7a7bdad94ac9bR33

maxi297 · 2024-05-30T15:33:42Z

...grations/connectors/source-facebook-marketing/source_facebook_marketing/config_migrations.py

+            > False, otherwise.
+        """
+        credentials = config.get("credentials", None)
+        return credentials is None or (


I think this condition needs to change with the new release process. At first, we wanted to release both the platform and the source at the same time. This meant that we only had to bother about the migration once as:

Before the update, access_token would be at the root but source would migrate this to credentials.access_token

After the update:

when configuring the source, the access_token would be on credentials.access_token

when logging in through oauth, the platform would set the access_token would be on credentials.access_token

However, our new plan have those releases split. This means that we will have a state where the source is updated but not the platform. In this case, we could have the following steps:

The source is updated

The migration happens so access_token is moved to credentials.access_token

A user does oauth, the access token is set to access_token

Given this condition, there will be credentials.access_token and we won't leverage the new token from oauth

I think the condition here should just be "is there an access token on the root level". Does that make sense?

…m-airbyte/fb_oauth

JohnMikkelsonOvative and others added 9 commits April 24, 2024 16:21

EMRGE-1446 Selectable Auth working on local CLI. Still need to add ch…

eccc2a1

…anges to solve build issues, and changes to solve unit testing issues.

EMRGE-1446 Unit Tests for Selectable Auth all passing, 205 passed wit…

cda978e

…h 11 warnings

EMRGE-1446 Changed naming to reflect Facebook standard, updated sampl…

ccd385a

…e config. Updated version to reflect Airbyte versioning standard (this use case is a major version upgrade due to configuration of credentials needed to be changed)

Merge branch 'airbytehq:master' into JohnMikkelsonOvative/feature-fac…

4079845

…ebook-marketing-selectable-auth

Merge remote-tracking branch 'origin/JohnMikkelsonOvative/feature-fac…

378f275

…ebook-marketing-selectable-auth' into JohnMikkelsonOvative/feature-facebook-marketing-selectable-auth

Merge branch 'master' into JohnMikkelsonOvative/feature-facebook-mark…

a5f4017

…eting-selectable-auth

Merge branch 'master' into JohnMikkelsonOvative/feature-facebook-mark…

7939c8a

…eting-selectable-auth

merged from JohnMikkelsonOvative

b60755a

FB OAuth switch

dba3748

Merge branch 'master' into cmm-airbyte/fb_oauth

f973e69

octavia-squidington-iii added area/connectors Connector related issues connectors/source/facebook-marketing labels May 16, 2024

vercel bot deployed to Preview May 16, 2024 21:40 View deployment

cristina.mariscal added 4 commits May 18, 2024 14:44

fixing unit test

040e234

pull from master

c55e538

Merge branch 'cmm-airbyte/fb_oauth' of https://github.com/airbytehq/a…

ec1ef32

…irbyte into cmm-airbyte/fb_oauth

fixing formatting

7aa761e

cmm-airbyte changed the title ~~FB oauth origin~~ ✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) May 21, 2024

cmm-airbyte marked this pull request as ready for review May 21, 2024 15:01

cmm-airbyte requested a review from a team as a code owner May 21, 2024 15:01

modifying specs to test pass

e5f8341

cmm-airbyte requested a review from a team as a code owner May 21, 2024 18:43

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 21, 2024

octavia-squidington-iv requested a review from a team May 21, 2024 18:45

vercel bot deployed to Preview May 21, 2024 18:48 View deployment

maxi297 reviewed May 21, 2024

View reviewed changes

cristina.mariscal added 3 commits May 22, 2024 05:55

Merge branch 'master' of https://github.com/airbytehq/airbyte into cm…

e68dca4

…m-airbyte/fb_oauth

applying feedback

8331b49

Merge branch 'master' of https://github.com/airbytehq/airbyte into cm…

864dc1a

…m-airbyte/fb_oauth

vercel bot deployed to Preview May 23, 2024 12:44 View deployment

bazarnov mentioned this pull request May 27, 2024

✨ Source Facebook-Marketing: Add Selectable Auth #37625

Open

2 tasks

cristina.mariscal and others added 3 commits May 29, 2024 04:53

pull from master

40285e0

fixes

fca9c4c

Merge branch 'master' into cmm-airbyte/fb_oauth

f935fca

vercel bot deployed to Preview May 29, 2024 12:10 View deployment

cristina.mariscal added 4 commits May 30, 2024 07:43

Merge branch 'master' of https://github.com/airbytehq/airbyte into cm…

cc91553

…m-airbyte/fb_oauth

Supporting credentials at both locations

9f1b19f

Merge branch 'cmm-airbyte/fb_oauth' of https://github.com/airbytehq/a…

ba309a6

…irbyte into cmm-airbyte/fb_oauth

Merge branch 'master' of https://github.com/airbytehq/airbyte into cm…

2476b72

…m-airbyte/fb_oauth

vercel bot deployed to Preview May 30, 2024 14:45 View deployment

maxi297 reviewed May 30, 2024

View reviewed changes

Merge branch 'master' of https://github.com/airbytehq/airbyte into cm…

82683f8

…m-airbyte/fb_oauth

bazarnov assigned cmm-airbyte May 31, 2024

cristina.mariscal added 3 commits May 31, 2024 06:59

fixes spec

ff417dd

re-format test jsons

985d552

affinf auth type to spec

5380649

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

cmm-airbyte commented May 16, 2024 •

edited

vercel bot commented May 16, 2024 •

edited

CLAassistant commented May 16, 2024 •

edited

bazarnov commented May 21, 2024

maxi297 commented May 21, 2024

maxi297 left a comment

maxi297 May 21, 2024

cmm-airbyte May 22, 2024

maxi297 May 22, 2024

cmm-airbyte May 22, 2024

maxi297 May 21, 2024

cmm-airbyte May 22, 2024

maxi297 May 22, 2024

maxi297 May 21, 2024

maxi297 May 21, 2024

maxi297 May 21, 2024

maxi297 May 21, 2024

maxi297 May 21, 2024

maxi297 left a comment

maxi297 May 30, 2024

maxi297 May 30, 2024

✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

Are you sure you want to change the base?

✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

Conversation

cmm-airbyte commented May 16, 2024 • edited

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented May 16, 2024 • edited

CLAassistant commented May 16, 2024 • edited

bazarnov commented May 21, 2024

maxi297 commented May 21, 2024

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmm-airbyte commented May 16, 2024 •

edited

vercel bot commented May 16, 2024 •

edited

CLAassistant commented May 16, 2024 •

edited