Skip to content

fix: handle sub accounts properly#91

Open
pnadolny13 wants to merge 8 commits intoMatatika:mainfrom
MeltanoLabs:parent_customers
Open

fix: handle sub accounts properly#91
pnadolny13 wants to merge 8 commits intoMatatika:mainfrom
MeltanoLabs:parent_customers

Conversation

@pnadolny13
Copy link
Copy Markdown

Related to #79 and #83

I noticed that not all customers were coming through for me and after digging deeper it looks like the cause of the 403 Client Error: Forbidden for path I was experiencing is due to our requests not passing the proper login-customer-id when a customer account is a sub account. I reverted the changes in #83 as part of this and stopped seeing errors. I'm happy to not do that but I think this is the root cause so its better to throw errors again rather than silently skip.

Hierarchy:

  • AccessibleCustomers -> customers
    • CustomerHierarchyStream -> customer accounts
      • AdGroupsStream -> customer account records
      • CampaignsStream
      • etc.

This is my best attempt to describe whats happening 😅 :

  • AccessibleCustomers gets accessible customers and passes their ids to the child stream CustomerHierarchyStream as context
    • CustomerHierarchyStream then makes another request for accounts within that parent's accessible customer id. These are passed down as context to all other account streams (ads, ad groups, campaigns, etc)
      • The context thats passed down to account streams includes the account customer id which is then used to make requests directly on that account for ads, etc. /customers/{customer_id}/googleAds:search?query=
      • We were treating every account as a top level account so in the case where we have sub accounts we get a 403 forbidden even though we already know that we have access by way of the AccessibleCustomers parent request. These sub account need to be fetched slightly differently.
      • If an account is the top level then when making requests we set login-customer-id to the account customer ID we get from the parent record context (defined by CustomerHierarchyStream). /customers/1234/googleAds:search?query= and login-customer-id=1234
      • if an account is a sub account we need to set login-customer-id to the customer id passed down from AccessibleCustomers since we'll be querying through that customer to get the data. /customers/5678/googleAds:search?query= and login-customer-id=1234

@ReubenFrankel
Copy link
Copy Markdown

I reverted the changes in #83 as part of this and stopped seeing errors. I'm happy to not do that but I think this is the root cause so its better to throw errors again rather than silently skip.

I'm happy with reverting it as per our discussion in #79.

  • AccessibleCustomers gets accessible customers and passes their ids to the child stream CustomerHierarchyStream as context

    • CustomerHierarchyStream then makes another request for accounts within that parent's accessible customer id. These are passed down as context to all other account streams (ads, ad groups, campaigns, etc)

      • The context thats passed down to account streams includes the account customer id which is then used to make requests directly on that account for ads, etc. /customers/{customer_id}/googleAds:search?query=
      • We were treating every account as a top level account so in the case where we have sub accounts we get a 403 forbidden even though we already know that we have access by way of the AccessibleCustomers parent request. These sub account need to be fetched slightly differently.
      • If an account is the top level then when making requests we set login-customer-id to the account customer ID we get from the parent record context (defined by CustomerHierarchyStream). /customers/1234/googleAds:search?query= and login-customer-id=1234
      • if an account is a sub account we need to set login-customer-id to the customer id passed down from AccessibleCustomers since we'll be querying through that customer to get the data. /customers/5678/googleAds:search?query= and login-customer-id=1234

This behaviour generally makes sense to me (essentially setting login-customer-id more intelligently). I have some more questions around how this works with some of the changes here - I will bring these up in a review.

@ReubenFrankel ReubenFrankel self-requested a review March 13, 2025 01:18
@ReubenFrankel ReubenFrankel added the enhancement New feature or request label Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants