Skip to content

Strip only the literal properties/ prefix from GA property link IDs - #70919

Merged
potiuk merged 1 commit into
apache:mainfrom
Eason09053360:fix-analytics-admin-property-id-prefix
Aug 1, 2026
Merged

Strip only the literal properties/ prefix from GA property link IDs#70919
potiuk merged 1 commit into
apache:mainfrom
Eason09053360:fix-analytics-admin-property-id-prefix

Conversation

@Eason09053360

Copy link
Copy Markdown
Contributor

GoogleAnalyticsAdminCreatePropertyOperator builds the property link ID with
prop.name.lstrip("properties/"). str.lstrip removes any leading character
belonging to the given set — it is not a prefix strip — so it also consumes any
leading p, r, o, e, t, i, s or / of the property ID itself.

This is correct today only because GA4 property IDs are numeric, so there is no
user-visible behaviour change. The intent is a prefix strip, so removeprefix
expresses it directly and removes the latent trap.Improve semantics and readability

The persist call had no test coverage; the added parametrized test pins the
exact-prefix semantics and fails on the previous implementation.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

lstrip removes any leading character belonging to the given set, not a
prefix, so the property ID itself could lose leading characters. This is
correct today only because GA4 property IDs happen to be numeric; the
intent is a prefix strip and should be expressed as one.
@Eason09053360
Eason09053360 requested a review from shahar1 as a code owner August 1, 2026 16:57
@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Aug 1, 2026

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct fix for a genuine latent trap. str.lstrip takes a character set rather than a prefix, so it also eats any leading p, r, o, e, t, i, s or / belonging to the ID:

properties/123456789  ->  lstrip: 123456789   removeprefix: 123456789
properties/s123       ->  lstrip: 123         removeprefix: s123
properties/porridge   ->  lstrip: dge         removeprefix: porridge

I appreciate that the description is straight about this not being user-visible today — GA4 property IDs are numeric, digits are not in the strip set, so lstrip stops at the first digit and happens to give the right answer. Fixing the intent rather than claiming a live bug is the right framing.

The parametrized ("properties/s123", "s123") case is what makes this worth having: it returns "123" under the old implementation, so the test genuinely fails without the change instead of passing either way. Good that the inline comment records why that case exists, since it would otherwise look arbitrary. persist had no coverage at all before, so this closes that too.

One tidy-up before merge: the description has some leftover text — "removes the latent trap.Improve semantics and readability" — which reads like two drafts joined. Worth fixing, as the body becomes the squash commit message.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit c146f20 into apache:main Aug 1, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants