[OSDEV-1577] Add geo-bounding box support to the GET /v1/production-locations/ endpoint#503
Conversation
mazursasha1990
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
changed the title
/v1/production-locations/ endpoint/v1/production-locations/ endpoint
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
mazursasha1990
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
…he-production-locations-endpoint
📝 WalkthroughWalkthroughThis pull request introduces geo-bounding box query support for the GET Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Endpoint (/api/v1/production-locations/)
participant Serializer as ProductionLocationsSerializer
participant Validator as GeoBoundingBoxValidator
participant QueryDirector as OpenSearchQueryDirector
participant QueryBuilder as ProductionLocationsQueryBuilder
participant OpenSearch
Client->>API: GET request with geo_bounding_box params
API->>Serializer: Validate and process request data
Serializer->>Validator: Validate geo bounding box parameters
Validator-->>Serializer: Return validation result
Serializer->>QueryDirector: Forward validated data
QueryDirector->>QueryBuilder: Call add_geo_bounding_box(top, left, bottom, right)
QueryBuilder->>OpenSearch: Build & execute query
OpenSearch-->>QueryBuilder: Return query results
QueryBuilder-->>QueryDirector: Send query clause
QueryDirector-->>API: Return constructed query response
API-->>Client: Deliver API response
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/django/api/serializers/v1/opensearch_common_validators/geo_bounding_box_validator.py (1)
6-67: Add type hints for better code maintainability.The validator implementation is correct and thorough. Consider adding type hints to improve code maintainability.
- def validate_opensearch_params(self, data): + def validate_opensearch_params(self, data: dict) -> list[dict]:Also, consider extracting the magic numbers to class constants:
+ class GeoBoundingBoxValidator(OpenSearchValidationInterface): + LAT_MIN = -90 + LAT_MAX = 90 + LNG_MIN = -180 + LNG_MAX = 180 + def validate_opensearch_params(self, data: dict) -> list[dict]: errors = [] fields = ['top', 'left', 'bottom', 'right'] coords = { field: data.get(f'geo_bounding_box_{field}') for field in fields } # ... rest of the code ... range_limits = { - 'top': (-90, 90), - 'bottom': (-90, 90), - 'left': (-180, 180), - 'right': (-180, 180), + 'top': (self.LAT_MIN, self.LAT_MAX), + 'bottom': (self.LAT_MIN, self.LAT_MAX), + 'left': (self.LNG_MIN, self.LNG_MAX), + 'right': (self.LNG_MIN, self.LNG_MAX), }src/tests/v1/test_production_locations.py (1)
122-159: Enhance test coverage for geo-bounding box functionality.While the current test verifies basic functionality, consider adding tests for:
- Edge cases (coordinates at boundaries -90/90 and -180/180)
- Invalid bounding box (bottom > top or left > right)
- Points exactly on the bounding box edges
def test_production_locations_geo_bounding_box(self): + # Test existing case doc = { "sector": [ "Apparel" ], "address": "Test Address 2", "name": "Test Name 2", "country": { "alpha_2": "US" }, "os_id": "US2020052SV22KJ", "coordinates": { "lon": -102.378162, "lat": 40.1166236 }, } self.open_search_client.index( index=self.production_locations_index_name, body=doc, id=self.open_search_client.count() ) self.open_search_client.indices.refresh( index=self.production_locations_index_name ) query = ( "?geo_bounding_box[top]=41&geo_bounding_box[left]=" "-103&geo_bounding_box[bottom]=39&geo_bounding_box[right]=-101" ) response = requests.get( f"{self.root_url}/api/v1/production-locations/{query}", headers=self.basic_headers, ) result = response.json() self.assertIsNotNone(result['data']) self.assertEqual(len(result['data']), 1) self.assertEqual(result['data'][0]['os_id'], 'US2020052SV22KJ') + # Test edge case: Point on boundary + doc_boundary = { + "os_id": "US2020052SV22KK", + "coordinates": {"lon": -180, "lat": 90} + } + self.open_search_client.index( + index=self.production_locations_index_name, + body=doc_boundary, + id=self.open_search_client.count() + ) + self.open_search_client.indices.refresh( + index=self.production_locations_index_name + ) + + query_boundary = "?geo_bounding_box[top]=90&geo_bounding_box[left]=-180&geo_bounding_box[bottom]=89&geo_bounding_box[right]=-179" + response = requests.get( + f"{self.root_url}/api/v1/production-locations/{query_boundary}", + headers=self.basic_headers, + ) + result = response.json() + self.assertEqual(len(result['data']), 1) + self.assertEqual(result['data'][0]['os_id'], 'US2020052SV22KK')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/geo_bounding_box_validator.py(1 hunks)src/django/api/serializers/v1/production_locations_serializer.py(3 hunks)src/django/api/tests/test_production_locations_query_builder.py(1 hunks)src/django/api/tests/test_v1_utils.py(1 hunks)src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py(2 hunks)src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py(1 hunks)src/django/api/views/v1/parameters_list.py(1 hunks)src/django/api/views/v1/utils.py(1 hunks)src/tests/v1/test_production_locations.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
30-30: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (9)
src/django/api/views/v1/parameters_list.py (1)
36-36: LGTM!The new constant follows the existing naming convention and is appropriately placed within the class.
src/django/api/serializers/v1/production_locations_serializer.py (1)
61-64: LGTM!The new fields and validator are well-integrated with the existing serializer structure.
Also applies to: 73-73
src/django/api/views/v1/utils.py (1)
27-30: LGTM!The new geo bounding box parameters are correctly integrated with the existing parameter flattening logic.
src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1)
133-143: LGTM! The geo-bounding box filter implementation looks correct.The implementation properly constructs the geo_bounding_box filter structure required by OpenSearch.
src/django/api/tests/test_v1_utils.py (4)
197-214: Well-structured test for geo-bounding box parameter serialization!The test effectively validates:
- Successful serialization of all required parameters
- Correct handling of edge case values (-180/180 for longitude, -90/90 for latitude)
- Proper parameter naming convention
216-239: Good error case validation for missing parameters!The test effectively validates:
- Error handling when required coordinates are missing
- Clear error message indicating all coordinates are required
- Standard error response structure
241-272: Thorough validation of coordinate range constraints!The test effectively validates:
- Proper error handling for out-of-range values
- Specific error messages identifying which coordinate is invalid
- Correct aggregation of multiple validation errors
274-305: Excellent validation of coordinate ordering requirements!The test effectively validates:
- Proper error handling for invalid coordinate ordering
- Clear error messages explaining the ordering requirements
- Correct aggregation of multiple ordering validation errors
doc/release/RELEASE-NOTES.md (1)
22-30: Well-documented feature addition in release notes!The release notes effectively document:
- The new geo-bounding box query support
- All validation rules for the coordinates
- Clear link to the JIRA ticket for traceability
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
25-25: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
30-30: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py
Show resolved
Hide resolved
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 (2)
src/django/api/serializers/v1/production_locations_serializer.py(2 hunks)src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/serializers/v1/production_locations_serializer.py
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-fe-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (1)
src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py (1)
44-44: LGTM!The addition of
__process_filterto the query building process follows the established pattern and is placed appropriately in the sequence of operations.
src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py (3)
26-28: Update error message to use constants.The error message contains hardcoded values (-90 and 90) while the validation uses constants. For consistency and maintainability, use the constants in the error message as well.
- "detail": "Latitude must be between -90 and 90 degrees." + "detail": f"Latitude must be between {CoordinateLimits.LAT_MIN} and {CoordinateLimits.LAT_MAX} degrees."
35-38: Update error message to use constants.Similarly, update the longitude error message to use constants instead of hardcoded values.
- "detail": - "Longitude must be between -180 and 180 degrees." + "detail": + f"Longitude must be between {CoordinateLimits.LNG_MIN} and {CoordinateLimits.LNG_MAX} degrees."
30-33: Optimize nested if statements.Per static analysis suggestion, combine the nested if statements for better readability.
- if lng is not None: - if not ( - CoordinateLimits.LNG_MIN <= lng <= CoordinateLimits.LNG_MAX - ): + if lng is not None and not ( + CoordinateLimits.LNG_MIN <= lng <= CoordinateLimits.LNG_MAX + ):🧰 Tools
🪛 Ruff (0.8.2)
30-33: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
src/django/api/constants.py (1)
210-215: Add docstring to document the coordinate limits class.The class would benefit from documentation explaining its purpose and the units of measurement for the coordinates.
class CoordinateLimits: + """ + Defines the minimum and maximum limits for geographic coordinates. + + All values are in decimal degrees where: + - Latitude ranges from -90° (South) to +90° (North) + - Longitude ranges from -180° (West) to +180° (East) + """ LAT_MIN = -90 LAT_MAX = 90 LNG_MIN = -180 LNG_MAX = 180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/django/api/constants.py(1 hunks)src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py(2 hunks)src/django/api/serializers/v1/opensearch_common_validators/geo_bounding_box_validator.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/serializers/v1/opensearch_common_validators/geo_bounding_box_validator.py
🧰 Additional context used
🪛 Ruff (0.8.2)
src/django/api/serializers/v1/opensearch_common_validators/coordinates_validator.py
30-33: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-django-code-quality
roman-stolar
left a comment
roman-stolar
left a comment
There was a problem hiding this comment.
Looks clear, please check 2 issue from SonarQube just to see if it something that need to be changed.
Approved.
…lidator to fix SonarQube errors
|
@roman-stolar Fixed. |
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Approved. Left optional comment.
3 participants
OSDEV-1577 - [API] Add Geo-bounding box support to the production locations endpoint.
Added geo-bounding box query support to the GET
/api/v1/production-locations/endpoint. To filter production locations whose geopoints fall within the bounding box, it is necessary to specify valid values for the parametersgeo_bounding_box[top],geo_bounding_box[left],geo_bounding_box[bottom], andgeo_bounding_box[right].The validation rules are as follows: