feat: Update OpenFeature User ID Mapping to Support userId Attribute
#91
Conversation
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
Co-authored-by: jonathan <jonathan@taplytics.com>
Co-authored-by: jonathan <jonathan@taplytics.com>
jonathannorris
requested review from
JamieSinn,
Copilot,
kaushalkapasi and
suthar26
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
suthar26
reviewed
Jul 22, 2025
JamieSinn
reviewed
Jul 22, 2025
… exclusion logic, include targeting_key - Remove unused user_id_source variable and assignments (addresses code clutter concern) - Consolidate user ID exclusion logic into single efficient statement - Include targeting_key in exclusion logic to prevent it from being added to custom data - Update comment to mention all three user ID types for clarity - Add comprehensive test coverage for targeting_key exclusion from attributes
- Split user ID validation into two distinct error cases: 1. Missing user ID (no valid targeting_key, user_id, or userId found) 2. Invalid type (user ID found but not a string) - Include original field name in type validation error for better debugging - Re-introduce user_id_source variable for error reporting purposes only - Enhanced error specificity: 'targeting_key must be a string, got int' vs generic message
…convention - Rename test functions from camelCase to snake_case: - test_create_user_from_context_with_userId → test_create_user_from_context_with_user_id_attribute - test_create_user_from_context_userId_excluded_when_used → test_create_user_from_context_user_id_attr_excluded_when_used - test_create_user_from_context_invalid_userId_types → test_create_user_from_context_invalid_user_id_attr_types - test_resolve_details_with_userId_priority → test_resolve_details_with_user_id_attr_priority - Rename variables from camelCase to snake_case: - userId → user_id_attr (to avoid confusion with user_id) - Remove unused variable assignments in test_provider.py - Fix whitespace and trailing whitespace issues
- Merged 6 user model tests into 2 comprehensive tests: 1. test_create_user_from_context_with_user_id_attribute: covers userId functionality, priority order, and error cases 2. test_create_user_from_context_user_id_exclusion: covers all user ID field exclusion scenarios - Removed redundant provider test since core logic is tested in user model - Maintained full test coverage while significantly reducing test count - All tests passing with same functionality coverage
- Applied Black formatter to test files to pass CI formatting checks - All tests still passing after formatting - Resolves 'Check formatting' CI failure
There was a problem hiding this comment.
Pull Request Overview
This PR extends OpenFeature evaluation context user identification to support userId as a third option for user identification, alongside the existing targeting_key and user_id support. The implementation maintains a clear priority hierarchy and ensures backward compatibility.
- Enhanced user ID resolution with priority:
targeting_key>user_id>userId - Updated attribute filtering to exclude all user ID types from custom data
- Improved error messages to include all supported user ID sources
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| devcycle_python_sdk/models/user.py | Added userId attribute support in user creation logic with priority handling and updated exclusion filtering |
| test/models/test_user.py | Added comprehensive test coverage for userId functionality, priority order, and exclusion behavior |
Comments suppressed due to low confidence (1)
devcycle_python_sdk/models/user.py:130
- [nitpick] The error message format is inconsistent with the previous error message style. Consider maintaining consistency by using the same prefix format: 'DevCycle: Evaluation context...' or create a more user-friendly message that doesn't expose internal variable names.
f"DevCycle: {user_id_source} must be a string, got {type(user_id).__name__}"
Comment on lines
109
to
+127
| user_id = None | ||
| user_id_source = None | ||
|
|
||
| if context: | ||
| if context.targeting_key: | ||
| user_id = context.targeting_key | ||
| user_id_source = "targeting_key" | ||
| elif context.attributes and "user_id" in context.attributes.keys(): | ||
| user_id = context.attributes["user_id"] | ||
| user_id_source = "user_id" | ||
| elif context.attributes and "userId" in context.attributes.keys(): | ||
| user_id = context.attributes["userId"] | ||
| user_id_source = "userId" | ||
|
|
||
| if not user_id or not isinstance(user_id, str): | ||
| if not user_id: | ||
| raise TargetingKeyMissingError( | ||
| "DevCycle: Evaluation context does not contain a valid targeting key or user_id attribute" | ||
| "DevCycle: Evaluation context does not contain a valid targeting key, user_id, or userId attribute" | ||
| ) | ||
|
|
There was a problem hiding this comment.
[nitpick] The user_id_source variable is only used for error messages when type validation fails. Consider initializing it closer to where it's used or using a more descriptive approach like extracting the source determination into a separate method to improve code clarity.
Copilot uses AI. Check for mistakes.
JamieSinn
approved these changes
Jul 22, 2025
jonathannorris
deleted the
cursor/update-openfeature-user-id-mapping-080b
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Summary
Extends OpenFeature evaluation context user identification to support
userIdas a third option alongside existingtargeting_keyanduser_idsupport.Changes
targeting_key>user_id>userIduser_id,userId,targeting_key) from custom datauserIdin user identification failure messagesTesting
userIdpriority and exclusion behaviorBackward Compatibility
targeting_keyanduser_idbehavior unchangeduserIdattribute supportThis change improves OpenFeature integration flexibility while maintaining consistent user identification behavior across all three supported user ID sources.
Open Background Agent:
Web · Cursor
[Slack Thread](https://taplytics.slack.com/archives/C02DU5GTB6U/p1752872862815949%3Fthread_ts=1752872862.815949