fix(flags): fixing the last* bug with new flags #28359
Conversation
| // Check for variant override in the condition | ||
| let variant = if let Some(variant_override) = &condition.variant { | ||
| // Check if the override is a valid variant | ||
| if flag | ||
| .get_variants() | ||
| .iter() | ||
| .any(|v| &v.key == variant_override) | ||
| { | ||
| Some(variant_override.clone()) | ||
| } else { | ||
| // If override isn't valid, fall back to computed variant | ||
| self.get_matching_variant(flag, hash_key_overrides.clone()) | ||
| .await? | ||
| } | ||
| } else { | ||
| // No override, use computed variant | ||
| self.get_matching_variant(flag, hash_key_overrides.clone()) | ||
| .await? | ||
| }; |
There was a problem hiding this comment.
here's the relevant bit of Python that does what I'm doing here
| @@ -40,16 +40,11 @@ impl FlagRequest { | |||
| /// Only supports base64 encoded payloads or uncompressed utf-8 as json. | |||
| #[instrument(skip_all)] | |||
| pub fn from_bytes(bytes: Bytes) -> Result<FlagRequest, FlagError> { | |||
| tracing::debug!(len = bytes.len(), "decoding new request"); | |||
There was a problem hiding this comment.
these logs were from back in the day when I was debugging request traffic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR fixes a critical bug in the feature flag variant override functionality, focusing on aligning behavior between /decide and /flags endpoints.
- Added variant override validation in
flag_matching.rsto ensure specified variants exist before applying them - Fixed condition-level variant override handling in
flag_matching.rsto properly respect "All users in this set will be in control" settings - Removed unnecessary debug logging in
flag_request.rsfor cleaner error output - Added test coverage for variant override scenarios in
flag_matching.rsto prevent regressions
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| let payload = String::from_utf8(bytes.to_vec()).map_err(|e| { | ||
| tracing::error!("failed to decode body: {}", e); | ||
| FlagError::RequestDecodingError(String::from("invalid body encoding")) | ||
| })?; |
There was a problem hiding this comment.
style: consider using debug! instead of error! for decoding failures since they may be common with malformed requests
| // Check if the override is a valid variant | ||
| if flag | ||
| .get_variants() | ||
| .iter() | ||
| .any(|v| &v.key == variant_override) | ||
| { | ||
| Some(variant_override.clone()) | ||
| } else { |
There was a problem hiding this comment.
style: Consider adding early validation of variant overrides when flags are created to prevent invalid overrides at runtime
There was a problem hiding this comment.
yeah, good nit, but I'm not doing to do this yet.
*last bug so far....
But for real this is the last one: I was running my tests a final time and realized the last discrepancy between ``/decide
and/flags` was this one flag that I'd generated and I couldn't figure out why `/flags` was returning a different variant than `/decide`. I thought for sure I'd screwed up something with my hash calculation and painstakingly debugged the whole thing, and when I couldn't find any variance in that, I finally had the bright idea to take a closer look at the flag being evaluated. Here it isDid you catch that little bit underneath the condition set that said "All users in this set will be in control"? Because I didn't the first time.
Anyway, turns out that I'd never actually implement variant overrides with new flags, so that was the reason I saw a different variant in my tests. Phew.
Fixed this, and everything is coming up Milhouse. Frickin stoked.