Add status change topic#155
Conversation
kevinwallimann
requested review from
lsulak,
oto-macenauer-absa,
petr-pokorny-absa and
tmikula-dev
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new status_change topic (constant, schema, access), topic-keys config and loader, HandlerTopic message-key resolution and POST wiring, writer signature updates to accept message_key, and corresponding unit tests and editor test settings. ChangesStatus Change Topic Support
Sequence DiagramsequenceDiagram
participant Client
participant HandlerTopic as src/handlers/handler_topic.py
participant ConfigLoader as src/utils/config_loader.py
participant TopicKeys as conf/topic_keys.json
participant WriterKafka as src/writers/writer_kafka.py
Client->>HandlerTopic: POST /topics/{topic} with message
HandlerTopic->>ConfigLoader: (on init) load topic_keys_config
HandlerTopic->>TopicKeys: lookup configured key field for topic
HandlerTopic->>WriterKafka: writer.write(topic, message, message_key)
WriterKafka->>KafkaProducer: produce(topic=..., key=message_key, value=json_bytes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
tests/unit/handlers/test_handler_topic.py (1)
90-90: ⚡ Quick winAdd one POST-path test for
public.cps.za.status-changeschema validation.Current additions validate discovery/listing only. A valid + invalid POST case for the new topic would protect the end-to-end contract (load + validate + reject bad payloads) from regressions.
Also applies to: 103-107, 117-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/handlers/test_handler_topic.py` at line 90, Add a new test function (e.g., test_post_status_change_schema_validation) in the existing test_handler_topic suite that exercises the POST path for the schema named "public.cps.za.status-change": send one valid POST payload (matching the schema, e.g., including execution_id as string) and assert the handler accepts it (status accepted/success), then send an obviously invalid payload (e.g., execution_id missing or wrong type) and assert the handler rejects it with a validation error (400 or rejection response and an error message referencing the schema/field); place both assertions in the same test to ensure load+validate behavior is covered end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conf/topic_schemas/status_change.json`:
- Around line 20-33: The schema currently treats tenant_id as optional and lacks
explicit previous_state/new_state fields; update status_change.json to make the
application identifier required (mark "tenant_id" as required in the root
"required" array or otherwise add a required "application_id" equivalent), add
"previous_state" and "new_state" properties (type "string", non-nullable) to the
schema, and include those two fields in the schema's "required" array so every
status_change event must carry tenant_id plus previous_state and new_state;
apply the same changes to the other schema blocks noted (lines referenced in the
review).
- Around line 52-55: The schema defines "timestamp_event" as epoch milliseconds
but uses "type": "number" which allows fractions; change the "timestamp_event"
field in the JSON schema to use "type": "integer" (and add "minimum": 0 if you
want to enforce non-negative timestamps) so the schema enforces
whole-millisecond epoch values.
---
Nitpick comments:
In `@tests/unit/handlers/test_handler_topic.py`:
- Line 90: Add a new test function (e.g.,
test_post_status_change_schema_validation) in the existing test_handler_topic
suite that exercises the POST path for the schema named
"public.cps.za.status-change": send one valid POST payload (matching the schema,
e.g., including execution_id as string) and assert the handler accepts it
(status accepted/success), then send an obviously invalid payload (e.g.,
execution_id missing or wrong type) and assert the handler rejects it with a
validation error (400 or rejection response and an error message referencing the
schema/field); place both assertions in the same test to ensure load+validate
behavior is covered end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 469ed8af-1aad-4a69-aa28-c2f10a0eb96a
📒 Files selected for processing (7)
conf/access.jsonconf/topic_schemas/status_change.jsonsrc/handlers/handler_topic.pysrc/utils/config_loader.pysrc/utils/constants.pytests/unit/handlers/test_handler_topic.pytests/unit/utils/test_config_loader.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
conf/topic_schemas/status_change.json (1)
34-41: 💤 Low valueMinor: stray leading space in
source_appdescription.Line 36's description starts with a leading space (
" Standardized source application name..."). Trivial cleanup while you're touching the field.✏️ Proposed tweak
- "description": " Standardized source application name (aqueduct, unify, lum, etc)" + "description": "Standardized source application name (aqueduct, unify, lum, etc)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conf/topic_schemas/status_change.json` around lines 34 - 41, The description value for the JSON schema field "source_app" contains a stray leading space; update the "source_app" property's "description" (in status_change.json) to remove the leading whitespace so it reads "Standardized source application name (aqueduct, unify, lum, etc)" exactly, ensuring the JSON string has no extra leading character.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@conf/topic_schemas/status_change.json`:
- Around line 34-41: The description value for the JSON schema field
"source_app" contains a stray leading space; update the "source_app" property's
"description" (in status_change.json) to remove the leading whitespace so it
reads "Standardized source application name (aqueduct, unify, lum, etc)"
exactly, ensuring the JSON string has no extra leading character.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61200d85-a19c-420a-9e0c-38cd27ac8d69
📒 Files selected for processing (1)
conf/topic_schemas/status_change.json
oto-macenauer-absa
left a comment
oto-macenauer-absa
left a comment
There was a problem hiding this comment.
I see couple issue that should be fixed:
postgres writer - this one is not implemented - it requires specific sql defined, could be opted out if sql peristence is not desired but with the current state it will spam "unknow topic"errors
kafka writer - at the moment the key for the message is not used, but with this topic, if we need the ordering, it should use some key
is the ADR going to be part of the commit? There are some inconsistencies, e.g. country_code vs country, incomplete events, also isn't the status_type duplicate with the event_type?
Also I'm not sure EventGate works with the "allOf" conditional validation, this should be tested.
| TOPIC_TEST = "public.cps.za.test" | ||
| TOPIC_STATUS_CHANGE = "public.cps.za.status-change" | ||
|
|
||
| SUPPORTED_WRITE_TOPICS: frozenset[str] = frozenset({TOPIC_RUNS, TOPIC_DLCHANGE, TOPIC_TEST}) |
There was a problem hiding this comment.
missing the status_change topic in SUPPORTED_WRITE_TOPICS
There was a problem hiding this comment.
Hmm also there is no addition of this new queue in src/writers/sql/inserts.sql - do we want to push it into our Postgres for analytical purposes or no? It might become quite massive over time though
| "description": "Environment (dev, uat, pre-prod, prod, test or others)" | ||
| }, | ||
| "timestamp_event": { | ||
| "type": "integer", |
There was a problem hiding this comment.
The runs schema uses "type": "number" for timestamps. Herei s "integer". While epoch milliseconds are integers, the inconsistency may confuse producers.
There was a problem hiding this comment.
Yes, number can also be floating point, so integer is the correct choice here. However, your call, rather be consistent but wrong, or eventually fix it in the runs schema as well?
| "properties": { | ||
| "event_type": { | ||
| "enum": [ | ||
| "JobCreatedEvent", |
There was a problem hiding this comment.
Ok, I like that you thought about it, but aren't half of these already mandatory? These: source_app and source_app_version and environment so to me only the last 2 make sense here unless I misunderstood how it works.
Another thing - maybe some other event types can also require some mandatory attributes?
Like, JobUpdatedEvent might require status_type or status_detail - this is the primary change that the event would emit, right? Otherwise, what's there to update?
Thinking about this a bit more - mandatory parameters:
- Consider adding
job_nameand perhaps evendefinition_idas required on creation events (identity should be established at creation) - Consider making
status_typerequired for "JobCreatedAndStartedEvent", "JobStartedEvent", "JobUpdatedEvent"
There was a problem hiding this comment.
- I made
status_typemandatory for all, even though there is only one valid value for JobCreatedEvent, JobStartedEvent and JobCreatedAndStartedEvent. - As for
source_appetc. I removed them from generally required, to only mandatory inJobCreatedEventandJobCreatedAndStartedEvent, since there's no point repeating that information throughout the lifecycle of the job. Thejob_idis sufficient to identify the job - I've added
job_nameanddefinition_idto the required parameters inJobCreatedEventandJobCreatedAndStartedEvent
Co-authored-by: Oto Macenauer <112095903+oto-macenauer-absa@users.noreply.github.com>
| { | ||
| "python.testing.pytestArgs": [ | ||
| "tests" | ||
| ], | ||
| "python.testing.unittestEnabled": false, | ||
| "python.testing.pytestEnabled": true | ||
| } No newline at end of file |
There was a problem hiding this comment.
Was this file committed on purpose? This one is IDE-specific and some parts of the configuration can be moved inside the pyproject.toml, that we use for the project conf setting. Or is there some point of having this on GitHub, I do not see?
There was a problem hiding this comment.
No, I've removed it now. Thanks
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)
src/writers/writer.py (1)
41-52: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAlign
Writermethod contracts with repo writer guidelines.The abstract contract still uses
write(...) -> Noneandcheck_health() -> str | None, but the writers guideline requires tuple-return contracts. Since this interface is being changed in this PR, please align it now (and cascade to implementations/callers) or explicitly update the guideline before merge.As per coding guidelines
src/writers/*.py: Writers must inherit fromWriter(ABC)and implementwrite(topic, message) -> (bool, str|None)andcheck_health() -> (bool, str)methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/writers/writer.py` around lines 41 - 52, Update the Writer abstract interface to match the repo guideline: change Writer.write signature to return a tuple (bool, str|None) instead of None and change Writer.check_health to return (bool, str) instead of str|None; update the docstrings to document the boolean success flag and message, and then cascade these signature changes to all concrete implementations and callers so they return and propagate (success, message) tuples (look for usages of write and check_health in classes inheriting Writer and in any code calling those methods and adjust error handling accordingly).
🧹 Nitpick comments (1)
tests/unit/handlers/test_handler_topic.py (1)
74-118: ⚡ Quick winUse
pytest-mockpatching style in these new tests.These added tests use
unittest.mock.patch/patch.object; this repo standard asks formocker.patch(...)/mocker.patch.object(...)in tests for consistency.Example conversion pattern
-def test_post_passes_topic_key_to_writers(event_gate_module, make_event, valid_payload): +def test_post_passes_topic_key_to_writers(event_gate_module, make_event, valid_payload, mocker): ... - with patch.object(event_gate_module.handler_token, "decode_jwt", return_value={"sub": "TestUser"}): + mocker.patch.object(event_gate_module.handler_token, "decode_jwt", return_value={"sub": "TestUser"}) + ...As per coding guidelines
tests/**/*.py: Usemocker.patch("module.dependency")ormocker.patch.object(Class, "method")for mocking in tests.Also applies to: 260-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/handlers/test_handler_topic.py` around lines 74 - 118, Replace unittest.mock.patch usage in the new tests with pytest-mock's mocker fixture calls: in test_load_topic_keys_config_from_local_file and test_load_topic_keys_config_from_s3 use mocker.patch("builtins.open", ...) instead of patch(...) and use mocker to create/patch AWS S3 interactions (e.g., mocker.patch.object or mocker.patch to stub HandlerTopic dependencies) so mocks are created via the mocker fixture; keep assertions on HandlerTopic.with_load_topic_keys_config, handler.topic_keys, and mock_aws_s3.Bucket/Object calls unchanged while converting all patch/patch.object usages referenced in these functions to mocker.patch/mocker.patch.object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/config_loader.py`:
- Around line 50-65: Run Black on this file to satisfy the formatting CI:
reformat the function _load_json_from_path (and the file) using Black (e.g.,
black src/utils/config_loader.py) so the conditional return lines, argument
annotations, and docstring conform to Black style; ensure you stage the
resulting changes and re-run black --check before pushing.
- Around line 149-184: Restore a _normalize_topic_keys_config(topic_key_data:
dict[str, Any]) function and call it from load_topic_keys_config so the module
exports the expected symbol and returns normalized data; implement
_normalize_topic_keys_config to (1) coerce/validate topic names and field names
to strings, strip surrounding whitespace from both keys and values, and raise
ValueError via _validate_topic_keys_config if a value is not a string, and (2)
return a TopicKeyMap (dict[str, str]) with normalized topic keys (e.g.,
trimmed/lowercased if your tests expect that) before load_topic_keys_config
returns the mapping; update load_topic_keys_config to invoke
_normalize_topic_keys_config(topic_key_data) and return its result instead of
the raw topic_key_data.
In `@tests/unit/utils/test_config_loader.py`:
- Around line 27-34: The test imports a private helper named
_normalize_topic_keys_config from src.utils.config_loader but that symbol
doesn't exist; either add/export a function with that exact name in the
config_loader module (implement it or make it a thin wrapper that calls the
existing public normalizer such as
load_topic_keys_config/_normalize_access_config) or change the test to stop
importing the private helper and use the public API (e.g., call
load_topic_keys_config or the public normalizer). Locate the missing symbol by
checking the config_loader module and either define _normalize_topic_keys_config
there or update tests/unit/utils/test_config_loader.py to import and assert
behavior via the public functions instead.
---
Outside diff comments:
In `@src/writers/writer.py`:
- Around line 41-52: Update the Writer abstract interface to match the repo
guideline: change Writer.write signature to return a tuple (bool, str|None)
instead of None and change Writer.check_health to return (bool, str) instead of
str|None; update the docstrings to document the boolean success flag and
message, and then cascade these signature changes to all concrete
implementations and callers so they return and propagate (success, message)
tuples (look for usages of write and check_health in classes inheriting Writer
and in any code calling those methods and adjust error handling accordingly).
---
Nitpick comments:
In `@tests/unit/handlers/test_handler_topic.py`:
- Around line 74-118: Replace unittest.mock.patch usage in the new tests with
pytest-mock's mocker fixture calls: in
test_load_topic_keys_config_from_local_file and
test_load_topic_keys_config_from_s3 use mocker.patch("builtins.open", ...)
instead of patch(...) and use mocker to create/patch AWS S3 interactions (e.g.,
mocker.patch.object or mocker.patch to stub HandlerTopic dependencies) so mocks
are created via the mocker fixture; keep assertions on
HandlerTopic.with_load_topic_keys_config, handler.topic_keys, and
mock_aws_s3.Bucket/Object calls unchanged while converting all
patch/patch.object usages referenced in these functions to
mocker.patch/mocker.patch.object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99ff74a3-b46d-4653-a0ee-41527d804eba
📒 Files selected for processing (13)
conf/config.jsonconf/topic_keys.jsonconf/topic_schemas/status_change.jsonsrc/event_gate_lambda.pysrc/handlers/handler_topic.pysrc/utils/config_loader.pysrc/writers/writer.pysrc/writers/writer_eventbridge.pysrc/writers/writer_kafka.pysrc/writers/writer_postgres.pytests/unit/handlers/test_handler_topic.pytests/unit/utils/test_config_loader.pytests/unit/writers/test_writer_kafka.py
✅ Files skipped from review due to trivial changes (1)
- conf/config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/topic_schemas/status_change.json
I've added per-topic configuration to specify a property of the event to use as the key. The fallback value is the empty string
Some output: Other output: Failed validating 'enum' in schema['allOf'][1]['then']['properties']['status_type']: On instance['status_type']: |
4 participants
Overview
See
Release Notes
Related
Closes #151
Summary by CodeRabbit
New Features
Chores
Tests