Improve validation of client ID and env #179
Conversation
WalkthroughRefactored validation logic in ApitallyClientBase from inline initialization checks to separate static methods. Client ID and environment are now stored as fields with an enabled flag toggled based on validation results. Invalid inputs log errors instead of raising exceptions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 91.90% 91.92% +0.02%
==========================================
Files 20 20
Lines 1889 1895 +6
==========================================
+ Hits 1736 1742 +6
Misses 153 153 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apitally/client/client_base.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
apitally/client/client_base.py
109-109: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (1)
apitally/client/client_base.py (1)
105-111: LGTM! UUID validation logic is correct.The implementation correctly validates UUID format using Python's built-in
UUIDconstructor. The static analysis hint suggesting to move the return to anelseblock is a false positive—the current early-return pattern is idiomatic and more readable.
| self.client_id = str(client_id) | ||
| self.env = str(env) | ||
| self.enabled = True | ||
|
|
||
| if not self.validate_client_id(self.client_id): | ||
| self.enabled = False | ||
| logger.error(f"invalid client_id '{self.client_id}' (expecting string in hexadecimal UUID format)") | ||
| if not self.validate_env(self.env): | ||
| self.enabled = False | ||
| logger.error( | ||
| f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters and hyphens only)" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Good approach to prevent TypeError, but consider warning about type mismatches.
The string conversion prevents the TypeError that was causing 500 errors, which directly addresses issue #173. However, silently converting non-string types (e.g., None → "None", 1234 → "1234") may hide configuration errors that developers should be aware of.
Consider adding type checks before conversion to log warnings:
+ if not isinstance(client_id, str):
+ logger.warning(f"client_id should be a string, got {type(client_id).__name__}")
self.client_id = str(client_id)
+ if not isinstance(env, str):
+ logger.warning(f"env should be a string, got {type(env).__name__}")
self.env = str(env)Additionally, verify that the enabled flag is checked in the middleware to ensure that when validation fails, the client is actually disabled and doesn't process requests:
🏁 Script executed:
#!/bin/bash
# Description: Verify that the enabled flag is checked before processing requests
# Search for usages of the enabled flag
rg -nP --type=py -C3 'self\.enabled|client\.enabled|instance\.enabled'Length of output: 9171
---
🏁 Script executed:
```shell
#!/bin/bash
# Retrieve the full implementation of validate_client_id and validate_env methods
rg -nP --type=py 'validate_(client_id|env)' -A 5 'apitally/client/client_base.py'
Length of output: 765
Fix regex pattern or error message inconsistency in validate_env method.
The enabled flag is properly checked throughout all middleware implementations before processing requests, which addresses the concern from the original review. However, there's an inconsistency in the validate_env method:
The regex pattern r"^[\w-]{1,32}$" includes underscores (since \w matches [a-zA-Z0-9_]), but the error message states: "expecting string with 1-32 alphanumeric characters and hyphens only" — which doesn't mention underscores.
Either update the error message to include underscores, or change the regex to r"^[a-zA-Z0-9-]{1,32}$" to match the documented behavior. Lines 114-115 in apitally/client/client_base.py and lines 57-58 must be kept in sync.
🤖 Prompt for AI Agents
In apitally/client/client_base.py around lines 47-58 (and ensure consistency
with validate_env at lines 114-115), the regex r"^[\w-]{1,32}$" allows
underscores while the error message states "alphanumeric characters and hyphens
only"; update the implementation so both agree — change the regex to only allow
letters, digits and hyphens (e.g. use a pattern that matches [a-zA-Z0-9-]{1,32})
or alternatively update the error message to mention underscores; pick the
former (change the regex to exclude underscores) and ensure the error message
and validate_env pattern on lines 114-115 remain in sync.
| @staticmethod | ||
| def validate_env(env: str) -> bool: | ||
| return re.match(r"^[\w-]{1,32}$", env) is not None |
There was a problem hiding this comment.
Regex pattern includes underscores but error message doesn't mention them.
The regex pattern \w matches alphanumeric characters, underscores, and (when combined with the hyphen in the character class) hyphens. However, the error message at line 57 states "alphanumeric characters and hyphens only" without mentioning underscores.
Choose one of the following fixes:
Option 1: Exclude underscores from the pattern (if not intended):
- def validate_env(env: str) -> bool:
- return re.match(r"^[\w-]{1,32}$", env) is not None
+ def validate_env(env: str) -> bool:
+ return re.match(r"^[a-zA-Z0-9-]{1,32}$", env) is not NoneOption 2: Update the error message to reflect that underscores are allowed:
logger.error(
- f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters and hyphens only)"
+ f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters, hyphens, and underscores)"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def validate_env(env: str) -> bool: | |
| return re.match(r"^[\w-]{1,32}$", env) is not None | |
| @staticmethod | |
| def validate_env(env: str) -> bool: | |
| return re.match(r"^[a-zA-Z0-9-]{1,32}$", env) is not None |
🤖 Prompt for AI Agents
In apitally/client/client_base.py around lines 113–115, the validate_env regex
uses \w which allows underscores but the error message (line 57) says
"alphanumeric characters and hyphens only"; to fix, replace \w in the character
class with an explicit alphanumeric set (e.g. [A-Za-z0-9]) so the pattern
becomes /^[A-Za-z0-9-]{1,32}$/ and thus matches the error text; update any
related tests or callers if they relied on underscores.
Fixes #173
Summary by CodeRabbit