GH-124: Update the response schema for /presidents endpoint - #125
Conversation
|
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:
📝 WalkthroughWalkthroughThe presidents API now uses ChangesPresidents endpoint migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PresidentsRoute
participant OrganisationService
participant GazetteSources
PresidentsRoute->>OrganisationService: fetch_presidents()
OrganisationService->>GazetteSources: fetch relations and gazettes
GazetteSources-->>OrganisationService: return source records
OrganisationService-->>PresidentsRoute: return tenureList and gazetteList
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gi_service/contract/rest/organisation_api_contract.yaml (1)
21-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "terms" wording left after the tenureList rename.
The summary/description still reference "president list with terms" / "presidents and terms," even though the response now returns
tenureList/gazetteList. Update the text to match the new model.📝 Suggested fix
- summary: Get all president list with terms - description: Returns the list of presidents and terms. + summary: Get all president list with tenures + description: Returns the list of presidents and their tenures.🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml` around lines 21 - 23, Update the endpoint summary and description near the Organization schema reference to describe the returned tenureList/gazetteList model, removing the stale “terms” wording while preserving the endpoint’s purpose of listing presidents and their tenure information.
🧹 Nitpick comments (1)
gi_service/contract/rest/organisation_api_contract.yaml (1)
851-864: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary nested
allOfaround thePerson$ref.Wrapping
$ref: "#/components/schemas/Person"in its owntype: object+allOfis redundant — a$refused as a loneallOfarray item has no sibling-key restriction, so it can be referenced directly.♻️ Suggested simplification
President: allOf: - - type: object - allOf: - - $ref: "`#/components/schemas/Person`" + - $ref: "`#/components/schemas/Person`" - type: object required: - tenureList properties: tenureList: type: array items: $ref: "`#/components/schemas/Tenure`"🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml` around lines 851 - 864, In the President schema, simplify the first nested allOf branch by referencing Person directly as an item in the outer allOf array. Preserve the separate object branch containing the required tenureList property and Tenure array definition.
🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 865-888: Align the ongoing-tenure representation in the Tenure
schema with the existing person-history and department-history convention by
using the same Present sentinel for endDate, or define endDate as
nullable/optional if that is the chosen shared contract. Update the Tenure
definition’s required/properties metadata accordingly and ensure all history
responses use only this single representation.
---
Outside diff comments:
In `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 21-23: Update the endpoint summary and description near the
Organization schema reference to describe the returned tenureList/gazetteList
model, removing the stale “terms” wording while preserving the endpoint’s
purpose of listing presidents and their tenure information.
---
Nitpick comments:
In `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 851-864: In the President schema, simplify the first nested allOf
branch by referencing Person directly as an item in the outer allOf array.
Preserve the separate object branch containing the required tenureList property
and Tenure array definition.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f511a9bc-7ac6-4091-ad4c-bf6f168a3042
📒 Files selected for processing (1)
gi_service/contract/rest/organisation_api_contract.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gi_service/contract/rest/organisation_api_contract.yaml (1)
24-48: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSync the response contract with the service implementation.
The service still exposes
/all-presidentsand returnstermswithstart,end, andgazettes_published, while this contract requirestenureListwithstartDate,endDate, andgazetteList. The service also returns date-only strings, not the timestamp format in the example. Update the router and service, or defer this contract change until those updates are included.🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml` around lines 24 - 48, The getPresidents response contract is ahead of the implementation: align the router and service behind getPresidents with the documented /all-presidents response, returning tenureList entries with startDate, endDate, and gazetteList, using date-only strings; alternatively revert or defer the contract change until those implementation updates are included.
🤖 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.
Outside diff comments:
In `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 24-48: The getPresidents response contract is ahead of the
implementation: align the router and service behind getPresidents with the
documented /all-presidents response, returning tenureList entries with
startDate, endDate, and gazetteList, using date-only strings; alternatively
revert or defer the contract change until those implementation updates are
included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b48fe04-089e-4499-aa7d-30df7531c119
📒 Files selected for processing (1)
gi_service/contract/rest/organisation_api_contract.yaml
102f6ec to
1180053
Compare
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)
gi_service/contract/rest/organisation_api_contract.yaml (1)
41-51: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the example date format to match the actual service output.
The example shows full ISO timestamps for
startDate,endDate, andgazetteList[].date(for example2024-09-23T00:00:00Z,2024-11-18T00:00:00Z).OrganisationService.fetch_presidentsbuilds these values withrelation.startTime.split("T")[0]andgazette.created.split("T")[0], which produce date-only strings such as2024-09-23. The documented example does not match the real response shape. Update the example values to date-only strings so API consumers do not build clients around an incorrect format.📝 Proposed fix for the example
body: - name: Anura Kumara Dissanayake id: 2403-03-01_cit_1 tenureList: - - startDate: 2024-09-23T00:00:00Z + - startDate: 2024-09-23 endDate: "" gazetteList: - - date: 2024-11-18T00:00:00Z + - date: 2024-11-18 idList: - 2411-09 - - date: 2024-12-13T00:00:00Z + - date: 2024-12-13 idList: - 2412-03 - 2412-04🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml` around lines 41 - 51, Update the tenureList example in the organisation API contract so startDate, endDate, and each gazetteList[].date use date-only strings in YYYY-MM-DD format, matching fetch_presidents output; preserve the existing example values and structure otherwise.
🧹 Nitpick comments (1)
src/services/organisation_service.py (1)
1042-1057: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the gazette search.
organization_gazettes_taskandperson_gazettes_taskfetch every gazette entity of the given kind, system-wide, on every call to/v1/presidents. As the gazette corpus grows, this becomes an unbounded fetch and in-memory grouping operation on a hot path with no pagination or caching.🤖 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/services/organisation_service.py` around lines 1042 - 1057, Bound the gazette retrieval in the `/v1/presidents` flow around `organization_gazettes_task` and `person_gazettes_task` so each query does not fetch the entire system-wide corpus; apply the service’s existing pagination, limit, or relevant-entity filtering mechanism while preserving the required gazette grouping behavior.
🤖 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/services/organisation_service.py`:
- Around line 1117-1128: Update fetch_presidents in
src/services/organisation_service.py at lines 1117-1128 to log entity and
gazette exceptions, treating NotFoundError as the expected empty-result case
while surfacing or clearly logging other exceptions instead of silently ignoring
them. Update the empty-gazette mock in test/test_organisation_service.py at
lines 1287-1307 so get_entities.side_effect raises NotFoundError rather than
returning an empty list, exercising the real exception path.
- Around line 1068-1074: Update the generic exception handler surrounding the
president-relations flow in the relevant service method to re-raise
InternalServerError unchanged, following the existing
BadRequestError/NotFoundError exclusion pattern. Preserve the specific message
raised in the president_relations Exception branch while continuing to wrap
other unexpected exceptions generically.
---
Outside diff comments:
In `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 41-51: Update the tenureList example in the organisation API
contract so startDate, endDate, and each gazetteList[].date use date-only
strings in YYYY-MM-DD format, matching fetch_presidents output; preserve the
existing example values and structure otherwise.
---
Nitpick comments:
In `@src/services/organisation_service.py`:
- Around line 1042-1057: Bound the gazette retrieval in the `/v1/presidents`
flow around `organization_gazettes_task` and `person_gazettes_task` so each
query does not fetch the entire system-wide corpus; apply the service’s existing
pagination, limit, or relevant-entity filtering mechanism while preserving the
required gazette grouping behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e377780-a9f5-4f9f-9734-401dda11d967
📒 Files selected for processing (8)
gi_service/contract/rest/organisation_api_contract.yamlgi_service/contract/rest/person_api_contract.yamlsrc/routers/organisation_router.pysrc/routers/person_router.pysrc/services/organisation_service.pysrc/services/person_service.pytest/test_organisation_service.pytest/test_person_service.py
💤 Files with no reviewable changes (4)
- src/services/person_service.py
- src/routers/person_router.py
- gi_service/contract/rest/person_api_contract.yaml
- test/test_person_service.py
| if isinstance(president_relations, Exception): | ||
| logger.error( | ||
| f"Failed to fetch president relations: {president_relations}" | ||
| ) | ||
| raise InternalServerError( | ||
| "An unexpected error occurred while fetching president relations" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The specific error message never reaches the caller.
This block logs a specific message and raises InternalServerError("An unexpected error occurred while fetching president relations"). This raise happens inside the same try block guarded by the generic except Exception as e: raise InternalServerError("An unexpected error occurred") from e at line 1182. Since InternalServerError is not excluded from that generic handler, it gets caught again and replaced with the generic message. The test still passes because it only checks the exception type, but the more useful diagnostic message is lost in practice. Other methods in this class use an except (BadRequestError, NotFoundError): raise pattern to bypass the generic wrap; apply the same technique here.
🔧 Proposed fix
if isinstance(president_relations, Exception):
logger.error(
f"Failed to fetch president relations: {president_relations}"
)
raise InternalServerError(
"An unexpected error occurred while fetching president relations"
)
...
- except Exception as e:
+ except InternalServerError:
+ raise
+ except Exception as e:
logger.error(f"Error fetching all presidents: {e}")
raise InternalServerError("An unexpected error occurred") from e🤖 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/services/organisation_service.py` around lines 1068 - 1074, Update the
generic exception handler surrounding the president-relations flow in the
relevant service method to re-raise InternalServerError unchanged, following the
existing BadRequestError/NotFoundError exclusion pattern. Preserve the specific
message raised in the president_relations Exception branch while continuing to
wrap other unexpected exceptions generically.
| for i, president_id in enumerate(unique_president_ids): | ||
| entity_data = entities_results[i] | ||
| if not isinstance(entity_data, Exception) and entity_data: | ||
| entity = entity_data[0] | ||
| decoded_name = Util.decode_protobuf_attribute_name(entity.name) | ||
| presidents_map[president_id]["name"] = decoded_name | ||
|
|
||
| # Combine all gazettes into a single list | ||
| all_gazettes = [] | ||
| for gazette_result in (organization_gazettes, person_gazettes): | ||
| if not isinstance(gazette_result, Exception) and gazette_result: | ||
| all_gazettes.extend(gazette_result) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
"No gazettes" and "gazette fetch failed" are indistinguishable, and the test doesn't cover the real failure path. OpenGINService.get_entities raises NotFoundError when a search returns an empty body, so a genuine "no gazettes of this kind" outcome surfaces to fetch_presidents as a caught exception, identical to any other upstream failure. The test suite mocks the empty case as a plain [] return instead of the exception that get_entities actually raises, so the real code path is never exercised.
src/services/organisation_service.py#L1117-L1128: Add logging whenentity_data/gazette_resultis anException, and distinguishNotFoundError(expected empty result) from other exceptions (genuine failure) so real failures are not silently masked.test/test_organisation_service.py#L1287-L1307: Changemock_opengin_service.get_entities.side_effectfor the empty-gazette case to raiseNotFoundError(matching the realget_entitiescontract) instead of returning[], so the test exercises the actual exception-handling branch infetch_presidents.
📍 Affects 2 files
src/services/organisation_service.py#L1117-L1128(this comment)test/test_organisation_service.py#L1287-L1307
🤖 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/services/organisation_service.py` around lines 1117 - 1128, Update
fetch_presidents in src/services/organisation_service.py at lines 1117-1128 to
log entity and gazette exceptions, treating NotFoundError as the expected
empty-result case while surfacing or clearly logging other exceptions instead of
silently ignoring them. Update the empty-gazette mock in
test/test_organisation_service.py at lines 1287-1307 so get_entities.side_effect
raises NotFoundError rather than returning an empty list, exercising the real
exception path.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/services/organisation_service.py (2)
1181-1183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEach tenure shares the same
idListobject.
gazettes_by_date[gazette_date]is appended by reference to every matching tenure. Serialization is correct today, but any later mutation of one tenure'sidListchanges all of them. Copy the list at assignment time:list(gazettes_by_date[gazette_date]).🤖 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/services/organisation_service.py` around lines 1181 - 1183, Update the gazette entry construction in the organisation service so each appended tenure receives an independent copy of the list from gazettes_by_date[gazette_date], while preserving the existing date and identifier values.
1042-1057: 🚀 Performance & Scalability | 🔵 TrivialThe endpoint loads every gazette entity on each call.
fetch_presidentsrequests allextgztorgandextgztpersondocuments with no date filter and no pagination, then groups them in memory. The cost grows without bound as gazettes accumulate, and the endpoint has no caching. Consider a date-bounded query, a server-side aggregation, or a cached gazette-by-date index.🤖 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/services/organisation_service.py` around lines 1042 - 1057, The fetch_presidents flow currently retrieves all organisation and person gazette entities without bounds, causing unbounded per-request loading. Update the queries in fetch_presidents to restrict results by the relevant date range and use pagination or server-side aggregation where supported, while preserving the existing grouping behavior for the bounded result set.gi_service/contract/rest/organisation_api_contract.yaml (1)
797-809: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the nested
allOfinPresident.The outer
allOfcontains an object whose only member is anotherallOfwith a single$ref. One level is enough.♻️ Proposed simplification
President: allOf: - - type: object - allOf: - - $ref: "`#/components/schemas/Person`" + - $ref: "`#/components/schemas/Person`" - type: object required: - tenureList properties: tenureList: type: array items: $ref: "`#/components/schemas/Tenure`"🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml` around lines 797 - 809, Flatten the redundant nested allOf in the President schema: replace the inner object/allOf wrapper containing the Person reference with a direct $ref entry alongside the object defining required tenureList and its properties. Preserve the existing Person composition and tenureList validation.test/test_organisation_service.py (1)
1288-1308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for an unresolved president name.
The contract marks
nameas required, andfetch_presidentsleavesnameas""when the entity fetch fails (src/services/organisation_service.pylines 1130-1135). No test covers that path. A test that raises on the president name fetch would pin the intended behavior.🤖 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 `@test/test_organisation_service.py` around lines 1288 - 1308, The existing fetch_presidents tests do not cover a failed president entity lookup. Add a test alongside test_fetch_presidents_no_gazettes that makes the president name/entity fetch raise, then assert fetch_presidents still returns the president with the contract-required name value and preserves the expected response structure.
🤖 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 `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 41-51: The tenureList example in the organisation API contract
uses timestamp values, while OrganisationService.fetch_presidents returns
date-only strings. Update startDate, endDate, and each gazette date in the
example to the YYYY-MM-DD format produced by the service, preserving the
existing example dates and structure.
In `@src/routers/organisation_router.py`:
- Around line 96-100: The presidents route and API contract use different paths;
align them so clients target the served endpoint. In
src/routers/organisation_router.py lines 96-100, preserve the intentional
/v1/organisation prefix and update the contract path in
gi_service/contract/rest/organisation_api_contract.yaml line 18 to
/v1/organisation/presidents.
In `@src/services/organisation_service.py`:
- Around line 1169-1183: Update the inner loop over all_terms in the
gazette-date assignment logic to skip terms whose end is before gazette_date,
not only those removed by the leading term_index while loop. Preserve the
existing start-date break and append gazette data only for terms active on the
current date.
---
Nitpick comments:
In `@gi_service/contract/rest/organisation_api_contract.yaml`:
- Around line 797-809: Flatten the redundant nested allOf in the President
schema: replace the inner object/allOf wrapper containing the Person reference
with a direct $ref entry alongside the object defining required tenureList and
its properties. Preserve the existing Person composition and tenureList
validation.
In `@src/services/organisation_service.py`:
- Around line 1181-1183: Update the gazette entry construction in the
organisation service so each appended tenure receives an independent copy of the
list from gazettes_by_date[gazette_date], while preserving the existing date and
identifier values.
- Around line 1042-1057: The fetch_presidents flow currently retrieves all
organisation and person gazette entities without bounds, causing unbounded
per-request loading. Update the queries in fetch_presidents to restrict results
by the relevant date range and use pagination or server-side aggregation where
supported, while preserving the existing grouping behavior for the bounded
result set.
In `@test/test_organisation_service.py`:
- Around line 1288-1308: The existing fetch_presidents tests do not cover a
failed president entity lookup. Add a test alongside
test_fetch_presidents_no_gazettes that makes the president name/entity fetch
raise, then assert fetch_presidents still returns the president with the
contract-required name value and preserves the expected response structure.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cbc14cb-b43b-47a4-afa5-8ea3c961d4c2
📒 Files selected for processing (8)
gi_service/contract/rest/organisation_api_contract.yamlgi_service/contract/rest/person_api_contract.yamlsrc/routers/organisation_router.pysrc/routers/person_router.pysrc/services/organisation_service.pysrc/services/person_service.pytest/test_organisation_service.pytest/test_person_service.py
💤 Files with no reviewable changes (3)
- test/test_person_service.py
- gi_service/contract/rest/person_api_contract.yaml
- src/routers/person_router.py
ChanukaUOJ
changed the title
2 participants
This PR closes #124
Summary by CodeRabbit
/v1/presidentsto retrieve presidents with person details, tenure dates, and gazette identifiers.termsstructure withtenureList.