Conversation
WalkthroughCentralizes URL protection and auth-key resolution inside AuthConfig, removes related utils, propagates per-path auth headers through proxy-generated methods, drops ProxyPluginConfig.black_list, and adds/updates tests, mocks, fixtures, and pytest config for proxy endpoints. Changes
Sequence DiagramsequenceDiagram
participant Plugin as Proxy Plugin
participant Auth as AuthConfig
participant Ingress as Ingress/Router
participant Remote as Remote Proxy Service
Plugin->>Auth: get_auth_keys(url)
alt URL not protected
Auth-->>Plugin: None
Plugin->>Ingress: register_route(..., headers=None)
else Exact match
Auth-->>Plugin: specific_keys
Plugin->>Ingress: register_route(..., headers=compute(specific_keys))
else Wildcard match (longest)
Auth-->>Plugin: wildcard_keys
Plugin->>Ingress: register_route(..., headers=compute(wildcard_keys))
else Fallback
Auth-->>Plugin: general_keys
Plugin->>Ingress: register_route(..., headers=compute(general_keys))
end
Ingress->>Remote: fetch_response(method, url, headers)
Remote-->>Ingress: response
Ingress-->>Ingress: log success (includes params)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/framex/config.py (1)
75-99: Auth key resolution logic is correct with hierarchical matching.The
get_auth_keysmethod implements proper precedence: exact match → longest wildcard prefix → general keys. The logic correctly handles all test cases.Note: If
auth_urlsis configured but all key lists (general_auth_keysand matchingspecial_auth_keys) are empty, the method returns an empty list[], which calling code may treat as "no auth required" (via falsy check). This is a configuration issue rather than a code bug, but consider adding validation to ensure at least one non-empty key list exists whenauth_urlsis non-empty.Optional: Add validation to catch misconfiguration
Consider adding this validation to catch the edge case where URLs are protected but no keys are configured:
@model_validator(mode="after") def validate_special_auth_urls(self) -> Self: for special_url in self.special_auth_keys: if not self._is_url_protected(special_url): raise ValueError(f"special_auth_keys url '{special_url}' is not covered by any auth_urls rule") + + # Ensure at least some auth keys exist if auth_urls is configured + if self.auth_urls and not self.general_auth_keys and not self.special_auth_keys: + raise ValueError("auth_urls is configured but no authentication keys provided") return self
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/framex/config.py(1 hunks)src/framex/driver/ingress.py(2 hunks)src/framex/plugins/proxy/__init__.py(6 hunks)src/framex/plugins/proxy/config.py(2 hunks)src/framex/utils.py(0 hunks)tests/test_utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- src/framex/utils.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_utils.py (2)
src/framex/utils.py (2)
StreamEnventType(41-45)make_stream_event(48-55)src/framex/config.py (1)
get_auth_keys(75-99)
src/framex/plugins/proxy/config.py (1)
src/framex/config.py (1)
AuthConfig(54-99)
src/framex/plugins/proxy/__init__.py (3)
src/framex/adapter/base.py (1)
BaseAdapter(20-68)src/framex/adapter/__init__.py (1)
get_adapter(9-13)src/framex/config.py (1)
get_auth_keys(75-99)
src/framex/driver/ingress.py (2)
src/framex/utils.py (1)
escape_tag(26-28)src/framex/config.py (1)
get_auth_keys(75-99)
🪛 Ruff (0.14.8)
src/framex/config.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
src/framex/plugins/proxy/config.py (1)
5-5: LGTM! Clean integration of authentication configuration.The addition of the
authfield toProxyPluginConfigenables per-plugin authentication configuration. This allows the proxy plugin to authenticate its outbound requests to external services independently of the global authentication config used for inbound client requests.Also applies to: 18-18
tests/test_utils.py (1)
7-7: LGTM! Test updates correctly reflect the new API.The test properly exercises the new
AuthConfig.get_auth_keys()instance method instead of the removed module-level helper. Test coverage remains comprehensive, validating exact matches, wildcard patterns, special keys, and unprotected URLs.Also applies to: 47-55
src/framex/driver/ingress.py (1)
19-19: LGTM! Auth key retrieval correctly uses settings-based lookup.The ingress now retrieves authentication keys via
settings.auth.get_auth_keys(), aligning with the consolidated authentication logic inAuthConfig. The local import in__init__is appropriate for initialization code.Also applies to: 47-49
src/framex/config.py (1)
62-62: LGTM! URL protection logic correctly implements exact and wildcard matching.The
_is_url_protectedmethod properly handles both exact matches and wildcard patterns. Note that wildcard rules like/api/v1/*intentionally match/api/v1/usersbut not/api/v1itself (the prefix without trailing content).Also applies to: 66-73
src/framex/plugins/proxy/__init__.py (2)
11-11: LGTM! Type annotation improves code clarity.The explicit
BaseAdaptertype annotation for the adapter variable enhances code readability and type checking.Also applies to: 59-59
70-76: LGTM! Auth header integration correctly implements outbound authentication.The code properly:
- Retrieves auth keys for the proxy path using
settings.auth.get_auth_keys(path)- Uses the walrus operator to safely check for non-empty keys before creating the Authorization header
- Threads headers through to the final
fetch_responsecallNote: This implements FrameX → External Service authentication using the proxy plugin's auth config (
settings.authfromproxy.config). This is separate from Client → FrameX authentication, which is handled by the ingress using the global auth config fromframex.config.Also applies to: 102-104, 163-163, 187-187
|
Creating a local commit for the generated unit tests... The unit tests have been committed to the current branch. Commit ID: |
touale
force-pushed
the
feat/enhance-proxy-api-key-auth
branch
from
70e11c9 to
facab28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/framex/driver/ingress.py (1)
47-49: Move the import to module level.The local import inside the loop is inefficient and non-idiomatic. Python's import system caches modules, but the lookup still occurs on every iteration.
🔎 Apply this diff to move the import to the top:
from framex.adapter import get_adapter +from framex.config import settings from framex.consts import BACKEND_NAME from framex.driver.application import create_fastapi_applicationThen remove the local import block:
): - from framex.config import settings - auth_keys = settings.auth.get_auth_keys(plugin_api.api) self.register_route(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/framex/config.py(1 hunks)src/framex/driver/ingress.py(2 hunks)src/framex/plugins/proxy/__init__.py(6 hunks)src/framex/plugins/proxy/config.py(2 hunks)src/framex/utils.py(0 hunks)tests/test_config.py(1 hunks)tests/test_utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- src/framex/utils.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_utils.py (3)
src/framex/cli.py (1)
framex(11-12)src/framex/utils.py (2)
StreamEnventType(41-45)make_stream_event(48-55)src/framex/config.py (1)
get_auth_keys(75-99)
tests/test_config.py (2)
src/framex/config.py (3)
AuthConfig(54-99)_is_url_protected(66-73)get_auth_keys(75-99)src/framex/plugins/proxy/config.py (1)
ProxyPluginConfig(9-18)
src/framex/plugins/proxy/__init__.py (3)
src/framex/adapter/base.py (1)
BaseAdapter(20-68)src/framex/adapter/__init__.py (1)
get_adapter(9-13)src/framex/config.py (1)
get_auth_keys(75-99)
src/framex/driver/ingress.py (2)
src/framex/utils.py (1)
escape_tag(26-28)src/framex/config.py (1)
get_auth_keys(75-99)
src/framex/plugins/proxy/config.py (1)
src/framex/config.py (1)
AuthConfig(54-99)
🪛 GitHub Actions: Test Project
tests/test_config.py
[error] 50-50: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 55-55: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 68-68: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 76-76: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 94-94: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 107-107: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 120-120: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 124-124: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 137-137: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 151-151: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 168-168: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 172-172: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 187-187: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 193-193: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 210-210: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 227-227: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 230-230: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 242-242: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 264-264: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 274-274: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 287-287: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 304-304: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 322-322: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 375-375: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 384-384: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 398-398: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 411-411: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 420-420: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 429-429: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 433-433: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 448-448: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 459-459: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 475-475: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 491-491: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
🪛 Ruff (0.14.8)
src/framex/config.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
src/framex/plugins/proxy/config.py (1)
5-5: LGTM! Clean integration of AuthConfig.The addition of
AuthConfigtoProxyPluginConfigprovides a clean way to configure authentication for proxy endpoints. The default emptyAuthConfig()ensures backward compatibility.Also applies to: 18-19
tests/test_utils.py (1)
36-55: LGTM! Clean migration to the new AuthConfig API.The test correctly instantiates
AuthConfigdirectly and validates theget_auth_keysmethod behavior with various URL patterns. The test cases remain comprehensive and clearly demonstrate exact matches, wildcard matches, and precedence rules.src/framex/config.py (2)
66-73: LGTM! Clean URL protection logic.The
_is_url_protectedmethod correctly handles both exact matches and wildcard patterns (trailing/*). The logic is straightforward and efficient.
75-99: LGTM! Well-designed key resolution with proper precedence.The
get_auth_keysmethod implements a sensible priority order:
- Returns
Nonefor unprotected URLs- Exact matches in
special_auth_keystake precedence- Longest matching wildcard wins for overlapping patterns
- Falls back to
general_auth_keysThis design provides flexibility while maintaining predictable behavior.
src/framex/plugins/proxy/__init__.py (1)
69-76: Potential IndexError when auth keys list is empty.Line 72 accesses
auth_api_key[0]without checking if the list is non-empty. Looking atAuthConfig.get_auth_keys()in src/framex/config.py (line 99), it can return an empty list whengeneral_auth_keysis empty and no special keys match.🔎 Apply this diff to add a safety check:
# Get auth api_keys if auth_api_key := settings.auth.get_auth_keys(path): - headers = {"Authorization": auth_api_key[0]} # Use the first auth key set - logger.debug(f"Proxy api({path}) requires auth") + if auth_api_key: # Ensure list is not empty + headers = {"Authorization": auth_api_key[0]} + logger.debug(f"Proxy api({path}) requires auth") + else: + headers = None else: headers = NoneAlternatively, simplify the logic:
# Get auth api_keys - if auth_api_key := settings.auth.get_auth_keys(path): - headers = {"Authorization": auth_api_key[0]} # Use the first auth key set + auth_api_key = settings.auth.get_auth_keys(path) + if auth_api_key and len(auth_api_key) > 0: + headers = {"Authorization": auth_api_key[0]} logger.debug(f"Proxy api({path}) requires auth") else: headers = NoneLikely an incorrect or invalid review comment.
|
|
||
|
|
||
| def test_auth_config_is_url_protected_exact_match(): | ||
| """Test _is_url_protected with exact URL matches.""" | ||
| auth = AuthConfig( | ||
| auth_urls=["/api/v1/user", "/api/v2/admin", "/health"], | ||
| ) | ||
|
|
||
| # Exact matches should return True | ||
| assert auth._is_url_protected("/api/v1/user") is True | ||
| assert auth._is_url_protected("/api/v2/admin") is True | ||
| assert auth._is_url_protected("/health") is True | ||
|
|
||
| # Non-matches should return False | ||
| assert auth._is_url_protected("/api/v1/users") is False | ||
| assert auth._is_url_protected("/api/v1") is False | ||
| assert auth._is_url_protected("/api/v2") is False | ||
| assert auth._is_url_protected("/healthcheck") is False | ||
|
|
||
|
|
||
| def test_auth_config_is_url_protected_wildcard_match(): | ||
| """Test _is_url_protected with wildcard patterns.""" | ||
| auth = AuthConfig( | ||
| auth_urls=["/api/v1/*", "/admin/*", "/public/docs/*"], | ||
| ) | ||
|
|
||
| # URLs under wildcard paths should return True | ||
| assert auth._is_url_protected("/api/v1/user") is True | ||
| assert auth._is_url_protected("/api/v1/user/profile") is True | ||
| assert auth._is_url_protected("/api/v1/anything") is True | ||
| assert auth._is_url_protected("/admin/login") is True | ||
| assert auth._is_url_protected("/admin/settings/advanced") is True | ||
| assert auth._is_url_protected("/public/docs/readme") is True | ||
|
|
||
| # URLs not under wildcard paths should return False | ||
| assert auth._is_url_protected("/api/v2/user") is False | ||
| assert auth._is_url_protected("/api/v1") is False | ||
| assert auth._is_url_protected("/public/images/logo.png") is False | ||
| assert auth._is_url_protected("/administrator/") is False | ||
|
|
||
|
|
||
| def test_auth_config_is_url_protected_mixed_rules(): | ||
| """Test _is_url_protected with both exact and wildcard rules.""" | ||
| auth = AuthConfig( | ||
| auth_urls=[ | ||
| "/api/v1/*", | ||
| "/api/v2/specific", | ||
| "/health", | ||
| "/admin/*", | ||
| ], | ||
| ) | ||
|
|
||
| # Mixed rule matching | ||
| assert auth._is_url_protected("/api/v1/anything") is True | ||
| assert auth._is_url_protected("/api/v2/specific") is True | ||
| assert auth._is_url_protected("/api/v2/other") is False | ||
| assert auth._is_url_protected("/health") is True | ||
| assert auth._is_url_protected("/healthcheck") is False | ||
| assert auth._is_url_protected("/admin/panel") is True | ||
|
|
||
|
|
||
| def test_auth_config_is_url_protected_empty_rules(): | ||
| """Test _is_url_protected with no auth_urls configured.""" | ||
| auth = AuthConfig(auth_urls=[]) | ||
|
|
||
| # Everything should be unprotected | ||
| assert auth._is_url_protected("/api/v1/user") is False | ||
| assert auth._is_url_protected("/admin/login") is False | ||
| assert auth._is_url_protected("/health") is False | ||
| assert auth._is_url_protected("/anything") is False | ||
|
|
||
|
|
||
| def test_auth_config_is_url_protected_edge_cases(): | ||
| """Test _is_url_protected with edge cases.""" | ||
| auth = AuthConfig( | ||
| auth_urls=["/api/*", "/exact", "/*"], | ||
| ) | ||
|
|
||
| # Root wildcard should match everything | ||
| assert auth._is_url_protected("/anything") is True | ||
| assert auth._is_url_protected("/nested/path") is True | ||
|
|
||
| # Empty string and special characters | ||
| assert auth._is_url_protected("") is False | ||
| assert auth._is_url_protected("/api/v1/user?query=param") is True | ||
| assert auth._is_url_protected("/api/v1/user#fragment") is True | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_unprotected_url(): | ||
| """Test get_auth_keys returns None for unprotected URLs.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general_key"], | ||
| auth_urls=["/api/v1/*"], | ||
| ) | ||
|
|
||
| # Unprotected URLs should return None | ||
| assert auth.get_auth_keys("/health") is None | ||
| assert auth.get_auth_keys("/api/v2/user") is None | ||
| assert auth.get_auth_keys("/public/docs") is None | ||
| assert auth.get_auth_keys("") is None | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_general_keys(): | ||
| """Test get_auth_keys returns general keys for protected URLs without special rules.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general_key_1", "general_key_2"], | ||
| auth_urls=["/api/v1/*", "/api/v2/specific"], | ||
| ) | ||
|
|
||
| # Protected URLs without special keys should return general keys | ||
| assert auth.get_auth_keys("/api/v1/user") == ["general_key_1", "general_key_2"] | ||
| assert auth.get_auth_keys("/api/v1/admin/settings") == ["general_key_1", "general_key_2"] | ||
| assert auth.get_auth_keys("/api/v2/specific") == ["general_key_1", "general_key_2"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_exact_special_keys(): | ||
| """Test get_auth_keys returns special keys for exact URL matches.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/v1/*", "/api/v2/admin"], | ||
| special_auth_keys={ | ||
| "/api/v1/user": ["user_special_1", "user_special_2"], | ||
| "/api/v2/admin": ["admin_special"], | ||
| }, | ||
| ) | ||
|
|
||
| # Exact matches should return special keys | ||
| assert auth.get_auth_keys("/api/v1/user") == ["user_special_1", "user_special_2"] | ||
| assert auth.get_auth_keys("/api/v2/admin") == ["admin_special"] | ||
|
|
||
| # Other URLs should return general keys | ||
| assert auth.get_auth_keys("/api/v1/other") == ["general"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_wildcard_special_keys(): | ||
| """Test get_auth_keys returns special keys for wildcard pattern matches.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/v1/*", "/api/v2/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/admin/*": ["admin_key_1", "admin_key_2"], | ||
| "/api/v2/user/*": ["user_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # Wildcard special key matches | ||
| assert auth.get_auth_keys("/api/v1/admin/settings") == ["admin_key_1", "admin_key_2"] | ||
| assert auth.get_auth_keys("/api/v1/admin/users/list") == ["admin_key_1", "admin_key_2"] | ||
| assert auth.get_auth_keys("/api/v2/user/profile") == ["user_key"] | ||
| assert auth.get_auth_keys("/api/v2/user/settings/privacy") == ["user_key"] | ||
|
|
||
| # Non-matching URLs should return general keys | ||
| assert auth.get_auth_keys("/api/v1/public") == ["general"] | ||
| assert auth.get_auth_keys("/api/v2/public") == ["general"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_longest_prefix_match(): | ||
| """Test get_auth_keys uses longest prefix matching for overlapping wildcards.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/*"], | ||
| special_auth_keys={ | ||
| "/api/*": ["api_key"], | ||
| "/api/v1/*": ["v1_key"], | ||
| "/api/v1/admin/*": ["admin_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # Should match the longest (most specific) prefix | ||
| assert auth.get_auth_keys("/api/v1/admin/settings") == ["admin_key"] | ||
| assert auth.get_auth_keys("/api/v1/user") == ["v1_key"] | ||
| assert auth.get_auth_keys("/api/v2/user") == ["api_key"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_exact_takes_precedence(): | ||
| """Test that exact matches take precedence over wildcard matches.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/v1/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/*": ["wildcard_key"], | ||
| "/api/v1/user": ["exact_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # Exact match should take precedence | ||
| assert auth.get_auth_keys("/api/v1/user") == ["exact_key"] | ||
|
|
||
| # Other URLs should use wildcard | ||
| assert auth.get_auth_keys("/api/v1/admin") == ["wildcard_key"] | ||
| assert auth.get_auth_keys("/api/v1/anything") == ["wildcard_key"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_no_general_keys(): | ||
| """Test get_auth_keys when no general keys are configured.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=[], | ||
| auth_urls=["/api/v1/*"], | ||
| ) | ||
|
|
||
| # Should return empty list when no special keys match | ||
| assert auth.get_auth_keys("/api/v1/user") == [] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_complex_scenario(): | ||
| """Test get_auth_keys with a complex real-world scenario.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["default_key"], | ||
| auth_urls=[ | ||
| "/api/v1/*", | ||
| "/api/v2/public", | ||
| "/api/v2/private/*", | ||
| "/admin/*", | ||
| ], | ||
| special_auth_keys={ | ||
| "/api/v1/auth/*": ["auth_service_key"], | ||
| "/api/v1/user/profile": ["profile_key_1", "profile_key_2"], | ||
| "/admin/*": ["admin_master_key"], | ||
| "/admin/super/*": ["super_admin_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # Various scenarios | ||
| assert auth.get_auth_keys("/api/v1/auth/login") == ["auth_service_key"] | ||
| assert auth.get_auth_keys("/api/v1/auth/register") == ["auth_service_key"] | ||
| assert auth.get_auth_keys("/api/v1/user/profile") == ["profile_key_1", "profile_key_2"] | ||
| assert auth.get_auth_keys("/api/v1/user/settings") == ["default_key"] | ||
| assert auth.get_auth_keys("/api/v2/public") == ["default_key"] | ||
| assert auth.get_auth_keys("/api/v2/private/data") == ["default_key"] | ||
| assert auth.get_auth_keys("/admin/users") == ["admin_master_key"] | ||
| assert auth.get_auth_keys("/admin/super/settings") == ["super_admin_key"] | ||
|
|
||
| # Unprotected | ||
| assert auth.get_auth_keys("/health") is None | ||
| assert auth.get_auth_keys("/api/v3/anything") is None | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_empty_special_keys(): | ||
| """Test get_auth_keys when special_auth_keys dict is empty.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/*"], | ||
| special_auth_keys={}, | ||
| ) | ||
|
|
||
| # All protected URLs should return general keys | ||
| assert auth.get_auth_keys("/api/v1/user") == ["general"] | ||
| assert auth.get_auth_keys("/api/v2/admin") == ["general"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_multiple_overlapping_wildcards(): | ||
| """Test get_auth_keys with multiple overlapping wildcard rules.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/*", "/api/v1/*", "/api/v1/admin/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/admin/users/*": ["users_key"], | ||
| "/api/v1/admin/settings/*": ["settings_key"], | ||
| "/api/v1/admin/*": ["admin_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # Most specific match wins | ||
| assert auth.get_auth_keys("/api/v1/admin/users/list") == ["users_key"] | ||
| assert auth.get_auth_keys("/api/v1/admin/users/create") == ["users_key"] | ||
| assert auth.get_auth_keys("/api/v1/admin/settings/update") == ["settings_key"] | ||
| assert auth.get_auth_keys("/api/v1/admin/logs") == ["admin_key"] | ||
| assert auth.get_auth_keys("/api/v1/other") == ["general"] | ||
|
|
||
|
|
||
| def test_auth_config_get_auth_keys_trailing_slash_handling(): | ||
| """Test get_auth_keys behavior with trailing slashes.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["general"], | ||
| auth_urls=["/api/v1/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/user": ["user_key"], | ||
| }, | ||
| ) | ||
|
|
||
| # URLs with and without trailing slashes | ||
| assert auth.get_auth_keys("/api/v1/user") == ["user_key"] | ||
| # Note: trailing slash creates different URL, so it won't match exact rule | ||
| assert auth.get_auth_keys("/api/v1/user/") == ["general"] | ||
|
|
||
|
|
||
| def test_auth_config_validator_with_wildcard_special_keys(): | ||
| """Test that validator accepts wildcard patterns in special_auth_keys.""" | ||
| # This should not raise an error | ||
| auth = AuthConfig( | ||
| auth_urls=["/api/v1/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/admin/*": ["admin_key"], | ||
| }, | ||
| ) | ||
| assert auth is not None | ||
|
|
||
|
|
||
| def test_auth_config_validator_rejects_uncovered_exact_special_keys(): | ||
| """Test validator rejects special_auth_keys not covered by auth_urls (exact match).""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| AuthConfig( | ||
| auth_urls=["/api/v1/user"], | ||
| special_auth_keys={ | ||
| "/api/v2/admin": ["admin_key"], | ||
| }, | ||
| ) | ||
| assert "special_auth_keys url '/api/v2/admin'" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_auth_config_validator_rejects_uncovered_wildcard_special_keys(): | ||
| """Test validator rejects wildcard special_auth_keys not covered by auth_urls.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| AuthConfig( | ||
| auth_urls=["/api/v1/*"], | ||
| special_auth_keys={ | ||
| "/api/v2/admin/*": ["admin_key"], | ||
| }, | ||
| ) | ||
| assert "special_auth_keys url '/api/v2/admin/*'" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_auth_config_validator_accepts_covered_special_keys(): | ||
| """Test validator accepts special_auth_keys properly covered by auth_urls.""" | ||
| # Exact special key covered by exact auth_url | ||
| auth1 = AuthConfig( | ||
| auth_urls=["/api/v1/user"], | ||
| special_auth_keys={ | ||
| "/api/v1/user": ["user_key"], | ||
| }, | ||
| ) | ||
| assert auth1 is not None | ||
|
|
||
| # Exact special key covered by wildcard auth_url | ||
| auth2 = AuthConfig( | ||
| auth_urls=["/api/v1/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/user": ["user_key"], | ||
| }, | ||
| ) | ||
| assert auth2 is not None | ||
|
|
||
| # Wildcard special key covered by parent wildcard auth_url | ||
| auth3 = AuthConfig( | ||
| auth_urls=["/api/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/admin/*": ["admin_key"], | ||
| }, | ||
| ) | ||
| assert auth3 is not None | ||
|
|
||
|
|
||
| def test_proxy_plugin_config_has_auth(): | ||
| """Test that ProxyPluginConfig includes auth field.""" | ||
| from framex.plugins.proxy.config import ProxyPluginConfig | ||
|
|
||
| # Default auth should be empty AuthConfig | ||
| config = ProxyPluginConfig() | ||
| assert hasattr(config, "auth") | ||
| assert isinstance(config.auth, AuthConfig) | ||
| assert config.auth.general_auth_keys == [] | ||
| assert config.auth.auth_urls == [] | ||
| assert config.auth.special_auth_keys == {} | ||
|
|
||
|
|
||
| def test_proxy_plugin_config_auth_configuration(): | ||
| """Test ProxyPluginConfig with custom auth configuration.""" | ||
| from framex.plugins.proxy.config import ProxyPluginConfig | ||
|
|
||
| config = ProxyPluginConfig( | ||
| proxy_urls=["http://example.com"], | ||
| auth=AuthConfig( | ||
| general_auth_keys=["proxy_key"], | ||
| auth_urls=["/api/*"], | ||
| special_auth_keys={"/api/admin": ["admin_key"]}, | ||
| ), | ||
| ) | ||
|
|
||
| assert config.auth.general_auth_keys == ["proxy_key"] | ||
| assert config.auth.auth_urls == ["/api/*"] | ||
| assert config.auth.special_auth_keys == {"/api/admin": ["admin_key"]} | ||
|
|
||
|
|
||
| def test_auth_config_default_values(): | ||
| """Test AuthConfig with default values.""" | ||
| auth = AuthConfig() | ||
|
|
||
| assert auth.general_auth_keys == [] | ||
| assert auth.auth_urls == [] | ||
| assert auth.special_auth_keys == {} | ||
|
|
||
| # Methods should work with empty config | ||
| assert auth._is_url_protected("/any/url") is False | ||
| assert auth.get_auth_keys("/any/url") is None | ||
|
|
||
|
|
||
| def test_auth_config_single_key_scenarios(): | ||
| """Test AuthConfig with single key configurations.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["single_general_key"], | ||
| auth_urls=["/api/*"], | ||
| special_auth_keys={ | ||
| "/api/admin": ["single_special_key"], | ||
| }, | ||
| ) | ||
|
|
||
| assert auth.get_auth_keys("/api/user") == ["single_general_key"] | ||
| assert auth.get_auth_keys("/api/admin") == ["single_special_key"] | ||
|
|
||
|
|
||
| def test_auth_config_case_sensitivity(): | ||
| """Test that URL matching is case-sensitive.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["key"], | ||
| auth_urls=["/api/v1/User"], | ||
| ) | ||
|
|
||
| # Case-sensitive matching | ||
| assert auth._is_url_protected("/api/v1/User") is True | ||
| assert auth._is_url_protected("/api/v1/user") is False | ||
| assert auth._is_url_protected("/API/V1/USER") is False | ||
|
|
||
|
|
||
| def test_auth_config_special_characters_in_urls(): | ||
| """Test AuthConfig with special characters in URLs.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["key"], | ||
| auth_urls=["/api/v1/user-profile", "/api/v1/user_settings", "/api/v1/user.info"], | ||
| special_auth_keys={ | ||
| "/api/v1/user-profile": ["profile_key"], | ||
| }, | ||
| ) | ||
|
|
||
| assert auth._is_url_protected("/api/v1/user-profile") is True | ||
| assert auth._is_url_protected("/api/v1/user_settings") is True | ||
| assert auth._is_url_protected("/api/v1/user.info") is True | ||
| assert auth.get_auth_keys("/api/v1/user-profile") == ["profile_key"] | ||
|
|
||
|
|
||
| def test_auth_config_numeric_paths(): | ||
| """Test AuthConfig with numeric components in paths.""" | ||
| auth = AuthConfig( | ||
| general_auth_keys=["key"], | ||
| auth_urls=["/api/v1/*", "/api/v2/*"], | ||
| special_auth_keys={ | ||
| "/api/v1/user/123": ["user_123_key"], | ||
| }, | ||
| ) | ||
|
|
||
| assert auth._is_url_protected("/api/v1/user/123") is True | ||
| assert auth.get_auth_keys("/api/v1/user/123") == ["user_123_key"] | ||
| assert auth.get_auth_keys("/api/v1/user/456") == ["key"] |
There was a problem hiding this comment.
Fix formatting issues blocking the pipeline.
The test suite is comprehensive and well-structured, covering exact matches, wildcards, precedence rules, validators, and edge cases. However, the pipeline is failing due to 34 whitespace formatting errors on blank lines.
Run the following command to fix all whitespace issues:
#!/bin/bash
# Remove trailing whitespace from blank lines in tests/test_config.py
sed -i 's/^[[:space:]]*$//' tests/test_config.py🧰 Tools
🪛 GitHub Actions: Test Project
[error] 50-50: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 55-55: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 68-68: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 76-76: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 94-94: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 107-107: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 120-120: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 124-124: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 137-137: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 151-151: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 168-168: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 172-172: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 187-187: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 193-193: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 210-210: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 227-227: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 230-230: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 242-242: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 264-264: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 274-274: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 287-287: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 304-304: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 322-322: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 375-375: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 384-384: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 398-398: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 411-411: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 420-420: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 429-429: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 433-433: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 448-448: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 459-459: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 475-475: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
[error] 491-491: ruff: W293 - Blank line contains whitespace. Remove whitespace from blank line.
🤖 Prompt for AI Agents
In tests/test_config.py (around lines 43-494) there are trailing-whitespace
characters on blank lines causing 34 formatting errors; remove trailing spaces
from blank lines (and any trailing whitespace at end of lines) throughout the
file and ensure the file ends with a single newline, then re-run the
formatter/CI; apply the change either with your editor's "trim trailing
whitespace" feature or a project-standard formatting tool and commit the fixed
file.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/mock.py (1)
14-14: Consider using custom exception classes for better error handling.The long exception messages in
AssertionErrorcalls could be replaced with custom exception classes for better testability and clarity. However, this is acceptable for test mocks whereAssertionErrorclearly indicates test failures.Also applies to: 45-45
tests/api/test_proxy.py (1)
6-8: Inconsistent path construction across tests.This test constructs the path using
f"{API_STR}/proxy/mock/info", while other tests in this file use direct paths like"/proxy/mock/get". Consider using a consistent approach across all tests for better maintainability.🔎 Suggested fix for consistency:
Option 1: Use
API_STRconsistently (if all paths should be prefixed):def test_get_proxy_version(client: TestClient): res = client.get(f"{API_STR}/proxy/mock/info").json() assert res == {"info": "i_am_mock_proxy_info"} def test_get_proxy_get(client: TestClient): params = {"message": "hello world"} - res = client.get("/proxy/mock/get", params=params).json() + res = client.get(f"{API_STR}/proxy/mock/get", params=params).json()Option 2: Use direct paths consistently (if the API_STR prefix is unnecessary):
def test_get_proxy_version(client: TestClient): - res = client.get(f"{API_STR}/proxy/mock/info").json() + res = client.get("/api/v1/proxy/mock/info").json() assert res == {"info": "i_am_mock_proxy_info"}src/framex/plugins/proxy/__init__.py (1)
78-84: Handle missing or unknown parameter types more explicitly.The parameter extraction logic silently skips parameters when:
param.get("schema").get("type")returnsNonetype_map.get(typ)returnsNonefor unrecognized typesThis could lead to silent failures where required parameters are omitted from the API signature. Consider adding logging or warnings when parameters are skipped due to missing or unsupported types.
🔎 Consider adding explicit handling:
params: list[tuple[str, Any]] = [ (name, c_type) for param in (body.get("parameters") or []) if (name := param.get("name")) and (typ := param.get("schema").get("type")) and (c_type := type_map.get(typ)) ] + + # Log skipped parameters for debugging + for param in (body.get("parameters") or []): + name = param.get("name") + schema = param.get("schema", {}) + typ = schema.get("type") + if name and (not typ or not type_map.get(typ)): + logger.warning(f"Skipping parameter '{name}' with unsupported type: {typ}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.pre-commit-config.yaml(1 hunks)book/src/advanced_usage/system_proxy_plugin.md(0 hunks)book/src/basic_usage/plugin_configreation.md(0 hunks)pytest.ini(1 hunks)src/framex/driver/ingress.py(3 hunks)src/framex/plugins/proxy/__init__.py(5 hunks)src/framex/plugins/proxy/config.py(2 hunks)tests/api/test_proxy.py(1 hunks)tests/conftest.py(3 hunks)tests/consts.py(1 hunks)tests/mock.py(1 hunks)tests/plugins/alias_model.py(2 hunks)
💤 Files with no reviewable changes (2)
- book/src/basic_usage/plugin_configreation.md
- book/src/advanced_usage/system_proxy_plugin.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/framex/driver/ingress.py
🧰 Additional context used
🧬 Code graph analysis (5)
pytest.ini (1)
tests/test_plugin.py (1)
test_call_plugin_api_with_proxy_success(74-88)
tests/conftest.py (2)
tests/mock.py (2)
mock_get(7-16)mock_request(19-47)src/framex/__init__.py (1)
run(55-144)
tests/api/test_proxy.py (1)
tests/conftest.py (1)
client(62-64)
src/framex/plugins/proxy/__init__.py (3)
src/framex/adapter/base.py (1)
BaseAdapter(20-68)src/framex/adapter/__init__.py (1)
get_adapter(9-13)src/framex/config.py (1)
get_auth_keys(75-99)
src/framex/plugins/proxy/config.py (1)
src/framex/config.py (1)
AuthConfig(54-99)
🪛 Ruff (0.14.8)
tests/mock.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
tests/plugins/alias_model.py (1)
1-1: LGTM! Correct Pydantic v2 import.The
ConfigDictimport is necessary for the Pydantic v2 configuration style used below..pre-commit-config.yaml (1)
21-21: LGTM!The exclusion pattern correctly exempts test utility files (
consts.pyandmock.py) from thename-tests-testhook, which is appropriate since these files contain test helpers rather than test cases.pytest.ini (1)
25-26: LGTM!The proxy configuration correctly specifies the mock proxy URL and whitelist for testing. The omission of
/proxy/mock/black_getfrom the whitelist is intentional, enabling validation of the filtering behavior intest_get_proxy_black_get.tests/conftest.py (1)
50-58: LGTM!The fixture correctly uses
patchto mock HTTP interactions during test execution. The generator pattern withyieldproperly maintains the patched context throughout the test session.tests/consts.py (1)
1-155: LGTM!The mock OpenAPI specification is comprehensive and well-structured. It provides adequate coverage for testing both whitelisted endpoints (for positive cases) and blacklisted endpoints (for negative cases), along with proper schema definitions.
tests/api/test_proxy.py (1)
11-31: LGTM!The test cases comprehensively cover GET/POST methods with query parameters, JSON body, and the negative case for non-whitelisted endpoints. The assertions properly verify request method propagation and response structure.
src/framex/plugins/proxy/__init__.py (2)
156-204: LGTM!The dynamic method creation correctly propagates headers through to the HTTP request, enabling per-request authentication. The implementation properly handles both streaming and non-streaming responses, with appropriate error handling and logging.
69-74: Clarify the intent of multiple authentication keys or implement retry fallback logic.The implementation calls
get_auth_keys()which returns a list of keys, but uses onlyauth_api_key[0]. Since the configuration supports multiple keys per path (e.g.,"/api/v1/c/*": ["key1", "key2"]), clarify whether this is:
- Intentional (only first key should ever be used), or
- A gap where alternative keys should be tried on authentication failure
If key rotation or fallback is intended, implement retry logic to attempt alternate keys when the primary authentication fails.
src/framex/plugins/proxy/config.py (1)
17-17: TheProxyPluginConfig.authfield is actively used and not redundant.At line 70 of
src/framex/plugins/proxy/__init__.py, the code callssettings.auth.get_auth_keys(path). Thesettingsobject is imported fromsrc/framex/plugins/proxy/config.py(line 18) and is an instance ofProxyPluginConfigcreated viaget_plugin_config("proxy", ProxyPluginConfig). Therefore,settings.authrefers to the plugin-specificProxyPluginConfig.authfield, not the global auth configuration. The field is correctly designed to allow plugin-specific authentication configuration and is being used as intended.Likely an incorrect or invalid review comment.
| "method": "POST", | ||
| "params": params, | ||
| } | ||
| elif url.endswith("/proxy/mock/post_model") and method == "POST": |
There was a problem hiding this comment.
Fix the typo in method name.
There's a typo: endsW ith should be endswith. This will cause an AttributeError when the code tries to check if the URL ends with "/proxy/mock/post_model".
🔎 Apply this diff to fix the typo:
- elif url.endsW ith("/proxy/mock/post_model") and method == "POST":
+ elif url.endswith("/proxy/mock/post_model") and method == "POST":📝 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.
| elif url.endswith("/proxy/mock/post_model") and method == "POST": | |
| elif url.endswith("/proxy/mock/post_model") and method == "POST": |
🤖 Prompt for AI Agents
In tests/mock.py around line 35, there is a typo in the string method call:
replace the incorrect `endsW ith` with the correct Python method `endswith` on
the URL check (`url.endswith("/proxy/mock/post_model")`) so the code uses the
valid attribute and avoids AttributeError.
|
|
||
| class Config: | ||
| populate_by_name = True | ||
| model_config = ConfigDict(populate_by_name=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the test file
fd tests/plugins/alias_model.py -x cat -n {} | head -40Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
# Check for Pydantic version in common dependency files
find . -type f \( -name "pyproject.toml" -o -name "requirements.txt" -o -name "setup.py" -o -name "setup.cfg" \) | xargs grep -l pydantic 2>/dev/null | head -3Repository: touale/FrameX-kit
Length of output: 77
🏁 Script executed:
# Read the pyproject.toml to check Pydantic version
cat pyproject.toml | grep -A 5 -B 5 pydantic
# Read the target file
cat -n tests/plugins/alias_model.py | head -40
# Search for any remaining Pydantic v1 Config classes
rg -n "class Config:" --type py -A 2 | head -30Repository: touale/FrameX-kit
Length of output: 1428
🏁 Script executed:
# Search for other uses of populate_by_name in the codebase
rg -n "populate_by_name" --type py
# Also check if there are any validate_by_name patterns already in use
rg -n "validate_by_name\|validate_by_alias" --type pyRepository: touale/FrameX-kit
Length of output: 223
Update to future-proof Pydantic v2.11+ configuration.
The code correctly follows Pydantic v2 migration patterns, but populate_by_name usage is not recommended in v2.11+ and will be deprecated in v3; instead, you should use the validate_by_name configuration setting. Since the project requires pydantic>=2.11.7, migrate to: ConfigDict(validate_by_name=True, validate_by_alias=True) for consistency with current best practices. This same pattern appears in tests/test_plugins.py:24 and should also be updated.
🤖 Prompt for AI Agents
In tests/plugins/alias_model.py around line 22, replace the deprecated
ConfigDict(populate_by_name=True) usage with the Pydantic v2.11+ recommended
settings: use ConfigDict(validate_by_name=True, validate_by_alias=True) so
validation by name and alias is explicit and future-proof; also update the
identical occurrence in tests/test_plugins.py at line 24 to the same
ConfigDict(validate_by_name=True, validate_by_alias=True).
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/api/test_proxy.py (1)
6-43: Consider adding negative test cases.The current test suite covers happy paths effectively. Consider adding tests for error scenarios to improve coverage:
- Missing or invalid query parameters
- Authentication failures (missing/invalid auth headers)
- Malformed JSON bodies
- Nonexistent endpoints beyond the blacklist test
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pytest.ini(1 hunks)src/framex/plugins/proxy/__init__.py(5 hunks)src/framex/plugins/proxy/config.py(2 hunks)tests/api/test_proxy.py(1 hunks)tests/consts.py(1 hunks)tests/mock.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/framex/plugins/proxy/__init__.py (3)
src/framex/adapter/base.py (1)
BaseAdapter(20-68)src/framex/plugins/proxy/config.py (1)
is_white_url(19-28)src/framex/config.py (1)
get_auth_keys(75-99)
tests/api/test_proxy.py (1)
tests/conftest.py (1)
client(62-64)
src/framex/plugins/proxy/config.py (1)
src/framex/config.py (1)
AuthConfig(54-99)
🪛 Ruff (0.14.8)
tests/mock.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
pytest.ini (1)
25-29: LGTM! Test configuration correctly sets up proxy plugin auth settings.The pytest environment variables properly configure the new auth structure with general keys, auth URLs with wildcard patterns, and special auth keys for specific endpoints. The double braces for dict values are correctly escaped for pytest-env.
tests/consts.py (1)
1-203: LGTM! Well-structured mock OpenAPI specification for testing.The mock response provides comprehensive coverage of proxy endpoints including GET/POST operations, model-based requests, and auth-protected paths. The schema definitions align with OpenAPI 3.1.0 standards and support the test scenarios for whitelist/auth functionality.
tests/mock.py (2)
7-16: LGTM! Clean mock implementation for OpenAPI schema retrieval.The
mock_getfunction correctly handles the OpenAPI endpoint and fails fast on unexpected URLs, which helps catch test misconfiguration early.
19-44: LGTM! Mock request handler covers the core proxy endpoints.The implementation properly simulates responses for GET/POST operations with appropriate method/URL validation.
src/framex/plugins/proxy/config.py (1)
17-17: LGTM! Auth configuration integration.Adding the nested
AuthConfigfield cleanly centralizes authentication settings for proxy endpoints, aligning with the broader refactoring to useAuthConfig.get_auth_keys().src/framex/plugins/proxy/__init__.py (3)
65-74: LGTM! Auth key retrieval and header propagation.The whitelist check and conditional auth header setup correctly integrates with
AuthConfig.get_auth_keys(). Using the first auth key for theAuthorizationheader is a reasonable approach for per-request authentication.
103-105: LGTM! Headers are properly threaded through the call chain.The headers parameter flows correctly from
_parse_openai_docs→_create_dynamic_method→fetch_response, ensuring auth headers are included in proxy requests.Also applies to: 163-163, 187-187
59-59: LGTM! Type annotation improvement.Adding the
BaseAdaptertype annotation improves code clarity and enables better IDE support.tests/api/test_proxy.py (2)
11-26: LGTM!These test functions correctly exercise GET and POST proxy endpoints with query parameters and JSON payloads. The assertions appropriately verify the mocked responses.
34-43: LGTM!The authentication endpoint tests are correctly structured and verify the expected response format for protected routes.
| params: list[tuple[str, Any]] = [ | ||
| (name, c_type) | ||
| for param in (body.get("parameters") or []) | ||
| if (name := param.get("name")) and (c_type := type_map.get("type")) | ||
| if (name := param.get("name")) | ||
| and (typ := param.get("schema").get("type")) | ||
| and (c_type := type_map.get(typ)) | ||
| ] |
There was a problem hiding this comment.
Potential AttributeError if parameter schema is missing.
Line 82 chains .get("schema").get("type") which will raise AttributeError if schema key is missing (returning None, then calling .get() on None).
🔎 Apply this diff to safely handle missing schema:
params: list[tuple[str, Any]] = [
(name, c_type)
for param in (body.get("parameters") or [])
if (name := param.get("name"))
- and (typ := param.get("schema").get("type"))
+ and (schema := param.get("schema"))
+ and (typ := schema.get("type"))
and (c_type := type_map.get(typ))
]🤖 Prompt for AI Agents
In src/framex/plugins/proxy/__init__.py around lines 78 to 84, the list
comprehension chains param.get("schema").get("type") which will raise
AttributeError when "schema" is missing; update the comprehension to safely
retrieve schema first (e.g., schema = param.get("schema") or {} or use
param.get("schema") and guard), then get type from that schema, and only include
the parameter if name, typ, and mapped c_type exist; ensure you filter out
params with missing schema/type so no .get() is called on None.
| def is_white_url(self, url: str) -> bool: | ||
| """Check if a URL is protected by any auth_urls rule.""" | ||
| if self.white_list == []: | ||
| return True | ||
| for rule in self.white_list: | ||
| if rule == url: | ||
| return True | ||
| if rule.endswith("/*") and url.startswith(rule[:-1]): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Fix misleading docstring.
The docstring incorrectly states "Check if a URL is protected by any auth_urls rule" but this method checks the white_list, not auth_urls. This appears to be copy-pasted from the similar method in AuthConfig.
🔎 Apply this diff to fix the docstring:
def is_white_url(self, url: str) -> bool:
- """Check if a URL is protected by any auth_urls rule."""
+ """Check if a URL is allowed by the white_list rules."""
if self.white_list == []:
return True📝 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.
| def is_white_url(self, url: str) -> bool: | |
| """Check if a URL is protected by any auth_urls rule.""" | |
| if self.white_list == []: | |
| return True | |
| for rule in self.white_list: | |
| if rule == url: | |
| return True | |
| if rule.endswith("/*") and url.startswith(rule[:-1]): | |
| return True | |
| return False | |
| def is_white_url(self, url: str) -> bool: | |
| """Check if a URL is allowed by the white_list rules.""" | |
| if self.white_list == []: | |
| return True | |
| for rule in self.white_list: | |
| if rule == url: | |
| return True | |
| if rule.endswith("/*") and url.startswith(rule[:-1]): | |
| return True | |
| return False |
🤖 Prompt for AI Agents
In src/framex/plugins/proxy/config.py around lines 19 to 28, the method
is_white_url currently has a misleading docstring saying it checks "auth_urls"
rules; update the docstring to accurately describe that this method checks the
white_list (returns True if the URL is allowed by the white_list, including the
behavior when white_list is empty) and briefly document the matching rules
(exact match and prefix match for rules ending with "/*").
| def test_get_proxy_version(client: TestClient): | ||
| res = client.get(f"{API_STR}/proxy/mock/info").json() | ||
| assert res == {"info": "i_am_mock_proxy_info"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if test file exists and show its content
echo "=== test_proxy.py content ==="
if [ -f "tests/api/test_proxy.py" ]; then
cat -n tests/api/test_proxy.py
else
echo "File not found"
find . -name "test_proxy.py" 2>/dev/null | head -5
fi
echo ""
echo "=== API_STR definition ==="
rg -n "^API_STR\s*=" --type=py
echo ""
echo "=== TestClient setup in conftest ==="
find . -name "conftest.py" -type f | head -3 | xargs cat -n 2>/dev/null | grep -A 10 -B 10 "TestClient\|base_url\|client"Repository: touale/FrameX-kit
Length of output: 3127
🏁 Script executed:
#!/bin/bash
# Check how the proxy routes are registered
echo "=== Proxy route definitions ==="
find . -path ./node_modules -prune -o -name "*.py" -type f -exec grep -l "proxy.*mock" {} \; | head -10
# Look for route registration patterns
echo ""
echo "=== Looking for @app.get/@app.post with proxy ==="
rg -A 3 "@.*\.(get|post).*proxy" --type=py
# Check main app setup
echo ""
echo "=== Main app/FastAPI setup ==="
find . -name "main.py" -o -name "app.py" | head -3 | xargs cat -n 2>/dev/null | grep -A 5 -B 5 "FastAPI\|/api\|API_STR"Repository: touale/FrameX-kit
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Check what's in tests/consts.py and tests/mock.py
echo "=== tests/consts.py ==="
cat tests/consts.py 2>/dev/null | head -30
echo ""
echo "=== tests/mock.py (first 100 lines) ==="
head -100 tests/mock.py 2>/dev/null
# Look for where the test app is configured
echo ""
echo "=== Looking for app configuration / routers ==="
rg "include_router|APIRouter|@app\." --type=py -B 2 -A 2 | head -50
# Check framex module structure
echo ""
echo "=== framex module files ==="
find src/framex -name "*.py" -type f | head -20Repository: touale/FrameX-kit
Length of output: 4551
🏁 Script executed:
#!/bin/bash
# Check if there's a black_get handler in mock.py
echo "=== Searching for black_get handler ==="
rg "black_get" --type=py
# Check the full mock_request function to see all handlers
echo ""
echo "=== Full mock_request function ==="
sed -n '/^async def mock_request/,/^async def\|^def\|^class/p' tests/mock.py | head -100
# See if there are other test fixtures or setup
echo ""
echo "=== Tests conftest (full file) ==="
cat tests/conftest.pyRepository: touale/FrameX-kit
Length of output: 4081
Fix URL path inconsistency and add missing black_get mock handler.
Line 7 uses f"{API_STR}/proxy/mock/info" while all other test calls use bare paths like /proxy/mock/get (lines 13, 19, 25, 30, 36, 42). Standardize to one format.
Additionally, test_get_proxy_black_get expects a JSON response with res["status"] == 404, but the mock handler in tests/mock.py doesn't have a case for /proxy/mock/black_get—it will raise AssertionError instead. Add the missing handler to mock_request to return {"status": 404}.
🤖 Prompt for AI Agents
In tests/api/test_proxy.py around lines 6 to 8, change the GET call to use the
bare path "/proxy/mock/info" (to match the other tests that use bare paths like
"/proxy/mock/get") instead of f"{API_STR}/proxy/mock/info"; and in tests/mock.py
add a branch in the mock_request handler for the path "/proxy/mock/black_get"
that returns a JSON response {"status": 404} so test_get_proxy_black_get
receives the expected 404 payload (ensure the new branch is consistent with
existing mock response patterns).
| def test_get_proxy_black_get(client: TestClient): | ||
| res = client.get("/proxy/mock/black_get").json() | ||
| assert res["status"] == 404 |
There was a problem hiding this comment.
Fix status code assertion.
The test calls .json() and then checks res["status"] == 404. HTTP status codes should be checked via response.status_code, not from the JSON body. Additionally, calling .json() on a 404 response may fail if no JSON body is returned.
🔎 Apply this diff to fix the status code check:
def test_get_proxy_black_get(client: TestClient):
- res = client.get("/proxy/mock/black_get").json()
- assert res["status"] == 404
+ res = client.get("/proxy/mock/black_get")
+ assert res.status_code == 404🤖 Prompt for AI Agents
In tests/api/test_proxy.py around lines 29 to 31, the test currently calls
.json() on the response and asserts res["status"] == 404; change it to call
client.get(...) and assert the HTTP status via response.status_code (e.g. resp =
client.get("/proxy/mock/black_get"); assert resp.status_code == 404) and only
call resp.json() if you need to inspect the body after verifying the status to
avoid JSON parsing errors on 404 responses.
| elif url.endswith("/proxy/mock/auth/sget") and method == "GET": | ||
| if headers.get("Authorization") != "i_am_proxy_special_auth_keys": | ||
| resp.json.return_value = { | ||
| "status": 401, | ||
| "message": f"Invalid API Key({headers.get('Authorization')}) for API(/api/v1/proxy/mock/auth/get)", | ||
| } | ||
| else: | ||
| resp.json.return_value = { | ||
| "method": "GET", | ||
| "params": params, | ||
| } |
There was a problem hiding this comment.
Fix copy-paste error in error message.
The error message on line 60 incorrectly references /api/v1/proxy/mock/auth/get instead of /api/v1/proxy/mock/auth/sget for the sget endpoint.
🔎 Apply this diff to fix the error message:
elif url.endswith("/proxy/mock/auth/sget") and method == "GET":
if headers.get("Authorization") != "i_am_proxy_special_auth_keys":
resp.json.return_value = {
"status": 401,
- "message": f"Invalid API Key({headers.get('Authorization')}) for API(/api/v1/proxy/mock/auth/get)",
+ "message": f"Invalid API Key({headers.get('Authorization')}) for API(/api/v1/proxy/mock/auth/sget)",
}🤖 Prompt for AI Agents
In tests/mock.py around lines 56 to 66, the error response for the sget endpoint
contains a copy-paste mistake referencing "/api/v1/proxy/mock/auth/get"; update
the error message string to reference the correct endpoint
"/api/v1/proxy/mock/auth/sget" so the reported path matches the actual URL being
checked when Authorization fails.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/framex/plugins/proxy/__init__.py (1)
78-84: The AttributeError risk flagged in the previous review remains unresolved.Line 82 still chains
param.get("schema").get("type")without checking ifschemaisNone. If theschemakey is missing from a parameter, this will raiseAttributeErrorwhen calling.get("type")onNone.The fix suggested in the previous review should be applied:
🔎 Apply this diff to safely handle missing schema:
params: list[tuple[str, Any]] = [ (name, c_type) for param in (body.get("parameters") or []) if (name := param.get("name")) - and (typ := param.get("schema").get("type")) + and (schema := param.get("schema")) + and (typ := schema.get("type")) and (c_type := type_map.get(typ)) ]
🧹 Nitpick comments (1)
src/framex/plugins/proxy/__init__.py (1)
98-98: Remove unused# noqadirective.Static analysis indicates the
# noqacomment on this line is no longer needed. Consider removing it to keep the code clean.🔎 Apply this diff:
- Model = create_pydantic_model(schema_name, model_schema, components) # noqa + Model = create_pydantic_model(schema_name, model_schema, components)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/framex/plugins/proxy/__init__.py(6 hunks)src/framex/plugins/proxy/builder.py(1 hunks)src/framex/plugins/proxy/config.py(2 hunks)tests/test_plugins.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/framex/plugins/proxy/config.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/framex/plugins/proxy/__init__.py (4)
src/framex/adapter/base.py (1)
BaseAdapter(20-68)src/framex/plugins/proxy/config.py (1)
is_white_url(19-28)src/framex/config.py (1)
get_auth_keys(75-99)src/framex/plugins/proxy/builder.py (1)
create_pydantic_model(70-93)
tests/test_plugins.py (1)
src/framex/plugins/proxy/builder.py (1)
resolve_annotation(19-49)
🪛 Ruff (0.14.8)
src/framex/plugins/proxy/__init__.py
96-96: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
🔇 Additional comments (9)
src/framex/plugins/proxy/__init__.py (5)
11-11: LGTM! Type annotation support added.The
BaseAdapterimport is correctly used for type annotation on line 59, improving type safety.
59-59: LGTM! Type annotation improves code clarity.The explicit type annotation for the adapter variable enhances type safety and code readability.
65-74: Auth header logic is correct.The implementation correctly:
- Checks URL whitelist status before processing
- Retrieves auth keys using
AuthConfig.get_auth_keys(path)- Constructs Authorization header using the first key when available
- Sets headers to None for unprotected paths
The walrus operator ensures that if
get_auth_keysreturnsNoneor an empty list (edge case with misconfiguration), headers will beNone, which is acceptable behavior.
103-105: LGTM! Headers parameter correctly integrated.The
headersparameter is properly:
- Added to the
_create_dynamic_methodsignature with appropriate type annotation and default value- Passed from the calling code with the auth-derived headers
- Propagated through to
fetch_response(line 187)This enables per-path authorization as intended by the PR.
Also applies to: 163-163
187-187: LGTM! Headers correctly propagated to HTTP client.The headers are properly passed to
fetch_response, which forwards them to the underlyinghttpxclient for both streaming and non-streaming requests.tests/test_plugins.py (3)
65-65: Good addition of type annotation.The explicit type hint improves code clarity and enables better static type checking.
110-115: Excellent test coverage for anyOf union types.This test case validates that
resolve_annotationcorrectly handles OpenAPIanyOfschemas by creating Union types, specifically ensuring that an anyOf with array and string types resolves tolist[str] | str.
116-124: Strong test coverage for arrays of referenced models.This test case validates recursive type resolution when arrays contain
$refpointers to complex models, ensuring thatresolve_annotationcorrectly handles nested model references within array types.src/framex/plugins/proxy/builder.py (1)
89-89: Remove# pragma: no coveror add integration test for optional fields without explicit defaults.The
# pragma: no coverannotation hides a reachable code path. In OpenAPI schemas, optional fields (not in the "required" list) without explicit "default" keys are valid. Theresolve_default()function is tested in isolation but not when called from this code path. If external API schemas can contain such fields, this fallback behavior should be tested rather than excluded from coverage.
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.