Skip to content

fix(flags): remove lower() when evaluating feature flag payloads – these payloads are case-sensitive! #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 20, 2025

Conversation

dmarticus
Copy link
Contributor

@dmarticus dmarticus commented Feb 14, 2025

Remove lower() when evaluating feature flags + feature flag payloads – feature flags are case-sensitive. Closes #178

@dmarticus dmarticus changed the title fix(flags): fix(flags): use consistent casing when evaluating feature flags Feb 14, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses case sensitivity inconsistencies in PostHog's Python client feature flag handling, ensuring uniform case-insensitive flag key matching.

  • Modified get_feature_flag() in /posthog/client.py to normalize flag keys to lowercase for consistent evaluation
  • Added case-insensitive payload retrieval logic in _compute_payload_locally() to match flag evaluation behavior
  • Added test cases in /posthog/test/test_feature_flags.py to verify mixed-case flag keys work correctly for both evaluation and payload retrieval
  • Fixed issue Feature flag evaluation is case sensitive, while feature flag payload match evaluation is evaluated as lower() #178 where flag evaluation and payload retrieval had inconsistent case sensitivity handling

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@haacked
Copy link
Contributor

haacked commented Feb 14, 2025

Is that safe? Flag keys are case sensitive:

image

@haacked
Copy link
Contributor

haacked commented Feb 14, 2025

I looked at the linked issue #178:

It's unclear what the desired behavior is here--either feature flags should be all lower case, or the payload evaluation with lower() should be removed.

I think the payload evaluation with lower() probably should be removed. Otherwise we could introduce a bug where two flags with the same key but different casing will have a conflict where one will never be evaluated.

As an aside, I think allowing case sensitivity in flag keys is probably not ideal. But changing that is challenging if anybody has duplicate keys. But there are ways we could handle that, such as making it an unexposed project setting. All new projects (and old projects with no duplicates) have case-insensitive keys.

@haacked
Copy link
Contributor

haacked commented Feb 14, 2025

Some more context. In posthog-python

payload = flag_payloads.get(str(match_value).lower(), None)
we have this code:

payload = flag_payloads.get(str(match_value).lower(), None)

Which I think may be wrong as well. However, it's tricky because match_value can be either True or False or a variant key. In the case of variant key, we want to match case sensitively. In the case of True or False, we need to do the lower() because the payload keys in those cases are going to be "true" or "false".

@dmarticus dmarticus changed the title fix(flags): use consistent casing when evaluating feature flags fix(flags): remove lower() when evaluating feature flag payloads – these payloads are case-sensitive! Feb 19, 2025
@dmarticus dmarticus merged commit 337f7da into master Feb 20, 2025
6 checks passed
@dmarticus dmarticus deleted the fix/case-sensitivity-comparisons branch February 20, 2025 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature flag evaluation is case sensitive, while feature flag payload match evaluation is evaluated as lower()
3 participants