Adding HandlerToken class #93
Conversation
tmikula-dev
requested review from
ABMC831,
Zejnilovic,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
WalkthroughAdds a HandlerTopic class to centralize topic schema loading, listing, schema retrieval, and message posting; refactors the Lambda entrypoint to route requests and delegate topic/token logic to handlers; adds build_error_response utility; updates and adds tests; bumps Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Lambda
participant HandlerTopic
participant HandlerToken
participant Validator
participant Writers
Client->>Lambda: POST /topics/{topic} (Authorization, body)
Lambda->>HandlerTopic: post_topic_message(topic, body, token)
HandlerTopic->>HandlerToken: decode_jwt(token)
alt token invalid
HandlerToken-->>HandlerTopic: error
HandlerTopic-->>Lambda: 401
Lambda-->>Client: 401
else token valid
HandlerToken-->>HandlerTopic: claims
HandlerTopic->>HandlerTopic: check topic & authorization
alt not found / unauthorized
HandlerTopic-->>Lambda: 404 / 403
Lambda-->>Client: 404 / 403
else ok
HandlerTopic->>Validator: validate(body against schema)
alt invalid
Validator-->>HandlerTopic: validation error
HandlerTopic-->>Lambda: 400
Lambda-->>Client: 400
else valid
HandlerTopic->>Writers: dispatch (kafka, eventbridge, postgres)
par
Writers-->>HandlerTopic: result(s)
end
alt any writer failed
HandlerTopic-->>Lambda: 500 (aggregated errors)
Lambda-->>Client: 500
else all succeeded
HandlerTopic-->>Lambda: 202
Lambda-->>Client: 202
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements.txt (1)
13-17: Fix botocore/boto3 version conflict causing pipeline failure.The pinned
botocore==1.42.4is incompatible withboto3==1.40.25, which requiresbotocore>=1.40.25,<1.41.0. This causes the dependency resolution to fail.Either remove the explicit botocore pin and let boto3 manage its dependency, or upgrade both packages to compatible versions:
boto3==1.40.25 confluent-kafka==2.12.1 # psycopg2-binary==2.9.10 # Ideal for local development, but not for long-term production use psycopg2==2.9.10 -botocore==1.42.4Or upgrade to compatible versions:
-boto3==1.40.25 +boto3==1.42.4 confluent-kafka==2.12.1 # psycopg2-binary==2.9.10 # Ideal for local development, but not for long-term production use psycopg2==2.9.10 botocore==1.42.4
🧹 Nitpick comments (5)
src/event_gate_lambda.py (1)
125-127: Uselogging.exceptionto include stack trace.When logging errors from caught exceptions,
logging.exceptionautomatically includes the stack trace, which aids debugging.except (KeyError, json.JSONDecodeError, ValueError, AttributeError, TypeError, RuntimeError) as request_exc: - logger.error("Request processing error: %s", request_exc) + logger.exception("Request processing error: %s", request_exc) return build_error_response(500, "internal", "Unexpected server error")src/handlers/handler_topic.py (3)
43-47: Inconsistent type annotation style.Line 43 mixes
Dictfromtypingwith built-inlist. For consistency, use either all typing module types or all built-in generics (Python 3.9+).- def __init__(self, conf_dir: str, access_config: Dict[str, list[str]], handler_token: HandlerToken): + def __init__(self, conf_dir: str, access_config: Dict[str, List[str]], handler_token: HandlerToken):And add
Listto the import:-from typing import Dict, Any +from typing import Dict, Any, List
49-65: Hardcoded topic-to-file mapping limits extensibility.The topic names and corresponding filenames are hardcoded. Consider making this configurable or deriving topic names from filenames dynamically.
This is acceptable for the current scope, but for future extensibility, consider a mapping configuration or file discovery pattern.
114-117: Handle empty/None token before decoding.If
token_encodedisNoneor empty string,decode_jwtmay raise an unexpected exception or produce unclear errors. Consider explicit validation.+ if not token_encoded: + return build_error_response(401, "auth", "Invalid or missing token") + try: token: Dict[str, Any] = self.handler_token.decode_jwt(token_encoded) except jwt.PyJWTError: # type: ignore[attr-defined] return build_error_response(401, "auth", "Invalid or missing token")tests/handlers/test_handler_topic.py (1)
38-42: Minor: Unused function parameters.The
argsandkwargsparameters are unused. While harmless, they can be removed or prefixed with underscore to suppress linter warnings.- def mock_open_side_effect(file_path, *args, **kwargs): + def mock_open_side_effect(file_path, *_args, **_kwargs):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
requirements.txt(2 hunks)src/event_gate_lambda.py(5 hunks)src/handlers/handler_topic.py(1 hunks)src/utils/utils.py(1 hunks)tests/handlers/test_handler_token.py(6 hunks)tests/handlers/test_handler_topic.py(1 hunks)tests/test_event_gate_lambda.py(2 hunks)tests/utils/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/event_gate_lambda.py (3)
src/handlers/handler_topic.py (5)
HandlerTopic(38-154)load_topic_schemas(49-65)get_topics_list(67-78)get_topic_schema(80-97)post_topic_message(99-154)src/utils/utils.py (1)
build_error_response(22-42)src/handlers/handler_token.py (1)
extract_token(141-173)
tests/utils/test_utils.py (1)
src/utils/utils.py (1)
build_error_response(22-42)
src/handlers/handler_topic.py (2)
src/handlers/handler_token.py (1)
decode_jwt(113-131)src/utils/utils.py (1)
build_error_response(22-42)
tests/handlers/test_handler_topic.py (3)
src/handlers/handler_topic.py (2)
HandlerTopic(38-154)load_topic_schemas(49-65)tests/conftest.py (2)
make_event(125-137)valid_payload(141-149)src/event_gate_lambda.py (1)
lambda_handler(94-127)
tests/test_event_gate_lambda.py (1)
src/event_gate_lambda.py (1)
lambda_handler(94-127)
tests/handlers/test_handler_token.py (2)
tests/conftest.py (1)
event_gate_module(34-121)src/event_gate_lambda.py (1)
lambda_handler(94-127)
🪛 GitHub Actions: Python Check
requirements.txt
[error] 13-13: pip install -r requirements.txt failed due to dependency conflict: botocore==1.42.4 is incompatible with boto3==1.40.25 (boto3 requires botocore<1.41.0 and >=1.40.25). ResolutionImpossible: conflicting dependencies prevent installation.
🪛 Ruff (0.14.7)
src/event_gate_lambda.py
126-126: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/handlers/test_handler_topic.py
38-38: Unused function argument: args
(ARG001)
38-38: Unused function argument: kwargs
(ARG001)
42-42: Avoid specifying long messages outside the exception class
(TRY003)
tests/handlers/test_handler_token.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (15)
tests/utils/test_utils.py (1)
22-35: LGTM!The test comprehensively validates the response structure including status code, headers, and body contents. Consider adding edge-case tests (e.g., empty strings, special characters in messages) in a follow-up if needed.
src/utils/utils.py (1)
22-42: LGTM!Clean utility function with proper typing and documentation. The standardized error response structure aligns well with API Gateway Lambda Proxy integration requirements.
tests/handlers/test_handler_token.py (2)
35-60: LGTM!Test correctly updated to use single-argument
lambda_handlersignature and properly patcheshandler_token.decode_jwtwith the appropriatejwt.ExpiredSignatureError.
63-91: LGTM!JWT decoding tests properly updated to patch
jwt.decodedirectly and usejwt.PyJWTErrorfor exception handling. The test scenarios cover both partial success (second key succeeds) and complete failure (all keys fail) cases appropriately.src/event_gate_lambda.py (1)
85-86: LGTM!Clean initialization of
HandlerTopicwith proper dependency injection ofCONF_DIR,ACCESS, andhandler_token. The fluent.load_topic_schemas()call keeps initialization concise.tests/test_event_gate_lambda.py (4)
21-27: LGTM!Test correctly updated to use single-argument
lambda_handlerand validates 404 response for unknown routes.
29-33: LGTM!API endpoint test properly updated for single-argument invocation.
36-42: LGTM!Patch target correctly updated to
handler_topic.get_topics_listto align with the new HandlerTopic-based architecture.
45-57: LGTM!Test correctly verifies that invalid JSON body triggers the internal error path (500) via the exception handler in
lambda_handler.src/handlers/handler_topic.py (3)
131-148: Partial write risk with sequential writer calls.If
writer_kafka.writesucceeds butwriter_eventbridge.writefails, the message is partially persisted with no rollback. This can lead to data inconsistency across systems.Is partial write acceptable for this use case? If not, consider:
- Implementing compensating transactions/rollback
- Using an outbox pattern for reliable delivery
- At minimum, documenting this behavior as a known limitation
67-78: LGTM!Simple and correct implementation returning available topics as JSON.
80-97: LGTM!Correctly returns schema for existing topics or 404 for unknown topics.
tests/handlers/test_handler_topic.py (3)
26-52: LGTM!Thorough test for schema loading with proper mocking of file I/O and validation of loaded topic keys.
55-76: LGTM!Good coverage for
get_topics_listandget_topic_schemaincluding the 404 case.
80-134: LGTM!Comprehensive auth and validation failure tests covering missing token, unauthorized user, schema validation errors, and invalid JWT decode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/event_gate_lambda.py (2)
94-104: Prefix unusedcontextparameter with underscore.AWS Lambda requires the
contextparameter, but since it's unused here, prefixing with underscore signals intentional non-use and silences the linter warning.-def lambda_handler(event: Dict[str, Any], context: Any = None) -> Dict[str, Any]: +def lambda_handler(event: Dict[str, Any], _context: Any = None) -> Dict[str, Any]: """ AWS Lambda entry point. Dispatches based on API Gateway proxy 'resource' and 'httpMethod'. Args: event: The event data from API Gateway. - context: The mandatory context argument for AWS Lambda invocation. + _context: The mandatory context argument for AWS Lambda invocation (unused). Returns: A dictionary compatible with API Gateway Lambda Proxy integration. Raises: Request exception: For unexpected errors. """
126-128: Uselogger.exceptionto capture stack trace.
logging.exceptionautomatically includes the traceback, which is valuable for debugging unexpected server errors in production logs.except (KeyError, json.JSONDecodeError, ValueError, AttributeError, TypeError, RuntimeError) as request_exc: - logger.error("Request processing error: %s", request_exc) + logger.exception("Request processing error: %s", request_exc) return build_error_response(500, "internal", "Unexpected server error")tests/handlers/test_handler_topic.py (1)
37-41: Minor: Unusedargsandkwargsin mock function signature.These parameters exist for signature compatibility but are unused. This is acceptable for mock functions, though you could remove them if not needed for compatibility.
- def mock_open_side_effect(file_path, *args, **kwargs): + def mock_open_side_effect(file_path, *_args, **_kwargs): for filename, schema in mock_schemas.items(): if filename in file_path: return mock_open(read_data=json.dumps(schema)).return_value - raise FileNotFoundError(f"File not found: {file_path}") + raise FileNotFoundError(file_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/CODEOWNERS(1 hunks)requirements.txt(1 hunks)src/event_gate_lambda.py(5 hunks)tests/handlers/test_handler_topic.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (2)
tests/handlers/test_handler_topic.py (3)
src/handlers/handler_topic.py (2)
HandlerTopic(38-154)load_topic_schemas(49-65)tests/conftest.py (2)
make_event(125-137)valid_payload(141-149)src/event_gate_lambda.py (1)
lambda_handler(94-128)
src/event_gate_lambda.py (3)
src/handlers/handler_topic.py (5)
HandlerTopic(38-154)load_topic_schemas(49-65)get_topics_list(67-78)get_topic_schema(80-97)post_topic_message(99-154)src/utils/utils.py (1)
build_error_response(22-42)src/handlers/handler_token.py (1)
extract_token(141-173)
🪛 Ruff (0.14.7)
tests/handlers/test_handler_topic.py
37-37: Unused function argument: args
(ARG001)
37-37: Unused function argument: kwargs
(ARG001)
41-41: Avoid specifying long messages outside the exception class
(TRY003)
src/event_gate_lambda.py
94-94: Unused function argument: context
(ARG001)
127-127: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (10)
.github/CODEOWNERS (1)
1-1: Verify intentional removal of current reviewers from CODEOWNERS.This change removes
@ABMC831and@Zejnilovicfrom code ownership, yet both are listed as reviewers on this PR. Additionally, this ownership change is not mentioned in the PR description or objectives, which focus on adding a HandlerTopic class.Please confirm:
- The removal of these reviewers is intentional (e.g., team restructuring)
- This CODEOWNERS update should be included in this PR rather than submitted separately
Note: There's also an inconsistency between the PR title ("Adding HandlerToken class") and the PR description/linked issue #91 ("Add HandlerTopic Class"). Please clarify which is correct.
src/event_gate_lambda.py (3)
35-49: Configuration and logger initialization looks good.The explicit debug logging for configuration loading aids troubleshooting. The fallback warning for invalid
CONF_DIRis helpful for diagnosing deployment issues.
85-86: HandlerTopic initialization is clean.Fluent chaining of
load_topic_schemas()keeps initialization concise and readable.
105-125: Routing logic is well-structured.The resource-based dispatch cleanly separates concerns, delegating topic operations to
HandlerTopicand usingbuild_error_responsefor consistent error formatting.tests/handlers/test_handler_topic.py (6)
26-50: Good test coverage for schema loading.The mock setup correctly simulates file I/O and validates all three topic schemas are loaded with proper keys.
53-74: Comprehensive tests for topic listing and schema retrieval.Both found and not-found cases are covered for
get_topic_schema, and the topic list test validates expected content.
77-133: Auth and validation error paths are well tested.Tests cover missing token (401), unauthorized user (403), schema validation failure (400), and invalid token decode (401) - all with appropriate error type assertions.
137-155: Correct patch targets for writers.The patches correctly target
src.handlers.handler_topic.writer_*where the writers are imported and used.
158-197: Writer failure aggregation tests are thorough.Tests verify single and multiple writer failures return 500 status with properly aggregated error types.
200-215: Correct lowercase Authorization header test.The test now uses
{"authorization": "bearer token"}which properly validates case-insensitive header handling per theextract_tokenimplementation.
tmikula-dev
added
the
refactoring
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/event_gate_lambda.py (2)
35-41: Module‑level config/access loading is fine; consider fail‑fast clarityLoading
config.json,api.yaml, andACCESSat import time is reasonable for a Lambda cold‑start and keeps the handler hot‑path simple. If you ever need more graceful degradation (e.g., clearer error whenaccess_configis missing/malformed), you could:
- Use
config.get("access_config")with an explicitRuntimeErrorif missing.- Optionally wrap the config/API file reads in a small helper that logs a clear path and re‑raises.
Not urgent, but might improve diagnosability if configs are mis‑deployed.
Also applies to: 43-50, 51-55, 65-75
126-128: Simplifylogger.exceptioncall to avoid redundant exception formatting
logger.exceptionalready logs the current exception and stack trace; passingrequest_excas a%sargument is redundant and triggers Ruff TRY401.You can simplify to:
- except (KeyError, json.JSONDecodeError, ValueError, AttributeError, TypeError, RuntimeError) as request_exc: - logger.exception("Request processing error: %s", request_exc) + except (KeyError, json.JSONDecodeError, ValueError, AttributeError, TypeError, RuntimeError): + logger.exception("Request processing error")Functionality is unchanged, and the log still includes the full exception details.
tests/handlers/test_handler_topic.py (1)
136-215: Writer aggregation and token-extraction tests look correct; consider factoring repeated patchesThe writer‑related tests:
test_post_success_all_writerstest_post_single_writer_failuretest_post_multiple_writer_failurestest_token_extraction_lowercase_bearer_headercorrectly:
- Patch writer calls on
src.handlers.handler_topic.*(matching the handler’s imports).- Assert the right status codes (
202/500) and writertypeaggregation behavior.- Verify that a lowercase
authorization: bearer ...header still yields a successful flow.You might reduce duplication by extracting a small helper or fixture that sets up the common
decode_jwtand writer patches, but this is purely a cleanliness refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/event_gate_lambda.py(5 hunks)tests/handlers/test_handler_topic.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/event_gate_lambda.py (3)
src/handlers/handler_topic.py (5)
HandlerTopic(38-154)load_topic_schemas(49-65)get_topics_list(67-78)get_topic_schema(80-97)post_topic_message(99-154)src/utils/utils.py (1)
build_error_response(22-42)src/handlers/handler_token.py (1)
extract_token(141-173)
tests/handlers/test_handler_topic.py (1)
src/handlers/handler_topic.py (2)
HandlerTopic(38-154)load_topic_schemas(49-65)
🪛 Ruff (0.14.7)
src/event_gate_lambda.py
127-127: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (4)
src/event_gate_lambda.py (2)
28-32: HandlerTopic wiring and centralized topic initialization look consistentThe new
HandlerTopic(CONF_DIR, ACCESS, handler_token).load_topic_schemas()wiring matches the handler’s API and cleanly centralizes topic concerns away from the lambda entrypoint. Using the same writer modules that are initialized above keeps the integration straightforward.Also applies to: 85-86
94-122: lambda_handler routing and standardized error responses look goodThe updated
lambda_handlercorrectly:
- Accepts the AWS
contextparameter while keeping it unused (_context).- Routes by
resourceandhttpMethod, delegating topic/token concerns toHandlerTopic/HandlerToken.- Normalizes topic names to lowercase, which aligns with the configured topic keys.
- Uses
build_error_responsefor unknown routes, keeping error payloads consistent withHandlerTopic.This keeps the entrypoint thin and focused on routing.
Also applies to: 124-125
tests/handlers/test_handler_topic.py (2)
26-50: Schema loading test correctly exercises HandlerTopic.load_topic_schemasThe
mock_open_side_effectsetup neatly simulates three distinct topic schema files and validates thatHandlerTopic.topicsends up with the expected keys. This gives good confidence thatload_topic_schemasis wired to the right filenames and topic names.
53-75: GET and POST auth/validation tests provide solid coverage of topic endpointsThe tests around:
/topicsand/topics/{topic_name}(found/not found), and- POST failures (missing token, unauthorized user, schema validation error, invalid token decode)
thoroughly check the routing plus the main error paths exposed by
HandlerTopic.post_topic_message, including correct HTTP status codes and errortypevalues (auth,validation,topic). This is a good, focused regression net for the new handler flow.Also applies to: 79-134
oto-macenauer-absa
left a comment
oto-macenauer-absa
left a comment
There was a problem hiding this comment.
lgtm, just move the write init if possible
| handler_token = HandlerToken(config).load_public_keys() | ||
|
|
||
| # Initialize EventGate writers | ||
| writer_eventbridge.init(logger, config) |
There was a problem hiding this comment.
should the init of the writes rather go to the handler_topic? this might go to the constructor of the handler
There was a problem hiding this comment.
Based on this comment, I tried to implement the suggested change correctly. But found out, that the refactoring need has a bigger impact. So I wrote a new issue based on this comment, that will be implemented separately. #94
Overview
This pull request refactors topic management and error handling in the event gate Lambda, moving topic-related logic into a new
HandlerTopicclass and standardizing error responses. It also improves configuration loading and updates related tests for consistency.Release Notes:
Related
Closes #91
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.