fix: resolve Google Ads enums to their name, not their number - #592
Merged
Conversation
mureo builds its client with the SDK default `use_proto_plus=False`, so
the response interceptor converts every row to raw protobuf before a
mapper sees it, and on raw protobuf an enum field is a plain `int` with
no `.name`. Twenty reads stringified such a field with a bare `str()`
and emitted "2" where every consumer keys on "AD".
The cost was not cosmetic, and almost all of it was silent:
- `change_import` classification had never worked. `_RESOURCE_KINDS`,
`_AD_LEVEL_RESOURCE_TYPES`, `_CRITERION_RESOURCE_TYPES` and
`_OPERATION_ALIASES` all key on names, so every imported external
change fell through to kind "" and lost its ad-level identity, taking
the dedupe that keys on the kind with it.
- RSA asset analysis returned nothing, and always had. Assets are sorted
by `field_type == "HEADLINE"` / `"DESCRIPTION"`, so both lists came
back empty for every live account, and with them `best_headlines`,
`worst_headlines` and all of the RSA audit. The tool reported no
accumulated data to accounts with years of it.
- Placement-attributed delivery was always empty: `exclusion_sources`
tests `type` for membership of {website, mobile_application}, and a
digit string is in neither. Both the exclusion-impact preview and the
delivery-collapse diagnosis read that basis and saw nothing.
- The provider adapter did fail loudly: it coerces `match_type` against
{EXACT, PHRASE, BROAD}, so every live `list_keywords` through it
raised ValueError.
- The rarely-served-keyword warning was dead code, keyword match types
mis-compared in two analyses, device insights named devices by number
(with a hand-written fallback that was itself wrong — DESKTOP is 4),
and the schedule listing emitted a day number the update path cannot
take back, so read/write could not round-trip.
Resolution now goes through the existing `map_enum_name`, which never
reads `.name` and resolves a raw int through a map. The maps live in a
new `_enum_names.py` and are derived from the SDK enum types by
comprehension, so they cannot go stale; `mappers.py` is at 791 of its
800-line budget, which forced the new module rather than in-lining.
Add a guard, because three review passes each found more instances after
the previous fix looked complete. `tests/test_google_ads_enum_reads.py`
sweeps `str(<attribute chain>)` across `mureo/google_ads/`, resolves each
chain against the live v23 descriptors, and fails when the field is
TYPE_ENUM. It keeps the two properties `test_gaql_field_names.py` states:
derived rather than enumerated, and an extraction that is asserted rather
than assumed. It earned its place immediately — the ad-schedule surface
above was found by the guard, not by review.
Tests assert against the raw-protobuf shape the client actually
delivers, parametrized over both shapes so a future `use_proto_plus`
flip cannot silently break them, with the premise pinned so nobody
simplifies them back to direct proto-plus construction. That is the
whole point: the previous tests passed against doubles whose `__str__`
returned "FieldType.HEADLINE", a shape production never produces.
Fixes #588
Merged
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
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.
Fixes #588.
The defect
mureo builds its Google Ads client with the SDK default
use_proto_plus=False, so the SDK's response interceptor converts every row to raw protobuf before a mapper sees it — and on raw protobuf an enum field is a plainintwith no.name. Twenty reads across the tree stringified such a field with a barestr(), so they emitted"2"where every consumer keys on"AD".The 3.11
IntEnum.__str__change that opened the issue turned out to be a red herring: on the real path the value was the bare integer on every Python version. The issue body was corrected in a follow-up comment.What was silently broken
Everything below failed with no exception and nothing logged, except where noted:
change_importclassification had never worked._RESOURCE_KINDS,_AD_LEVEL_RESOURCE_TYPES,_CRITERION_RESOURCE_TYPESand_OPERATION_ALIASESall key on names like"AD". Every imported external change fell through to kind"", lost its ad-level identity, and took the dedupe that keys on the kind down with it.google_ads_rsa_assets_analyzesorts assets byfield_type == "HEADLINE"/"DESCRIPTION", which"2"/"3"never satisfied — both lists came back empty for every live account, and with thembest_headlines,worst_headlines, and all ofgoogle_ads_rsa_assets_audit. The tool told accounts with years of data that none had accumulated.exclusion_sourcesteststypefor membership of{website, mobile_application}; a digit string is in neither, so no placement row was ever attributable. Both the exclusion-impact preview (Show delivery impact before applying bulk exclusions / blocks, and let it be a guardrail #547) and the delivery-collapse diagnosis (feat: detect and diagnose delivery collapse across all platforms #572) read that basis and saw nothing.match_typeagainst{EXACT, PHRASE, BROAD}and raises otherwise, so every livelist_keywordsthrough it raisedValueError: unknown keyword match_type: '2'."RARELY_SERVED" in "3"is never true._analysis_keywords(== "BROAD") andexclusion_impact.matching(== "EXACT")."2 has 0 conversions…", and the hand-written fallback was itself wrong (DESKTOPis 4, not 1 — that lookup was dead regardless).google_ads_keywords_suggestshipped digits against a description promisingLOW / MEDIUM / HIGH;google_ads_bid_adjustments_getshipped"6"against a contract promisingDEVICE.google_ads_schedule_targeting_listemitted"2"for the day while the update path takes day names, so read → write could not round-trip.The fix
Resolution goes through the existing
map_enum_name(value, MAP), which never reads.nameand resolves a raw int through a map. The maps live in a newmureo/google_ads/_enum_names.pyand are derived from the SDK enum types by comprehension — never hand-transcribed, so they cannot go stale. (mappers.pysits at 791/800 lines against its budget test, which forced the new module rather than in-lining.)map_enum_namecall sites: 10 at HEAD → 33 now, +23. Of those 23, 20 replaced a read that produced a digit in production, 1 is the second branch of an existing ternary, and 2 are consolidations of code that already worked (system_serving_status,ad_network_type) onto the shared derived maps.The guard — why this converged
Three review passes each found more instances after the previous "complete" fix. So this PR adds
tests/test_google_ads_enum_reads.py, which AST-sweepsstr(<attribute chain>)acrossmureo/google_ads/, resolves each chain against the live v23 descriptors, and fails when the field isTYPE_ENUM. It follows the two propertiestests/test_gaql_field_names.pystates: derived, never enumerated, and extraction is asserted, not assumed (the sweep is pinned by a tokenizer-based count, and every read subject must be bound to a proto message or listed as unbindable with a reason — asserted as a set in both directions).It earned its place immediately: the ad-schedule surface above was found by the guard, not by review.
Verified load-bearing: reintroducing a bare
str()on a fixed site turns it red; restoring turns it green.Documented blind spot, stated in the module docstring: a chain first assigned to a bare local (
raw = crit.status; str(raw)) escapes the sweep, and it only coversmureo/google_ads/. A manual sweep of the rest ofmureo/found no further instances — the defect class is specific to the Google Ads SDK's raw-protobuf interceptor; no other platform adapter touches protobuf.Tests
Every assertion runs against the raw-protobuf shape the client actually delivers, via
google.ads.googleads.util.convert_proto_plus_to_protobuf, and is parametrized over both shapes (raw-protobuf/proto-plus) so a futureuse_proto_plusflip cannot silently break it.TestWhyTheRawShapeIsTheRealOnepins the premise — the client passes nouse_proto_plus, the SDK default isFalse, a raw field is anintwith no.name— so nobody "simplifies" these back to direct proto-plus construction.That distinction is the whole point: the pre-existing tests passed against doubles whose
__str__returned"FieldType.HEADLINE", a shape production never produces. Those legacy tests are kept alongside the real-shape ones, each annotated with why it cannot see the bug.Verification
python3 -m pytest— 8841 passed, 12 failed, 8 skipped. The 12 are exactly this machine's known environmental baseline (9 live-client tests needing credentials, 3 tool-count tests inflated by a locally installed plugin); none touch files this PR modifies.python3 -m mypy mureo/ --ignore-missing-imports—Success: no issues found in 338 source filesruff check .—All checks passed!black --check .—682 files would be left unchangedFour code-review passes. Pass 1 raised a CRITICAL — the first attempt resolved via
.name, which does nothing on the raw-protobuf path — and the approach was reworked to be mapping-based. Passes 2 and 3 each surfaced further instances, now fixed. Pass 4 is clean.Follow-ups, not in this PR
_analysis_rsa.pyfiltersperformance_label in ("LOW", "POOR"), butPOORis not a member ofAssetPerformanceLabelin v23 — now that the labels resolve, that arm is permanently dead. Pre-existing, unrelated to the enum defect.google_ads_schedule_targeting_listemitsday_of_weekwhile the update path takesday, so the list output still can't be passed straight back in without renaming the key.map_enum_name, and_resolve_enumin_analysis_constants.py). Unifying them needs a decision about_resolve_enum's.namebranch, whichmap_enum_namedeliberately does not have. Its two hand-maintained maps were aliased onto the SDK-derived ones here; the resolver itself was left alone as provably not behaviour-preserving to migrate.