[OSDEV-1913] Fix an error downloading results from the main search.#675
Merged
Innavin369 merged 5 commits intomainfrom Jul 4, 2025
Merged
[OSDEV-1913] Fix an error downloading results from the main search.#675Innavin369 merged 5 commits intomainfrom
Innavin369 merged 5 commits intomainfrom
Conversation
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 34.44%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 WalkthroughWalkthroughThe changes increase the maximum allowed length of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Serializer
participant Model
participant Database
Client->>API: POST /log_download (with path)
API->>Serializer: Validate input
Serializer->>Model: Get path max_length (4096)
alt path length <= 4096
Serializer-->>API: Validated data
API->>Database: Save DownloadLog (path)
API-->>Client: 204 No Content
else path length > 4096
Serializer-->>API: ValidationError (path too long)
API-->>Client: 400 Bad Request (error message)
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)src/django/api/serializers/log_download_query_params.py (14)🧬 Code Graph Analysis (1)src/django/api/serializers/log_download_query_params.py (1)
🪛 Pylint (3.3.7)src/django/api/serializers/log_download_query_params.py[refactor] 10-10: Too few public methods (1/2) (R0903) ⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (3)
src/django/api/migrations/0172_increase_path_max_length.py (1)
3-3: Remove unused import.The import
django.contrib.postgres.indexesis not used in this migration.-import django.contrib.postgres.indexessrc/django/api/tests/test_log_download.py (1)
3-3: Remove unused import.The
ValidationErrorimport is not used in this test file since validation is tested through HTTP requests.-from django.forms import ValidationErrordoc/release/RELEASE-NOTES.md (1)
31-32: Fix indentation – violates MD007 (ul-indent)Static analysis flagged the second-level list as over-indented (4 spaces instead of the project-standard 2). Adjusting keeps Markdown linters quiet and renders consistently.
- * Adjusted the post-submit popup. Instead of displaying the cleaned and transformed data from ContriCleaner, we now show only the raw input submitted by the user. + * Adjusted the post-submit popup. Instead of displaying the cleaned and transformed data from ContriCleaner, we now show only the raw input submitted by the user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
doc/release/RELEASE-NOTES.md(3 hunks)src/django/api/migrations/0172_increase_path_max_length.py(1 hunks)src/django/api/models/download_log.py(1 hunks)src/django/api/serializers/log_download_query_params.py(2 hunks)src/django/api/tests/test_log_download.py(2 hunks)src/django/api/views/log_download.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.
src/django/api/views/log_download.py (2)
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.
src/django/api/serializers/log_download_query_params.py (11)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#488
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py:114-119
Timestamp: 2025-01-29T08:51:47.803Z
Learning: In the Open Supply Hub codebase, request parameter validation is handled in the serializer layer through:
1. Field-level validation using DRF fields (e.g., IntegerField, CharField)
2. Custom validator classes (e.g., SizeValidator which enforces a maximum limit of 250 records)
Adding validation in query builders or directors would be redundant as it's already handled by the serializer layer.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py:20-25
Timestamp: 2024-11-13T07:21:28.686Z
Learning: In the `ModerationIdValidator` class located at `src/django/api/serializers/v1/opensearch_common_validators/moderation_id_validator.py`, it's not necessary to add input size validation for the `moderation_id` list. OpenSearch already imposes a page size limit, which mitigates potential DoS attacks from large inputs.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:12-13
Timestamp: 2024-11-14T11:06:35.337Z
Learning: In `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, input validation for methods like `add_from` is performed at a higher level, so additional validation within these methods is unnecessary.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/moderation_events.py:41-43
Timestamp: 2024-11-12T11:42:07.768Z
Learning: In the `ModerationEvents` API endpoint, parameter validation is handled in `src/django/api/serializers/v1/moderation_events_serializer.py`.
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#488
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py:114-119
Timestamp: 2025-01-29T08:51:47.803Z
Learning: In the Open Supply Hub codebase, request parameter validation is handled in the serializer layer through:
1. Field-level validation using DRF fields (e.g., IntegerField, CharField)
2. Custom validator classes (e.g., SizeValidator)
Adding validation in query builders or directors would be redundant as it's already handled by the serializer layer.
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/models/facility_download_limit.py:64-73
Timestamp: 2025-06-18T12:46:27.549Z
Learning: The validation checks in FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py are insufficient to prevent register_download from receiving more records than the user's available quota. The method only validates against zero total quota on first page and global limits, but doesn't validate records_returned against remaining user quota before calling register_download.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#420
File: src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py:87-92
Timestamp: 2024-11-26T12:24:22.563Z
Learning: In the Open Supply Hub codebase, validation for parameters such as `order_by` is performed in the serializers, specifically in `src/django/api/serializers/v1/moderation_events_serializer.py`, so validation is not needed in the query builder methods like `add_sort`.
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#642
File: src/django/api/facilities_download_view_set.py:115-119
Timestamp: 2025-06-17T10:55:08.363Z
Learning: In the FacilitiesDownloadViewSet.list method in src/django/api/facilities_download_view_set.py, the existing validation checks do not prevent paid_download_records from going negative. The validation only checks if the user has zero total quota (free + paid == 0) on the first page, and if the total query results exceed the global limit of 5000, but does not validate the user's remaining quota against the actual records_to_subtract value which is calculated after pagination.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py:9-11
Timestamp: 2024-11-12T11:15:04.794Z
Learning: In `src/django/api/serializers/v1/opensearch_common_validators/date_range_validator.py`, date format validation is not required within the `DateRangeValidator` class, as date validation is handled elsewhere in the codebase.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py:61-79
Timestamp: 2024-11-14T11:06:37.772Z
Learning: In the `OpenSearchQueryBuilder` class located at `src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py`, date validation is performed at a higher level in the application. Therefore, it's unnecessary to add additional date validation within the `__build_date_range` method.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/tests/test_moderation_events_query_builder.py:86-97
Timestamp: 2024-11-13T07:34:20.109Z
Learning: In our codebase, we raise `ValidationError` instead of `ValueError` to produce an error response with status code 400. Future test cases should reflect this practice.
src/django/api/tests/test_log_download.py (2)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:21-65
Timestamp: 2024-12-12T14:59:19.694Z
Learning: In `src/django/api/tests/test_moderation_events_add_production_location.py`, the tests have been refactored, and the use of `POST` methods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#391
File: src/django/api/tests/test_moderation_events_query_builder.py:86-97
Timestamp: 2024-11-13T07:34:20.109Z
Learning: In our codebase, we raise `ValidationError` instead of `ValueError` to produce an error response with status code 400. Future test cases should reflect this practice.
doc/release/RELEASE-NOTES.md (6)
undefined
<retrieved_learning>
Learnt from: VadimKovalenkoSNF
PR: #420
File: doc/release/RELEASE-NOTES.md:38-38
Timestamp: 2024-11-26T04:59:12.296Z
Learning: For endpoints that haven't been released to end users, it's acceptable to document API changes under the 'Bugfix' section in the release notes.
</retrieved_learning>
<retrieved_learning>
Learnt from: VadimKovalenkoSNF
PR: #420
File: doc/release/RELEASE-NOTES.md:47-54
Timestamp: 2024-11-28T06:36:47.122Z
Learning: API changes and migration details are documented in a separate repository with API specifications, rather than in the release notes.
</retrieved_learning>
<retrieved_learning>
Learnt from: VadimKovalenkoSNF
PR: #420
File: doc/release/RELEASE-NOTES.md:47-54
Timestamp: 2024-11-28T06:36:47.122Z
Learning: Endpoints that have not been enabled to end users do not require migration documentation or old vs new format examples in the release notes.
</retrieved_learning>
<retrieved_learning>
Learnt from: VadimKovalenkoSNF
PR: #641
File: doc/release/RELEASE-NOTES.md:6-35
Timestamp: 2025-06-02T13:24:57.659Z
Learning: The Open Supply Hub team keeps placeholders in release notes until code freeze, then fills in the actual content once all changes are finalized for the release.
</retrieved_learning>
<retrieved_learning>
Learnt from: VadimKovalenkoSNF
PR: #420
File: doc/release/RELEASE-NOTES.md:37-37
Timestamp: 2024-11-25T13:28:23.090Z
Learning: When modifying unreleased API endpoints, such as refactoring the search_after parameter into search_after_value and search_after_id in the OpenSearch implementation, it's acceptable to leave the "What's New" section empty since the changes haven't been released to end users.
</retrieved_learning>
<retrieved_learning>
Learnt from: Innavin369
PR: #431
File: src/django/api/serializers/v1/opensearch_common_validators/countries_validator.py:23-24
Timestamp: 2024-11-28T11:29:28.139Z
Learning: The files src/django/api/tests/test_facility_claim_view_set.py, src/django/api/views/facility/facilities_view_set.py, and src/django/api/facility_actions/processing_facility_api.py are not part of the V1 codebase in the Open Supply Hub project.
</retrieved_learning>
🧬 Code Graph Analysis (2)
src/django/api/views/log_download.py (1)
src/django/api/constants.py (1)
LogDownloadQueryParams(147-149)
src/django/api/tests/test_log_download.py (2)
src/django/api/models/download_log.py (1)
DownloadLog(4-24)src/django/api/constants.py (1)
LogDownloadQueryParams(147-149)
🪛 Flake8 (7.2.0)
src/django/api/migrations/0172_increase_path_max_length.py
[error] 3-3: 'django.contrib.postgres.indexes' imported but unused
(F401)
src/django/api/tests/test_log_download.py
[error] 3-3: 'django.forms.ValidationError' imported but unused
(F401)
🪛 Ruff (0.11.9)
src/django/api/migrations/0172_increase_path_max_length.py
3-3: django.contrib.postgres.indexes imported but unused
Remove unused import: django.contrib.postgres.indexes
(F401)
src/django/api/tests/test_log_download.py
3-3: django.forms.ValidationError imported but unused
Remove unused import: django.forms.ValidationError
(F401)
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
31-31: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (5)
src/django/api/views/log_download.py (1)
27-31: Formatting improvement looks good.The added blank lines improve readability around the variable assignments.
src/django/api/models/download_log.py (1)
15-15: Model field update correctly addresses the issue.Increasing
max_lengthfrom 2083 to 4096 characters should resolve the "too long value" errors for path downloads as described in OSDEV-1913.src/django/api/migrations/0172_increase_path_max_length.py (1)
14-18: Migration correctly updates the field constraint.The
AlterFieldoperation properly increases themax_lengthto 4096 characters to match the model change.src/django/api/tests/test_log_download.py (1)
68-98: Excellent test coverage for the path length validation.The test comprehensively covers both valid and invalid cases:
- Dynamically retrieves max_length from the model field
- Tests path exactly at the limit (should succeed)
- Tests path exceeding the limit (should fail with appropriate error)
- Verifies the specific error message
doc/release/RELEASE-NOTES.md (1)
15-16: Migration entry looks goodThe new migration note is concise and follows the existing format.
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.75%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Django App | Unittest test suite - Code coverage reportTotal: 80.8%Your code coverage diff: 0.02% ▴ ✅ All code changes are covered |
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
suggested changes
Jul 3, 2025
vladsha-dev
suggested changes
Jul 3, 2025
Contributor
vladsha-dev
left a comment
vladsha-dev
left a comment
There was a problem hiding this comment.
Could you please provide the PR title in a format that follows our Engineering playbook? Something like: “Fixed issue when…”
Innavin369
changed the title
VadimKovalenkoSNF
requested changes
Jul 4, 2025
Contributor
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Pls, apply minor fix.
added 2 commits
July 4, 2025 09:55
…/opensupplyhub/open-supply-hub into OSDEV-1913-fix-path-length-in-db
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Innavin369
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OSDEV-1913 Error downloading results from main search.
The
max_lengthfor thepathfield in theDownloadLogmodel has been increased from 2083 to 4096 to fix the too long value error.Added migration 0172_increase_path_max_length which increases
max_lengthfor thepathfield in theDownloadLogmodel.