Open
Conversation
tmikula-dev
requested review from
lsulak,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds a new read-only Event Stats Lambda with a PostgreSQL reader (keyset pagination), config loaders, HandlerStats, and tests; extends API spec with POST /stats/{topic_name} and ErrorResponse; refactors routing to a central dispatch, updates type hints across handlers/writers/readers, and deletes multiple .github agent docs and the AquaSec workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIGateway
participant EventStatsLambda
participant HandlerStats
participant HandlerToken
participant ReaderPostgres
participant PostgreSQL
Client->>APIGateway: POST /stats/{topic_name} (with JWT)
APIGateway->>EventStatsLambda: invoke lambda_handler(event)
EventStatsLambda->>HandlerStats: handle_request(event)
HandlerStats->>HandlerToken: extract_token(headers)
HandlerToken-->>HandlerStats: token_string
HandlerStats->>HandlerToken: decode_jwt(token_string)
HandlerToken-->>HandlerStats: token_payload
alt Unauthorized / Forbidden / NotFound
HandlerStats-->>EventStatsLambda: 401/403/404 response
else Valid request
HandlerStats->>ReaderPostgres: read_stats(filters, cursor, limit)
ReaderPostgres->>PostgreSQL: execute paginated SELECT
PostgreSQL-->>ReaderPostgres: rows
ReaderPostgres-->>HandlerStats: (data[], pagination)
HandlerStats-->>EventStatsLambda: 200 response with data & pagination
end
EventStatsLambda-->>APIGateway: lambda response
APIGateway-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+183
to
+188
| resource_id = aws_api_gateway_resource.event_gate_api_stats_topic_name.id | ||
| http_method = aws_api_gateway_method.event_gate_api_stats_topic_name_post.http_method | ||
| integration_http_method = "POST" | ||
| type = "AWS_PROXY" | ||
| uri = aws_lambda_function.event_stats_lambda.invoke_arn | ||
| } |
Check warning
Code scanning / AquaSec
API Gateway stages for V1 and V2 should have access logging enabled Medium
Comment on lines
+183
to
+188
| resource_id = aws_api_gateway_resource.event_gate_api_stats_topic_name.id | ||
| http_method = aws_api_gateway_method.event_gate_api_stats_topic_name_post.http_method | ||
| integration_http_method = "POST" | ||
| type = "AWS_PROXY" | ||
| uri = aws_lambda_function.event_stats_lambda.invoke_arn | ||
| } |
Check notice
Code scanning / AquaSec
API Gateway must have X-Ray tracing enabled Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/handler_topic.py (1)
166-177:⚠️ Potential issue | 🔴 CriticalStop fan-out after the first writer failure.
Once one writer returns
False, this loop still calls the remaining writers before returning 500. That creates a non-idempotent partial success: some backends may already have accepted the event, and a retry can duplicate those writes. At minimum, stop dispatching after the first failure.Minimum containment change
- errors = [] for writer_name, writer in self.writers.items(): ok, err = writer.write(topic_name, topic_message) if not ok: - errors.append({"type": writer_name, "message": err}) - - if errors: - return { - "statusCode": 500, - "headers": {"Content-Type": "application/json"}, - "body": json.dumps({"success": False, "statusCode": 500, "errors": errors}), - } + return { + "statusCode": 500, + "headers": {"Content-Type": "application/json"}, + "body": json.dumps( + { + "success": False, + "statusCode": 500, + "errors": [{"type": writer_name, "message": err}], + } + ), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/handler_topic.py` around lines 166 - 177, The loop over self.writers currently continues dispatching to all writers even after one writer.write(topic_name, topic_message) returns ok=False, which can cause partial, non-idempotent writes; change the logic in the handler that builds errors (the for loop using self.writers, writer.write and the errors list) to stop iterating on the first failure: as soon as writer.write returns not ok, append that single error and break the loop, then return the 500 response based on that error instead of calling remaining writers. Ensure you preserve the existing 500 response shape (statusCode, headers, body with errors) but only include the first failure.
🧹 Nitpick comments (6)
api.yaml (1)
336-353: Consider adoptingErrorResponseschema for existing endpoints.The new
ErrorResponseschema provides good standardization. For consistency, consider updating the existing error responses inPOST /topics/{topicName}(lines 145-180) and other endpoints to use$ref: '#/components/schemas/ErrorResponse'instead of inline schemas in a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api.yaml` around lines 336 - 353, Replace inline error response schemas for endpoints (start with the POST /topics/{topicName} operation) to reference the shared ErrorResponse schema; specifically update the responses sections that currently define inline objects to use $ref: '#/components/schemas/ErrorResponse' (and do the same for other endpoints with similar inline error shapes) so all error responses consistently point to the ErrorResponse schema defined under components.schemas; ensure response content/media types (e.g., application/json) remain and only the schema node is replaced with the $ref.src/utils/utils.py (1)
85-101: Consider handling Secrets Manager exceptions.If
get_secret_valuefails (e.g., secret not found, access denied, network issues), abotocore.exceptions.ClientErrorwill propagate unhandled. Depending on the desired behavior, you may want to catch and wrap this exception or document that callers must handle it.💡 Optional: Add exception handling
+from botocore.exceptions import ClientError + def load_postgres_config(secret_name: str, secret_region: str) -> dict[str, Any]: """Load PostgreSQL connection config from AWS Secrets Manager. Args: secret_name: Name or ARN of the secret. secret_region: AWS region where the secret is stored. Returns: Parsed connection dict with keys like database, host, user, password, port. Returns {"database": ""} when secret_name or secret_region is empty. + Raises: + RuntimeError: If secret retrieval fails. """ if not secret_name or not secret_region: return {"database": ""} - aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) - postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] - config: dict[str, Any] = json.loads(postgres_secret) - logger.debug("Loaded PostgreSQL config from Secrets Manager.") - return config + try: + aws_secrets = boto3.Session().client(service_name="secretsmanager", region_name=secret_region) + postgres_secret = aws_secrets.get_secret_value(SecretId=secret_name)["SecretString"] + config: dict[str, Any] = json.loads(postgres_secret) + logger.debug("Loaded PostgreSQL config from Secrets Manager.") + return config + except ClientError as exc: + logger.exception("Failed to load PostgreSQL config from Secrets Manager.") + raise RuntimeError("PostgreSQL config initialization failed") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/utils.py` around lines 85 - 101, load_postgres_config currently calls boto3.Session().client(...).get_secret_value(...) without handling AWS errors; wrap the secret fetch in a try/except that catches botocore.exceptions.ClientError (import it) around the aws_secrets.get_secret_value call, log the exception with logger.error including the exception details and context (e.g., secret_name/secret_region), and then either re-raise the original error or raise a clear wrapper (e.g., RuntimeError) so callers get a documented failure mode; ensure the function signature and return behavior remain the same otherwise.src/writers/writer_kafka.py (1)
23-23: Minor inconsistency: mixingOptionaland union|syntax.Line 23 imports
Optionalwhich is used on line 44 (Optional["Producer"]), but other signatures use the|union syntax (e.g.,Producer | Noneon line 47). Consider using consistent union syntax throughout for Python 3.9+ style:- self._producer: Optional["Producer"] = None + self._producer: "Producer | None" = NoneThen
Optionalcan be removed from imports.Also applies to: 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/writers/writer_kafka.py` at line 23, The import and type annotations are inconsistent: remove Optional from the import list and replace Optional["Producer"] in the WriterKafka class (or wherever referenced) with the modern union syntax Producer | None to match other annotations like Producer | None; ensure typing.Any remains imported and that forward references to "Producer" are preserved (use quotes only if needed) so all type hints use the same Python 3.9+ union style.tests/unit/readers/test_reader_postgres.py (1)
20-20: Prefermocker.patch(...)over nestedunittest.mock.patch(...)blocks.These unit tests are already on pytest, so switching the repeated patch contexts to
mocker.patch(...)will match the repo standard and make the fixtures less noisy.As per coding guidelines,
tests/**/*.py: Usemocker.patch("module.dependency")ormocker.patch.object(Class, "method")for mocking in tests.Also applies to: 60-76, 167-174, 212-219, 258-265, 362-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/readers/test_reader_postgres.py` at line 20, Replace uses of unittest.mock.patch and nested patch contexts in tests/unit/readers/test_reader_postgres.py (currently imported as MagicMock and patch) with pytest-mock's fixture style mocker.patch or mocker.patch.object; update the import to remove patch and MagicMock if not needed or keep MagicMock only for instance creation, then for each patched dependency referenced in the tests (the locations flagged at lines ~60-76, 167-174, 212-219, 258-265, 362-364) switch from context-manager patch(...) blocks to calls like mocker.patch("module.dependency") or mocker.patch.object(SomeClass, "method") inside the test functions so mocking follows the repo guideline and reduces nested patch noise.tests/unit/utils/test_config_loader.py (1)
70-97: Usemocker.patchfor dependency mocking in unit tests.These tests currently build mocks with
MagicMock; please align with the repo’smocker.patch(...)pattern for consistency and easier fixture composition.As per coding guidelines: "Use
mocker.patch("module.dependency")ormocker.patch.object(Class, "method")for mocking in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/test_config_loader.py` around lines 70 - 97, Update the two tests to use the pytest-mock fixture pattern instead of creating MagicMock instances directly: in test_loads_from_local_file keep calling load_config(conf_dir) and then assert load_access_config(config, aws_s3) behavior but replace the local aws_s3 MagicMock with a mocker.patch of the module dependency used by load_access_config (e.g., mocker.patch("module_under_test.aws_s3_client") or the specific symbol load_access_config imports), and in test_loads_from_s3 patch the S3 client with mocker.patch to return a fake Bucket/Object/get result whose Body.read returns the encoded JSON (use mocker.patch to stub the bucket factory and its Object().get() chain so you can assert mock calls like Bucket(...) and Object(...)); reference the test functions test_loads_from_local_file and test_loads_from_s3 and the function under test load_access_config when locating where to apply mocker.patch.tests/unit/test_event_stats_lambda.py (1)
30-99: Align fixture mocking withmocker.patchconvention.The module fixture currently manages a manual patch stack via
unittest.mock.patch; please switch tomocker.patch(...)/mocker.patch.object(...)for consistency with project tests.As per coding guidelines: "Use
mocker.patch("module.dependency")ormocker.patch.object(Class, "method")for mocking in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_event_stats_lambda.py` around lines 30 - 99, The event_stats_module fixture manually manages unittest.mock.patch and an ExitStack; convert it to use the pytest-mock `mocker` fixture instead: accept `mocker` as a parameter to `event_stats_module`, replace calls to the local `start_patch` helper with `mocker.patch("requests.get")`, `mocker.patch("cryptography.hazmat.primitives.serialization.load_der_public_key")`, `mocker.patch("boto3.Session")`, and use `mocker.patch.dict(sys.modules, {...})` for the conditional dummy module inserts instead of `patch.dict`; drop manual start/stop of patches and the ExitStack since mocker auto-tears-down; keep the same dummy classes/return values and importlib.import_module("src.event_stats_lambda") behavior and yield the module as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 113: Update the README entry for POSTGRES_SECRET_NAME so the documented
secret key names match the stats-reader contract: replace references to `dbname`
and `username` with `database` and `user` (and confirm `host`, `port`,
`password` remain), and note that the secret payload must include keys `host`,
`port`, `database`, `user`, and `password` to match the Lambda/stats-reader
expectations (ensure any mention of the Postgres writer/reader uses these exact
keys).
In `@src/event_stats_lambda.py`:
- Line 83: The docstring for the AWS Lambda entry point lambda_handler
incorrectly references "EventGate"; update the docstring in the lambda_handler
function to the correct service name "EventStats" (or another accurate service
identifier), ensuring the first line and any mention inside the triple-quoted
string are changed so the handler's documentation accurately reflects
EventStats.
In `@src/handlers/handler_stats.py`:
- Around line 83-104: The handler currently only validates
timestamp_start/timestamp_end/cursor/limit and calls reader_postgres.read_stats,
but the API must also accept filter and sort, support an opaque cursor (not raw
internal_id), and enforce a hard max of 1000 rows; update the body
parsing/validation to accept body.get("filter") and body.get("sort"), allow
cursor to be an opaque string (e.g., body.get("cursor")) and decode it to the
internal integer before calling read_stats (so you no longer accept or return
raw internal_id), and clamp limit to at most 1000 (e.g., limit = min(int(limit),
1000) while still validating positive int). Finally pass the new filter and sort
arguments into self.reader_postgres.read_stats (timestamp_start, timestamp_end,
cursor=<decoded_internal_id_or_none>, limit=<clamped_limit>, filter=filter,
sort=sort).
- Around line 77-86: The handler currently assumes the parsed JSON is an object
and directly calls body.get(...), which raises AttributeError for non-object
JSON like arrays or strings; after json.loads(...) in the request parsing block
(where variable body is assigned) add a type check to ensure body is a dict
(i.e., a JSON object) and if not return build_error_response(400, "validation",
"Request body must be a JSON object.") before accessing body.get for
timestamp_start, timestamp_end, cursor, limit; update any tests or callers that
rely on this behavior to expect a 400 for non-object JSON.
In `@src/readers/reader_postgres.py`:
- Around line 221-224: The try/except in check_health catches only
BotoCoreError/ClientError but _load_db_config can raise RuntimeError, allowing
the health check to propagate a 500; update the except in check_health to also
handle RuntimeError (either add RuntimeError to the exception tuple or add a
separate "except RuntimeError as err: return False, str(err)") so
_load_db_config failures return the degraded health tuple instead of raising.
- Line 136: Update the list comprehension that builds rows to use zip(...,
strict=True) to satisfy Ruff B905: change the zip call in the expression that
creates rows from col_names and raw_rows (the line assigning rows =
[dict(zip(col_names, row)) for row in raw_rows]) to include strict=True so
mismatched lengths would raise immediately; keep col_names and raw_rows as the
inputs to zip and preserve the dict(...) wrapping.
- Around line 122-130: The DB connection in reader_postgres.py created via
psycopg2.connect currently lacks a statement timeout and read-only enforcement;
update the with psycopg2.connect(...) block to set a server-side statement
timeout and enforce read-only mode by adding connection options (e.g.
options='-c statement_timeout=30000 -c default_transaction_read_only=on') to the
psycopg2.connect call and also call connection.set_session(readonly=True) after
the connection is opened (or execute "SET statement_timeout = ..." if you prefer
configurable ms from db_config). Target the psycopg2.connect invocation and the
connection object (connection.set_session) to implement this change.
In `@src/utils/config_loader.py`:
- Around line 78-90: The current hardcoded filename_to_topic mapping ignores any
additional schema files; replace that logic by scanning the topic_schemas
directory (schemas_dir) for all *.json files and generating topics dynamically
(e.g., for each schema file stem use topic_name = "public.cps.za." + stem)
instead of the filename_to_topic dict. Update the code that builds topics
(currently using filename_to_topic, schemas_dir, topics) to iterate over the
discovered files (os.listdir or glob.glob), ensure each path is a file
(os.path.isfile), derive the topic name from the file stem, and append it to
topics so all schema files produce a corresponding topic.
In `@src/writers/writer_postgres.py`:
- Around line 249-257: The write() method currently only checks for "database"
and psycopg2 presence before proceeding; mirror the same validation logic used
in check_health(): call _load_db_config(), verify host, user, password and port
are present and non-empty (and return the same (False, "<msg>") signature for
misconfiguration), and only then attempt to import/use psycopg2 and open a
connection; also apply the same guard around the later connection block (the
section corresponding to lines ~268-273) so you do not attempt a malformed
connection when secrets are partial or missing.
In `@terraform_examples/lambda.tf`:
- Around line 79-82: The stats Lambda's environment block only sets LOG_LEVEL
but must include the Postgres secret config used by the reader; update the
event_stats_lambda resource to add environment variables POSTGRES_SECRET_NAME
and POSTGRES_SECRET_REGION (populated from the same secret values used elsewhere
in the stack) so the reader can locate the DB credentials at runtime; ensure the
variables are added to the environment { variables = { ... } } map for the
event_stats_lambda so /stats can access the DB.
---
Outside diff comments:
In `@src/handlers/handler_topic.py`:
- Around line 166-177: The loop over self.writers currently continues
dispatching to all writers even after one writer.write(topic_name,
topic_message) returns ok=False, which can cause partial, non-idempotent writes;
change the logic in the handler that builds errors (the for loop using
self.writers, writer.write and the errors list) to stop iterating on the first
failure: as soon as writer.write returns not ok, append that single error and
break the loop, then return the 500 response based on that error instead of
calling remaining writers. Ensure you preserve the existing 500 response shape
(statusCode, headers, body with errors) but only include the first failure.
---
Nitpick comments:
In `@api.yaml`:
- Around line 336-353: Replace inline error response schemas for endpoints
(start with the POST /topics/{topicName} operation) to reference the shared
ErrorResponse schema; specifically update the responses sections that currently
define inline objects to use $ref: '#/components/schemas/ErrorResponse' (and do
the same for other endpoints with similar inline error shapes) so all error
responses consistently point to the ErrorResponse schema defined under
components.schemas; ensure response content/media types (e.g., application/json)
remain and only the schema node is replaced with the $ref.
In `@src/utils/utils.py`:
- Around line 85-101: load_postgres_config currently calls
boto3.Session().client(...).get_secret_value(...) without handling AWS errors;
wrap the secret fetch in a try/except that catches
botocore.exceptions.ClientError (import it) around the
aws_secrets.get_secret_value call, log the exception with logger.error including
the exception details and context (e.g., secret_name/secret_region), and then
either re-raise the original error or raise a clear wrapper (e.g., RuntimeError)
so callers get a documented failure mode; ensure the function signature and
return behavior remain the same otherwise.
In `@src/writers/writer_kafka.py`:
- Line 23: The import and type annotations are inconsistent: remove Optional
from the import list and replace Optional["Producer"] in the WriterKafka class
(or wherever referenced) with the modern union syntax Producer | None to match
other annotations like Producer | None; ensure typing.Any remains imported and
that forward references to "Producer" are preserved (use quotes only if needed)
so all type hints use the same Python 3.9+ union style.
In `@tests/unit/readers/test_reader_postgres.py`:
- Line 20: Replace uses of unittest.mock.patch and nested patch contexts in
tests/unit/readers/test_reader_postgres.py (currently imported as MagicMock and
patch) with pytest-mock's fixture style mocker.patch or mocker.patch.object;
update the import to remove patch and MagicMock if not needed or keep MagicMock
only for instance creation, then for each patched dependency referenced in the
tests (the locations flagged at lines ~60-76, 167-174, 212-219, 258-265,
362-364) switch from context-manager patch(...) blocks to calls like
mocker.patch("module.dependency") or mocker.patch.object(SomeClass, "method")
inside the test functions so mocking follows the repo guideline and reduces
nested patch noise.
In `@tests/unit/test_event_stats_lambda.py`:
- Around line 30-99: The event_stats_module fixture manually manages
unittest.mock.patch and an ExitStack; convert it to use the pytest-mock `mocker`
fixture instead: accept `mocker` as a parameter to `event_stats_module`, replace
calls to the local `start_patch` helper with `mocker.patch("requests.get")`,
`mocker.patch("cryptography.hazmat.primitives.serialization.load_der_public_key")`,
`mocker.patch("boto3.Session")`, and use `mocker.patch.dict(sys.modules, {...})`
for the conditional dummy module inserts instead of `patch.dict`; drop manual
start/stop of patches and the ExitStack since mocker auto-tears-down; keep the
same dummy classes/return values and
importlib.import_module("src.event_stats_lambda") behavior and yield the module
as before.
In `@tests/unit/utils/test_config_loader.py`:
- Around line 70-97: Update the two tests to use the pytest-mock fixture pattern
instead of creating MagicMock instances directly: in test_loads_from_local_file
keep calling load_config(conf_dir) and then assert load_access_config(config,
aws_s3) behavior but replace the local aws_s3 MagicMock with a mocker.patch of
the module dependency used by load_access_config (e.g.,
mocker.patch("module_under_test.aws_s3_client") or the specific symbol
load_access_config imports), and in test_loads_from_s3 patch the S3 client with
mocker.patch to return a fake Bucket/Object/get result whose Body.read returns
the encoded JSON (use mocker.patch to stub the bucket factory and its
Object().get() chain so you can assert mock calls like Bucket(...) and
Object(...)); reference the test functions test_loads_from_local_file and
test_loads_from_s3 and the function under test load_access_config when locating
where to apply mocker.patch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a21b433-b588-4e5f-9988-d21de26d987a
📒 Files selected for processing (55)
.github/agents/devops-engineer.agent.md.github/agents/reviewer.agent.md.github/agents/sdet.agent.md.github/agents/senior-developer.agent.md.github/agents/specification-master.agent.md.github/copilot-instructions.md.github/workflows/aquasec_repo_scan.ymlDEVELOPER.mdREADME.mdapi.yamlsrc/event_gate_lambda.pysrc/event_stats_lambda.pysrc/handlers/handler_api.pysrc/handlers/handler_health.pysrc/handlers/handler_stats.pysrc/handlers/handler_token.pysrc/handlers/handler_topic.pysrc/readers/__init__.pysrc/readers/reader_postgres.pysrc/utils/config_loader.pysrc/utils/constants.pysrc/utils/logging_levels.pysrc/utils/safe_serialization.pysrc/utils/trace_logging.pysrc/utils/utils.pysrc/writers/writer.pysrc/writers/writer_eventbridge.pysrc/writers/writer_kafka.pysrc/writers/writer_postgres.pyterraform_examples/api_gateway.tfterraform_examples/lambda.tfterraform_examples/variables.tftests/integration/conftest.pytests/integration/schemas/postgres_schema.pytests/integration/test_api_endpoint.pytests/integration/test_health_endpoint.pytests/integration/test_stats_endpoint.pytests/integration/test_token_endpoint.pytests/integration/test_topics_endpoint.pytests/unit/conftest.pytests/unit/handlers/test_handler_api.pytests/unit/handlers/test_handler_health.pytests/unit/handlers/test_handler_stats.pytests/unit/handlers/test_handler_token.pytests/unit/handlers/test_handler_topic.pytests/unit/readers/__init__.pytests/unit/readers/test_reader_postgres.pytests/unit/test_event_gate_lambda.pytests/unit/test_event_gate_lambda_local_access.pytests/unit/test_event_stats_lambda.pytests/unit/utils/test_conf_path.pytests/unit/utils/test_config_loader.pytests/unit/utils/test_safe_serialization.pytests/unit/utils/test_utils.pytests/unit/writers/test_writer_postgres.py
💤 Files with no reviewable changes (11)
- tests/integration/test_api_endpoint.py
- .github/agents/sdet.agent.md
- tests/integration/test_topics_endpoint.py
- tests/integration/test_health_endpoint.py
- .github/agents/senior-developer.agent.md
- .github/agents/devops-engineer.agent.md
- .github/workflows/aquasec_repo_scan.yml
- .github/agents/specification-master.agent.md
- tests/unit/utils/test_safe_serialization.py
- .github/agents/reviewer.agent.md
- tests/integration/test_token_endpoint.py
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/writers/writer_postgres.py (2)
268-270: End this log message with a period.The unknown-topic error text currently lacks terminal punctuation.
As per coding guidelines,
**/*.py: End all log messages with a period.✏️ Suggested tweak
- msg = f"Unknown topic for Postgres/{topic_name}" + msg = f"Unknown topic for Postgres/{topic_name}."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/writers/writer_postgres.py` around lines 268 - 270, The log message created when a topic is not found (see TOPIC_TABLE_MAP and topic_name) lacks terminal punctuation; update the construction of msg (used in the logger.error call) to append a trailing period so the error reads like "Unknown topic for Postgres/{topic_name}." and ensure logger.error(msg) logs the punctuated string.
53-60: You can remove the return-value type ignore here.
_db_configcan be typed asdict[str, Any] | None, which removes the need for# type: ignore[return-value]and keeps typing clearer.♻️ Suggested typing cleanup
- self._db_config: dict[str, Any | None] | None = None + self._db_config: dict[str, Any] | None = None @@ - return self._db_config # type: ignore[return-value] + return self._db_config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/writers/writer_postgres.py` around lines 53 - 60, The attribute _db_config is annotated as dict[str, Any | None] | None which forces the method _load_db_config to use a return-value type ignore; change the field annotation to dict[str, Any] | None (i.e., self._db_config: dict[str, Any] | None = None) so _load_db_config can safely return self._db_config without "# type: ignore[return-value]"; no other code changes needed besides removing the type ignore from the return in _load_db_config and keeping the call to load_postgres_config(self._secret_name, self._secret_region) intact.tests/unit/writers/test_writer_postgres.py (1)
274-274: Usemocker.patch/mocker.patch.objectfor these test patches.These mocks work, but the unit-test guideline requires the
mockerfixture patching APIs.♻️ Suggested refactor
-def test_init_with_secret(monkeypatch, reset_env): +def test_init_with_secret(mocker, reset_env): @@ - monkeypatch.setattr(secrets_mod.boto3, "Session", lambda: MockSession()) + mocker.patch.object(secrets_mod.boto3, "Session", return_value=MockSession()) -def test_check_health_success(reset_env, monkeypatch): +def test_check_health_success(reset_env, mocker): @@ - monkeypatch.setattr(secrets_mod.boto3, "Session", MockSession) + mocker.patch.object(secrets_mod.boto3, "Session", MockSession) -def test_check_health_missing_host(reset_env, monkeypatch): +def test_check_health_missing_host(reset_env, mocker): @@ - monkeypatch.setattr(secrets_mod.boto3, "Session", MockSession) + mocker.patch.object(secrets_mod.boto3, "Session", MockSession)As per coding guidelines,
tests/**/*.py: Usemocker.patch("module.dependency")ormocker.patch.object(Class, "method")for mocking in tests.Also applies to: 342-342, 358-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/writers/test_writer_postgres.py` at line 274, Replace the monkeypatch.setattr usage with the pytest-mock `mocker` fixture: instead of monkeypatch.setattr(secrets_mod.boto3, "Session", lambda: MockSession()), call mocker.patch("secrets_mod.boto3.Session", return_value=MockSession()) or mocker.patch.object(secrets_mod.boto3, "Session", return_value=MockSession()) so the test uses the recommended mocker.patch API; apply the same change for the other occurrences that patch boto3.Session (and any similar monkeypatch.setattr calls) in this test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/event_stats_lambda.py`:
- Around line 76-77: The route parameter name in code must match api.yaml's
`topicName`; update the ROUTE_MAP entry (replace "/stats/{topic_name}" with
"/stats/{topicName}") and change the path-parameter access inside
handler_stats.handle_request (replace uses of
event["pathParameters"]["topic_name"] with event["pathParameters"]["topicName"],
or implement a fallback that checks both keys) so the handler reads the same
parameter name emitted by API Gateway; ensure any other occurrences/tests
referencing "topic_name" are updated to "topicName" or handle both forms.
In `@src/handlers/handler_stats.py`:
- Around line 108-110: The except RuntimeError block in handler_stats.py
currently returns str(exc) to clients; change it to keep detailed error logging
via logger.exception("Stats query failed for topic %s.", topic_name) but return
a generic error message through build_error_response (e.g., "Failed to retrieve
stats" or "Internal server error") instead of str(exc) so internal DB details
are not exposed; update the except handler that references topic_name,
logger.exception, and build_error_response accordingly.
- Around line 91-98: The validation currently accepts booleans because bool
subclasses int; update the checks for timestamp_start, timestamp_end, cursor,
and limit so they explicitly reject bool (e.g., require type(...) is int or add
an "and not isinstance(..., bool)" clause) before calling build_error_response;
ensure the limit check also rejects boolean values while still enforcing limit
>= 1.
In `@src/readers/reader_postgres.py`:
- Around line 219-228: The health check currently returns healthy when the DB
secret or database name is missing, which can mask a broken /stats; update the
health logic in the health-checking function (the block using self._secret_name,
self._secret_region and calling self._load_db_config) so that missing
secret/region or missing db configuration returns an unhealthy status (False)
with a clear message instead of True/"not configured"; keep the existing
exception handling for _load_db_config (BotoCoreError, ClientError,
RuntimeError) but change the branches that check not self._secret_name / not
self._secret_region and not db_config.get("database") to return False and a
descriptive error string so that the service reports unhealthy when Postgres
configuration is absent.
In `@src/writers/writer_postgres.py`:
- Around line 45-47: Update the class docstring for the Postgres writer to
reflect that DB credentials are loaded lazily on first use rather than at object
initialization—mention that credentials are fetched from AWS Secrets Manager
when _load_db_config() is invoked (e.g., on first call to methods that access
the DB) so maintainers are not misled about eager loading behavior; reference
the class name (Postgres writer) and the _load_db_config() method in the
docstring.
---
Nitpick comments:
In `@src/writers/writer_postgres.py`:
- Around line 268-270: The log message created when a topic is not found (see
TOPIC_TABLE_MAP and topic_name) lacks terminal punctuation; update the
construction of msg (used in the logger.error call) to append a trailing period
so the error reads like "Unknown topic for Postgres/{topic_name}." and ensure
logger.error(msg) logs the punctuated string.
- Around line 53-60: The attribute _db_config is annotated as dict[str, Any |
None] | None which forces the method _load_db_config to use a return-value type
ignore; change the field annotation to dict[str, Any] | None (i.e.,
self._db_config: dict[str, Any] | None = None) so _load_db_config can safely
return self._db_config without "# type: ignore[return-value]"; no other code
changes needed besides removing the type ignore from the return in
_load_db_config and keeping the call to load_postgres_config(self._secret_name,
self._secret_region) intact.
In `@tests/unit/writers/test_writer_postgres.py`:
- Line 274: Replace the monkeypatch.setattr usage with the pytest-mock `mocker`
fixture: instead of monkeypatch.setattr(secrets_mod.boto3, "Session", lambda:
MockSession()), call mocker.patch("secrets_mod.boto3.Session",
return_value=MockSession()) or mocker.patch.object(secrets_mod.boto3, "Session",
return_value=MockSession()) so the test uses the recommended mocker.patch API;
apply the same change for the other occurrences that patch boto3.Session (and
any similar monkeypatch.setattr calls) in this test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c9eec98-898d-429d-9c36-7a977aa4f70a
📒 Files selected for processing (10)
README.mdsrc/event_stats_lambda.pysrc/handlers/handler_stats.pysrc/readers/reader_postgres.pysrc/writers/writer_postgres.pyterraform_examples/lambda.tfterraform_examples/variables.tftests/unit/handlers/test_handler_stats.pytests/unit/readers/test_reader_postgres.pytests/unit/writers/test_writer_postgres.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/handlers/test_handler_stats.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- terraform_examples/variables.tf
- tests/unit/readers/test_reader_postgres.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/writers/writer_postgres.py (1)
273-289:⚠️ Potential issue | 🟠 MajorPrevent silent success for mapped-but-unhandled topics.
If
TOPIC_TABLE_MAPgains a new mapped topic without a matching branch, this path currently returns success without writing anything.💡 Suggested fix
with connection.cursor() as cursor: if topic_name == "public.cps.za.dlchange": self._postgres_edla_write(cursor, table_info["main"], message) elif topic_name == "public.cps.za.runs": self._postgres_run_write(cursor, table_info["main"], table_info["jobs"], message) elif topic_name == "public.cps.za.test": self._postgres_test_write(cursor, table_info["main"], message) + else: + msg = f"Mapped topic '{topic_name}' has no Postgres write handler." + logger.error(msg) + return False, msg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/writers/writer_postgres.py` around lines 273 - 289, The code looks up table_info from TOPIC_TABLE_MAP but silently does nothing for any mapped topic_name that isn't one of the three handled branches; update the conditional in the with connection.cursor() block to handle unknown mapped topics by raising or logging an explicit error (e.g., raise ValueError or NotImplementedError) that includes the topic_name and table_info so failures are visible; ensure the new behavior lives alongside the existing handlers that call _postgres_edla_write, _postgres_run_write, and _postgres_test_write so any future additions to TOPIC_TABLE_MAP don’t silently return success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/handler_stats.py`:
- Line 55: The code accesses event["pathParameters"]["topic_name"].lower()
without validating presence or type, which can throw and return a 500; update
the handler (where topic_name is read) to first verify
event.get("pathParameters") is a dict and that "topic_name" exists and is a
string, then call .lower(); if validation fails return a 400 validation response
(with an appropriate error message) instead of letting an exception propagate
from the topic_name lookup or .lower() call.
In `@src/writers/writer_postgres.py`:
- Around line 250-260: The call to self._load_db_config() can raise
JSON/Value/Type errors and those exceptions must be caught so
write()/check_health() return their expected (bool, message) tuples instead of
propagating exceptions; update the blocks in the methods that call
_load_db_config (the shown block plus the similar checks around lines 291-294
and 307-311) to wrap the call in a try/except catching json.JSONDecodeError,
ValueError, TypeError (or a broad Exception if preferred), log a clear error
message using logger.error including the exception text, and return (False,
"<descriptive error>") so failure is reported consistently. Ensure you reference
and modify the code paths inside the methods write() and check_health() that
perform db_config = self._load_db_config() and the subsequent missing-field
validation.
---
Outside diff comments:
In `@src/writers/writer_postgres.py`:
- Around line 273-289: The code looks up table_info from TOPIC_TABLE_MAP but
silently does nothing for any mapped topic_name that isn't one of the three
handled branches; update the conditional in the with connection.cursor() block
to handle unknown mapped topics by raising or logging an explicit error (e.g.,
raise ValueError or NotImplementedError) that includes the topic_name and
table_info so failures are visible; ensure the new behavior lives alongside the
existing handlers that call _postgres_edla_write, _postgres_run_write, and
_postgres_test_write so any future additions to TOPIC_TABLE_MAP don’t silently
return success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e21d60c-f220-4e96-a778-65a71c71d37b
📒 Files selected for processing (4)
api.yamlsrc/handlers/handler_stats.pysrc/writers/writer_postgres.pytests/unit/handlers/test_handler_stats.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/handlers/test_handler_stats.py
- api.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/readers/test_reader_postgres.py (1)
20-21: Usemocker.patch/mocker.patch.objectinstead ofunittest.mock.patchin unit tests.This test module uses
patch(...)andpatch.object(...)fromunittest.mock, which diverges from the project's testing convention.🔧 Example migration pattern
-from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock +from pytest_mock import MockerFixture ... - def test_loads_config_from_secrets_manager(self, reader: ReaderPostgres, pg_secret: dict[str, Any]) -> None: + def test_loads_config_from_secrets_manager( + self, + reader: ReaderPostgres, + pg_secret: dict[str, Any], + mocker: MockerFixture, + ) -> None: mock_client = MagicMock() mock_client.get_secret_value.return_value = {"SecretString": json.dumps(pg_secret)} - - with patch("boto3.Session") as mock_session: - mock_session.return_value.client.return_value = mock_client - result = reader._load_db_config() + mock_session = mocker.patch("boto3.Session") + mock_session.return_value.client.return_value = mock_client + result = reader._load_db_config()Applies to: lines 20, 60, 72, 362, 371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/readers/test_reader_postgres.py` around lines 20 - 21, The test imports and uses unittest.mock.patch/patch.object (import line: "from unittest.mock import MagicMock, patch") which breaks project convention; replace usage with the pytest-mock fixture by removing patch from the import and switching all patch/patch.object calls in tests (e.g., places currently calling patch(...) or patch.object(...)) to use mocker.patch or mocker.patch.object instead, updating any test signatures to accept the mocker fixture where needed and adapt MagicMock uses to remain imported or replaced by mocker.Mock as appropriate so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/readers/reader_postgres.py`:
- Around line 101-127: The code in read_stats() calls self._load_db_config() and
only validates db_config.get("database") but then directly indexes
db_config["host"], ["user"], ["password"], ["port"] when calling
psycopg2.connect, which can raise KeyError; update read_stats() to validate all
required DB secret keys before attempting connection (e.g., ensure host, user,
password, port exist and are non-empty), and if any are missing raise the same
controlled RuntimeError (or include them in a single error message) instead of
letting psycopg2.connect surface a KeyError; reference checking of db_config and
the psycopg2.connect(...) call to locate and fix the code.
---
Nitpick comments:
In `@tests/unit/readers/test_reader_postgres.py`:
- Around line 20-21: The test imports and uses unittest.mock.patch/patch.object
(import line: "from unittest.mock import MagicMock, patch") which breaks project
convention; replace usage with the pytest-mock fixture by removing patch from
the import and switching all patch/patch.object calls in tests (e.g., places
currently calling patch(...) or patch.object(...)) to use mocker.patch or
mocker.patch.object instead, updating any test signatures to accept the mocker
fixture where needed and adapt MagicMock uses to remain imported or replaced by
mocker.Mock as appropriate so behavior remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c3859b5-222d-4515-924e-ff6b13e9ba2b
📒 Files selected for processing (2)
src/readers/reader_postgres.pytests/unit/readers/test_reader_postgres.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This branch introduces the EventStats feature — a dedicated read-only Lambda (
event_stats_lambda.py) exposing aPOST /stats/{topicName}endpoint that allows authorized callers to query ingested run and job events stored in PostgreSQL. The endpoint is backed by a newReaderPostgresreader that executes a JOIN query across thepublic_cps_za_runsandpublic_cps_za_runs_jobstables, supporting millisecond-epoch timestamp window filtering and keyset (cursor-based) pagination viainternal_idfor stable, efficient page traversal. Each returned row is enriched with computed columns —run_date,run_status,formatted_tenant,elapsed_time,start_time, andend_time— mirroring the transformations previously done in downstream reporting tools. The endpoint is protected by the same RS256 JWT authentication and per-topic ACL authorization as the existing write Lambda, and is currently scoped to thepublic.cps.za.runstopic via the newSUPPORTED_TOPICSconstant. The feature is covered by a full unit test suite forHandlerStats,ReaderPostgres, and the lambda routing layer, as well as integration tests using real testcontainers (PostgreSQL, Kafka, LocalStack) that validate the complete write-then-read chain, including authentication, pagination, computed column correctness, and sort ordering.Release Notes
Related
Closes #84
Summary by CodeRabbit
New Features
Documentation
Chores
Tests