feat(CFTL-489): fail-on-missing-permissions option for Meta extractors (SUPPORT-15700)#58
Open
matyas-jirat-keboola wants to merge 5 commits into
Conversation
Meta extractors (facebook-pages, facebook-ads-v2, instagram-v2) could finish with Success while extracting only the accounts table: per-account Facebook OAuth errors (codes 190/200/10 and 100/33 "missing permissions") were swallowed in both the page_loader recoverable-error path and the per-account log-and-continue loops in client.py. Add an opt-in config option `fail-on-missing-permissions` (default false, so existing configs are unchanged). When enabled, the first authorization error raised during extraction fails the job with a UserException (exit 1) instead of silently producing empty/partial output. - configuration.py: new fail_on_missing_permissions field - page_loader.py: FacebookErrorHandler.is_authorization_error / raise_if_authorization_error; flag threaded into PageLoader and applied at every HTTP error boundary before the recoverable check - client.py: flag threaded into FacebookClient + PageLoader; per-account loops re-raise UserException instead of swallowing it - tests: unit coverage for detection, page_loader boundary, client propagation
Switch the fail-on-missing-permissions behavior from fail-fast to collect-all: attempt every account/query, accumulate authorization errors, then fail once with a UserException listing every affected account. Safe because a non-zero exit makes the platform discard all output, so no partial data is committed regardless of when we raise — this just yields a complete error message. - page_loader.py: add AuthorizationError sentinel (not a UserException, so it doesn't abort on first hit) + authorization_error_details(); each HTTP boundary raises it when the option is on - client.py: FacebookClient collects per-account errors in permission_errors; raise_for_permission_errors() raises one deduped UserException at the end; per-account loops record-and-continue (page-token/user-token fallback honored) - component.py: call raise_for_permission_errors() after queries are processed - tests: cover detection, page_loader boundary (AuthorizationError), and client collect-then-fail (13 tests)
…support-15700-facebook-ads-v2-no-error-returned # Conflicts: # src/client.py # src/component.py
…t loops The merge with #57 surfaced a regression: the `except UserException: raise` clauses added to the per-account load/start loops also propagated pre-existing user-actionable errors (e.g. INVALID_METRIC from raise_if_user_actionable), which main deliberately swallows per-account (skip the query, job continues). This broke #57's instagram_account_v24/v25 cassette tests. CFTL-489 only needs AuthorizationError collection, not a change to how other errors are handled. Remove the over-reaching `except UserException: raise` from _start_async_jobs_for_query, _process_single_sync_query, and its user-token fallback (the poll handler keeps the same effective behavior as #57, where UserException propagates by not being in _CONTAINED_OBJECT_ERRORS).
keboola-pr-reviewer-bot
left a comment
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 3/5) · profile component-factory
This is a well-designed opt-in feature with default=False that preserves all existing behavior, but it modifies exception-handling boundaries across multiple async and sync extraction paths in…
Concerns:
src/client.py: _poll_and_process_async_jobs exception order change (UserException→AuthorizationError→contained) in async polling path needs verificationsrc/client.py: Batch path (code 406+) records AuthorizationError with account_id=None; dedupe key falls back to message string, potentially under-reportingsrc/page_loader.py: load_page_from_url raises AuthorizationError with account_id=None; caller in client.py has no fill-in for the missing account_id
Suggested reviewers: @keboola/component-factory
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Meta extractors (
keboola.ex-facebook-pages,keboola.ex-facebook-ads-v2,keboola.ex-instagram-v2— all built from this one codebase) can finish a job with Success / exit 0 while extracting nothing beyond theaccountstable./me/accountsdoes not requireads_read/ads_management, so it always succeeds. When the token has lost per-account permissions (or expired), every other query fails per-account with a Facebook OAuth error (codes 190 / 200 / 10, or 100 / subcode 33 "missing permissions"). Those errors are currently swallowed in two layers:FacebookErrorHandler.is_recoverable_error()treats the 100/33 case as recoverable → returns empty data.except Exception: ... continueloops inclient.pylog-and-skip non-recoverable errors (e.g. code 200).Result: the job reports Success and the missing data goes unnoticed.
Change
Adds an opt-in config option
parameters.fail-on-missing-permissions(boolean, defaultfalse→ existing configs behave exactly as before).When enabled, the extractor collects every per-account authorization error during the run and then fails once at the end with a
UserException(exit 1) listing every affected account — instead of silently producing empty/partial output.Design decisions
ex-facebookUI for all of them.Files
configuration.py— newfail_on_missing_permissionsfield (aliasfail-on-missing-permissions).page_loader.py—AuthorizationErrorsentinel (not aUserException, so it doesn't abort on first hit) +FacebookErrorHandler.is_authorization_error/authorization_error_details; the flag is threaded intoPageLoader, and each HTTP error boundary (_load_regular_page,load_page_from_url,start_async_insights_job, async final-results) raisesAuthorizationErrorbefore the recoverable check.client.py—FacebookClientcollects per-account errors inpermission_errors;raise_for_permission_errors()raises one dedupedUserExceptionat the end. Per-account loops record-and-continue (the page-token → user-token fallback is preserved); the batch path records too.except UserException: raiseis kept for genuine immediate failures.component.py— passes the flag into the client and callsraise_for_permission_errors()after all queries are processed.tests/test_fail_on_missing_permissions.py— detection, page_loader boundary (AuthorizationError), and client collect-then-fail (13 tests).Testing
uv run pytest tests/test_fail_on_missing_permissions.py→ 13 passed.*_keboola_accounts_v2xfunctional cases fail locally because those fixture dirs are empty/untracked stale local artifacts — they don't exist in CI and are unrelated to this change.UI
The matching checkbox ("Fail the job on authorization errors") is added in the
keboola/uimonorepo (sharedex-facebookmodule): keboola/ui#6306.Refs: CFTL-489, SUPPORT-15700