Live jurisidictions endpoint #1055 #1131
Conversation
WalkthroughAdds GET /v1/public/jurisdictions/live (optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant APIGW as API Gateway\n(/v1/public/jurisdictions/live)
participant Lambda as compact_configuration_api_handler
participant CCClient as CompactConfigurationClient
Client->>APIGW: GET /v1/public/jurisdictions/live?compact={optional}
APIGW->>Lambda: Invoke (event with query params)
alt compact provided and valid
Lambda->>CCClient: get_live_compact_jurisdictions(compact)
CCClient-->>Lambda: [postalAbbrev...]
Lambda-->>APIGW: { compact: [..] }
else compact missing
loop per configured compact
Lambda->>CCClient: get_live_compact_jurisdictions(compact)
CCClient-->>Lambda: [postalAbbrev...]
end
Lambda-->>APIGW: { compactA: [...], compactB: [...] }
else invalid compact
Lambda-->>APIGW: 400 Invalid compact
end
APIGW-->>Client: 200 application/json (or 400)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (2)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
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: 0
🧹 Nitpick comments (3)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
186-203: Consider adding response model validation for consistency.Other tests in this file validate response models (e.g., lines 74-93), but this test doesn't capture or verify the response model. While the endpoint may not require a formal response model, adding validation would maintain consistency with the established testing pattern.
Consider adding model validation similar to other tests:
# Ensure the GET method is configured with the lambda integration (no authorizer since it's public) + method_model_logical_id_capture = Capture() api_stack_template.has_resource_properties( type=CfnMethod.CFN_RESOURCE_TYPE_NAME, props={ 'HttpMethod': 'GET', 'ResourceId': {'Ref': live_compacts_jurisdictions_resource_id}, # ensure the lambda integration is configured with the expected handler 'Integration': TestApi.generate_expected_integration_object( api_stack.get_logical_id( api_stack.api.v1_api.compact_configuration_api.compact_configuration_api_function.node.default_child, ), ), 'MethodResponses': [ { + 'ResponseModels': {'application/json': {'Ref': method_model_logical_id_capture}}, 'StatusCode': '200', }, ], }, )backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
123-156: Consider documenting the invalid query parameter fallback behavior.The handler silently falls back to returning all compacts when an invalid
compactquery parameter is provided (lines 136-146). While this behavior is intentional and tested, it's not documented in the function docstring. Adding a note about this fallback would improve API clarity for future maintainers.Consider updating the docstring:
def _get_live_public_compact_jurisdictions(event: dict, context: LambdaContext): # noqa: ARG001 unused-argument """ Endpoint to get all live jurisdictions, optionally filtered by compact. :param event: API Gateway event with optional query parameter 'compact' :param context: Lambda context :return: Dictionary with compact abbreviations as keys and lists of live jurisdiction abbreviations as values + + Note: If an invalid compact is provided in the query parameter, the endpoint returns + data for all compacts rather than raising an error. This provides a graceful fallback + for the optional filter parameter. """backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
164-193: Consider adding a response model for API consistency.The endpoint doesn't define a response model (lines 169-173), while other public endpoints in this file do (e.g., lines 140-145). While this may be acceptable for a simple dictionary response, adding a response model would:
- Maintain consistency with other endpoints
- Enable API Gateway response validation
- Improve OpenAPI spec completeness
Consider adding a response model:
get_live_compact_jurisdictions_method = self.live_compacts_jurisdictions_resource.add_method( 'GET', LambdaIntegration(compact_configuration_api_handler), method_responses=[ MethodResponse( status_code='200', + response_models={'application/json': self.api_model.get_live_jurisdictions_response_model}, ), ], request_parameters={ 'method.request.querystring.compact': False, }, )Note: This would require defining
get_live_jurisdictions_response_modelin theapi_model.pyfile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py(1 hunks)backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py(2 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py(4 hunks)backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#882
File: backend/compact-connect/lambdas/python/migration/compact_configured_states_871/main.py:127-130
Timestamp: 2025-06-26T16:42:00.781Z
Learning: In the compact_configured_states_871 migration, existing jurisdiction configurations that have licenseeRegistrationEnabled: true are migrated with isLive: true for backwards compatibility. This prevents breaking existing live functionality since those states are already operational. The migration preserves the current live state of jurisdictions to maintain service continuity, while new states added after migration can start with isLive: false and be controlled by compact admins.
🧬 Code graph analysis (5)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (3)
backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
get(86-87)backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
compacts(104-105)compact_configuration_client(48-51)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_live_compact_jurisdictions(454-480)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object(28-45)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (2)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
put_default_compact_configuration_in_configuration_table(556-572)
backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (2)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCNotFoundException(32-33)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/compact/__init__.py (1)
configuredStates(139-140)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)
🪛 Checkov (3.2.334)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (6)
backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
454-480: LGTM!The method correctly filters configured states for live jurisdictions and appropriately returns an empty list when the compact configuration is not found. This graceful handling is suitable for the use case where the handler queries multiple compacts in a loop.
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
31-32: LGTM!Route registration correctly wired for the new live jurisdictions endpoint.
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (2)
23-43: LGTM!Constructor properly updated to accept and store the new live jurisdictions resource.
94-96: LGTM!Endpoint correctly added to API initialization flow.
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (2)
15-15: LGTM!Constant properly defined for the new endpoint resource path.
195-331: LGTM!Comprehensive test coverage for the live jurisdictions endpoint, including:
- All compacts scenario (no query parameter)
- Specific compact filtering (valid query parameter)
- Graceful fallback behavior (invalid query parameter)
The tests properly verify the response structure and use
assertCountEqualfor order-independent comparison.
ChiefStief
changed the title
landonshumway-ia
left a comment
landonshumway-ia
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of considerations I would like to get your thoughts on before I approve.
...d/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
137-143: Use logger.info or logger.warning for client errors.Line 142 uses
logger.errorfor an invalid query parameter, which is a client error (400 response). Reservelogger.errorfor server errors (5xx); uselogger.infoorlogger.warningfor client errors to maintain proper log severity levels.Apply this diff:
else: - logger.error('Invalid compact provided', compact=compact_filter) + logger.info('Invalid compact provided', compact=compact_filter) raise CCInvalidRequestException(f'Invalid request query param: {compact_filter}')backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
130-133: Update misleading comments.The comments on lines 130 and 132 mention "compacts" in the path, but the actual resources created are:
- Line 131:
/v1/public/jurisdictions- Line 133:
/v1/public/jurisdictions/liveThese comments appear to be outdated from when the path structure included "compacts" (before addressing previous review feedback).
Apply this diff to correct the comments:
- # /v1/public/compacts/jurisdictions + # /v1/public/jurisdictions self.public_jurisdictions_resource = self.public_resource.add_resource('jurisdictions') - # /v1/public/compacts/jurisdictions/live + # /v1/public/jurisdictions/live self.live_jurisdictions_resource = self.public_jurisdictions_resource.add_resource('live')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py(2 hunks)backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (3)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
compacts(104-105)compact_configuration_client(48-51)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_live_compact_jurisdictions(454-480)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (2)
31-32: LGTM!The route registration correctly maps the new public endpoint to the handler. The path
/v1/public/jurisdictions/livealigns with the feedback from previous reviews to remove "compacts" from the path since the endpoint isn't specific to a single compact.
144-155: LGTM!The logic correctly handles both filtered and unfiltered queries, assembles the result dictionary, and logs appropriately. The implementation cleanly delegates to
get_live_compact_jurisdictionsfor each compact.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
212-212: Verify the parameter name matches CompactConfigurationApi's constructor.The parameter name
live_compacts_jurisdictions_resourceincludes "compacts", but the attribute being passed isself.live_jurisdictions_resource(no "compacts"). A previous review comment flagged this inconsistency, suggesting the parameter name should match the attribute name for clarity.#!/bin/bash # Verify the expected parameter name in CompactConfigurationApi's __init__ method ast-grep --pattern $'class CompactConfigurationApi: $$$ def __init__($$$, live_$_jurisdictions_resource$$$): $$$'
🧹 Nitpick comments (3)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json (3)
19-26: Constraincompactquery to known enumsAdd enum to the
compactquery parameter for validation and client clarity (values used elsewhere: aslp, octp, coun)."schema": { - "type": "string" + "type": "string", + "enum": ["aslp", "octp", "coun"] }
28-49: Add 400 response for invalidcompactand tighten response schema
- Include a 400 response for invalid/unknown compact query.
- Constrain jurisdiction array items to the standard two-letter enums used throughout the spec.
- Optionally add maxItems to satisfy CKV_OPENAPI_21.
"responses": { "200": { "description": "200 response - Returns a dictionary with compact abbreviations as keys and arrays of live jurisdiction postal abbreviations as values", "content": { "application/json": { "schema": { "type": "object", "additionalProperties": { "type": "array", + "maxItems": 60, "items": { - "type": "string" + "type": "string", + "enum": [ + "al","ak","az","ar","ca","co","ct","de","dc","fl","ga","hi","id","il","in","ia","ks","ky","la","me","md","ma","mi","mn","ms","mo","mt","ne","nv","nh","nj","nm","ny","nc","nd","oh","ok","or","pa","pr","ri","sc","sd","tn","tx","ut","vt","va","vi","wa","wv","wi","wy" + ] } }, "example": { "aslp": ["co", "ne", "wy"], "octp": ["ak", "ky"] } } } } - } + }, + "400": { + "description": "Bad request (e.g., invalid compact value)", + "content": { + "application/json": { + "schema": { "$ref": "#/components/schemas/SandboLicenmbpMIz6MrP1s" } + } + } + } }Note: If the handler returns a different error shape, reference the appropriate schema.
33-46: Consider restricting object keys to known compact abbreviationsIf feasible, define explicit properties for "aslp", "octp", "coun" (and disallow additional) to improve typing for clients. Otherwise, current additionalProperties is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
🪛 Checkov (3.2.334)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json (1)
13-13: Path is correct and consistent across the codebase.The endpoint path
/v1/public/jurisdictions/livein the spec is correct. Verification of the API stack (api.py), handler (compact_configuration.py), and tests confirms:
- Stack construction (lines 131–133 of
api.py):public_resource.add_resource('jurisdictions').add_resource('live')builds/v1/public/jurisdictions/live- Handler routing (line 31 of
compact_configuration.py): Routes on'/v1/public/jurisdictions/live'- Test constants:
LIVE_JURISDICTIONS_ENDPOINT_RESOURCE = '/v1/public/jurisdictions/live'The misleading comments in the stack code suggesting
/v1/public/compacts/jurisdictions/livedo not reflect the actual resource hierarchy. The live jurisdictions endpoint is intentionally a direct child of/v1/public/, not nested under compacts. No changes to the spec are needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object(28-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (3)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
compacts(104-105)compact_configuration_client(48-51)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_live_compact_jurisdictions(454-480)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
31-32: LGTM!The routing for the new public live jurisdictions endpoint follows the established pattern and correctly maps to the handler function.
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
Outdated
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py(4 hunks)backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py
📚 Learning: 2025-08-18T21:59:25.029Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1014
File: backend/compact-connect/stacks/api_stack/v1_api/api.py:31-36
Timestamp: 2025-08-18T21:59:25.029Z
Learning: The CompactConnect codebase has two separate V1Api implementations: one for the internal API stack (backend/compact-connect/stacks/api_stack/v1_api/api.py) and one for the state API stack (backend/compact-connect/stacks/state_api_stack/v1_api/api.py). These are distinct classes with different constructor signatures and purposes. Changes to one V1Api implementation do not necessarily affect the other.
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api.py
🧬 Code graph analysis (3)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (3)
backend/compact-connect/lambdas/python/common/cc_common/config.py (2)
compacts(104-105)compact_configuration_client(48-51)backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (1)
CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/data_model/compact_configuration_client.py (1)
get_live_compact_jurisdictions(454-480)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object(28-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
123-156: LGTM!The implementation correctly:
- Handles the optional
compactquery parameter for filtering- Validates the compact abbreviation and raises
CCInvalidRequestExceptionwith a 400 response for invalid input (addressing previous review feedback)- Uses info-level logging for user errors (as per coding guidelines)
- Returns a dictionary mapping compact abbreviations to lists of live jurisdiction postal abbreviations
- Follows the pattern of other public endpoints (no authorization required)
The logic properly handles both scenarios: returning live jurisdictions for a specific compact when filtered, or for all compacts when unfiltered.
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
185-203: Also assert query param presence and no authorizer for public endpoint.Add RequestParameters to the assertion and explicitly check that AuthorizerId is absent to avoid regressions.
api_stack_template.has_resource_properties( type=CfnMethod.CFN_RESOURCE_TYPE_NAME, props={ 'HttpMethod': 'GET', 'ResourceId': {'Ref': live_jurisdictions_resource_id}, # ensure the lambda integration is configured with the expected handler 'Integration': TestApi.generate_expected_integration_object( api_stack.get_logical_id( api_stack.api.v1_api.compact_configuration_api.compact_configuration_api_function.node.default_child, ), ), + 'RequestParameters': { + 'method.request.querystring.compact': False, + }, 'MethodResponses': [ { 'StatusCode': '200', }, ], }, )Add this negative assertion below to ensure it stays public:
# Assert no authorizer configured on the live jurisdictions GET method methods = api_stack_template.find_resources(CfnMethod.CFN_RESOURCE_TYPE_NAME) matching = [ v for v in methods.values() if v['Properties'].get('HttpMethod') == 'GET' and v['Properties'].get('ResourceId') == {'Ref': live_jurisdictions_resource_id} ] self.assertTrue(matching, 'Expected to find GET method for live jurisdictions resource') self.assertNotIn('AuthorizerId', matching[0]['Properties'])backend/compact-connect/docs/api-specification/latest-oas30.json (1)
13-67: Add maxItems to satisfy Checkov and constrain array size.The array of jurisdiction codes lacks maxItems. Add it to address CKV_OPENAPI_21 and improve validation. Optionally constrain item format.
"responses": { "200": { "description": "200 response - Returns a dictionary with compact abbreviations as keys and arrays of live jurisdiction postal abbreviations as values", "content": { "application/json": { "schema": { "type": "object", "additionalProperties": { - "type": "array", - "items": { - "type": "string" - } + "type": "array", + "maxItems": 100, + "items": { + "type": "string" + /* optionally add: "pattern": "^[a-z]{2}$" */ + } }, "example": { "aslp": ["co", "ne", "wy"], "octp": ["ak", "ky"] } } } } },If you prefer stricter validation, consider adding enum on the
compactquery parameter: ["aslp","octp","coun"].backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
164-194: Optional: add a response model for stronger contract enforcement.Endpoint is public and correctly sets optional query param. To keep parity with other endpoints and the OAS, consider adding a response model (e.g., get_live_jurisdictions_response_model) so API Gateway validates responses.
backend/compact-connect/docs/internal/api-specification/latest-oas30.json (1)
17-26: Constrain thecompactquery schema to the allowed abbreviations.The handler rejects unknown compact codes with a 400, but the OpenAPI schema currently advertises an unconstrained string. Tightening the schema (e.g., enum
["aslp","octp","coun"]) keeps the contract truthful and lets client tooling surface validation errors earlier.Apply this diff to the
schemablock:- "schema": { - "type": "string" - } + "schema": { + "type": "string", + "enum": ["aslp", "octp", "coun"] + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/docs/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/docs/internal/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py(4 hunks)backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
🧬 Code graph analysis (2)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (2)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)
backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py (1)
backend/compact-connect/tests/app/test_api/__init__.py (2)
TestApi(11-83)generate_expected_integration_object(28-45)
🪛 Checkov (3.2.334)
backend/compact-connect/docs/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
94-97: LGTM: live jurisdictions endpoint wired.Correctly registers GET /v1/public/jurisdictions/live with the shared handler.
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
One edit on a test name/docstring, but otherwise looks great!
...ct-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
Outdated
Show resolved
Hide resolved
|
@jlkravitz ready for another look |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (2)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
put_default_compact_configuration_in_configuration_table(556-572)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py (3)
15-15: LGTM! Clear constant definition.The endpoint resource constant is properly defined and follows the existing naming convention.
195-250: LGTM! Comprehensive test coverage.This test effectively validates the endpoint behavior when no query parameter is provided, correctly verifying that all compacts are returned with their respective live jurisdictions. The use of
assertCountEqualfor list comparisons is appropriate.
251-291: LGTM! Properly tests single-compact filtering.This test correctly validates that when a specific compact is requested via query parameter, only that compact's live jurisdictions are returned. The assertions appropriately verify both inclusion and exclusion of expected data.
...ct-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
Show resolved
Hide resolved
ChiefStief
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json (1)
13-67: Tighten schema + reuse error component for GET /v1/public/jurisdictions/live
- Validate
compactwith an enum to reflect allowed values.- Constrain returned jurisdiction arrays: enforce uniqueness, reasonable max size, and two‑letter lowercase codes.
- Reuse existing message schema instead of an inline 400 object for consistency.
Apply minimally invasive edits:
@@ "parameters": [ { "name": "compact", "in": "query", "required": false, "description": "Optional compact abbreviation to filter results. If not provided, returns data for all compacts. If an invalid compact is provided, returns a 400 error.", - "schema": { - "type": "string" - } + "schema": { + "type": "string", + "enum": ["aslp", "octp", "coun"] + } } ], @@ - "additionalProperties": { - "type": "array", - "items": { - "type": "string" - } - }, + "additionalProperties": { + "type": "array", + "uniqueItems": true, + "maxItems": 60, + "items": { + "type": "string", + "pattern": "^[a-z]{2}$" + } + }, @@ "400": { "description": "400 response - Invalid compact abbreviation provided", "content": { "application/json": { "schema": { - "type": "object", - "properties": { - "message": { - "type": "string", - "example": "Invalid request query param: invalid_compact" - } - } + "$ref": "#/components/schemas/SandboLicenaLbwv28ZqyH6" } } } }Note: ensure these constraints are added in the CDK/source so the autogen step reflects them in this file. Based on learnings.
backend/compact-connect/docs/api-specification/latest-oas30.json (1)
13-67: Add enum forcompactand constrain array items; align 400 with existing StateApi error schema
- Add enum ["aslp","octp","coun"] to the query param.
- Add uniqueItems, maxItems, and a two‑letter lowercase pattern for jurisdiction codes.
- Prefer referencing
#/components/schemas/SandboStatecjpoBVtvh5nrfor 400 to match other endpoints.Suggested patch:
@@ "parameters": [ { "name": "compact", "in": "query", "required": false, "description": "Optional compact abbreviation to filter results. If not provided, returns data for all compacts. If an invalid compact is provided, returns a 400 error.", - "schema": { - "type": "string" - } + "schema": { + "type": "string", + "enum": ["aslp", "octp", "coun"] + } } ], @@ - "additionalProperties": { - "type": "array", - "items": { - "type": "string" - } - }, + "additionalProperties": { + "type": "array", + "uniqueItems": true, + "maxItems": 60, + "items": { + "type": "string", + "pattern": "^[a-z]{2}$" + } + }, @@ "400": { "description": "400 response - Invalid compact abbreviation provided", "content": { "application/json": { - "schema": { - "type": "object", - "properties": { - "message": { - "type": "string", - "example": "Invalid request query param: invalid_compact" - } - } - } + "schema": { "$ref": "#/components/schemas/SandboStatecjpoBVtvh5nr" } } } }As this file is auto‑generated, make the change in the source shapes so regeneration preserves it. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/docs/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/docs/internal/api-specification/latest-oas30.json(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py(4 hunks)backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/stacks/api_stack/v1_api/api.py
- backend/compact-connect/tests/app/test_api/test_compact_configuration_api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-15T19:59:09.332Z
Learnt from: jusdino
PR: csg-org/CompactConnect#1004
File: backend/compact-connect/docs/internal/api-specification/latest-oas30.json:2305-2354
Timestamp: 2025-08-15T19:59:09.332Z
Learning: In the CompactConnect API, the history endpoint paths are intentionally different between internal and public APIs. Internal compacts history includes "/privileges/" in the path (/v1/compacts/{compact}/providers/{providerId}/privileges/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history) while public compacts history omits it (/v1/public/compacts/{compact}/providers/{providerId}/jurisdiction/{jurisdiction}/licenseType/{licenseType}/history). This inconsistency is a deliberate product design choice, not an oversight.
Applied to files:
backend/compact-connect/docs/api-specification/latest-oas30.json
📚 Learning: 2025-08-22T21:20:35.260Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1029
File: backend/compact-connect/docs/api-specification/latest-oas30.json:468-471
Timestamp: 2025-08-22T21:20:35.260Z
Learning: The file backend/compact-connect/docs/api-specification/latest-oas30.json is auto-generated by API Gateway and should not be modified inline. Any schema changes would need to be addressed at the source in the CDK/CloudFormation definitions.
Applied to files:
backend/compact-connect/docs/api-specification/latest-oas30.json
🧬 Code graph analysis (1)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (1)
backend/compact-connect/lambdas/python/compact-configuration/handlers/compact_configuration.py (1)
compact_configuration_api_handler(24-42)
🪛 Checkov (3.2.334)
backend/compact-connect/docs/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
backend/compact-connect/docs/internal/api-specification/latest-oas30.json
[medium] 35-39: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py (3)
24-24: LGTM: Constructor parameter addition.The
live_jurisdictions_resourceparameter follows the established pattern for resource injection and is properly typed.
38-39: LGTM: Instance variable assignment.The comment and assignment properly document and store the live jurisdictions resource.
63-65: LGTM: Method invocation.The call correctly invokes the endpoint setup method with the appropriate handler parameter.
backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py
Show resolved
Hide resolved
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@ChiefStief This looks good to me! Looks like there are a couple CodeRabbit comments that need to be resolved before @isabeleliassen can merge. Feel free to address or resolve those as needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/compact-configuration/tests/function/test_compact_configuration.py
- backend/compact-connect/stacks/api_stack/v1_api/compact_configuration_api.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (6)
backend/compact-connect/tests/app/base.py (1)
get_context(59-60)backend/compact-connect/tests/app/test_pipeline.py (1)
get_context(22-31)backend/compact-connect-ui-app/tests/app/test_pipeline.py (1)
get_context(11-21)backend/compact-connect/tests/app/test_api/__init__.py (1)
get_context(17-25)backend/compact-connect/tests/app/test_event_listener.py (1)
get_context(19-27)backend/compact-connect/tests/app/test_transaction_monitoring.py (1)
get_context(27-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
|
@jlkravitz Code rabbit comments have been addressed. I think @isabeleliassen wanted you to give a final approval before merging |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to merge!
4 participants
Requirements List
Description List
public/compacts/jurisdictions/liveendpointTesting List
backend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #1055
Summary by CodeRabbit
New Features
Documentation
Tests