[OSDEV-2033] Fix long string parsing for v1/production-locations query param#646
Conversation
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
VadimKovalenkoSNF
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 Walkthrough""" WalkthroughThe changes introduce conditional handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_View
participant QueryDirector
participant QueryBuilder
participant OpenSearch
Client->>API_View: GET /v1/production-locations?query=...
API_View->>QueryDirector: process_multi_match(query)
QueryDirector->>QueryBuilder: add_multi_match(query, slop=3) (if long query)
QueryDirector->>QueryBuilder: add_multi_match(query) (if short query)
QueryBuilder->>OpenSearch: Constructed query with/without slop
OpenSearch-->>API_View: Search results
API_View-->>Client: Response
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)doc/release/RELEASE-NOTES.md (1)undefined <retrieved_learning> 🪛 markdownlint-cli2 (0.17.2)doc/release/RELEASE-NOTES.md15-15: Trailing punctuation in heading (MD026, no-trailing-punctuation) 93-93: Multiple headings with the same content (MD024, no-duplicate-heading) ⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (2)
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: 7
🧹 Nitpick comments (4)
doc/release/RELEASE-NOTES.md (4)
15-15: Remove trailing colon in section heading.The
#### Migrations:heading has an unnecessary trailing colon; remove it to align with Markdown style guidelines (e.g.,#### Migrations).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
18-18: Unify section heading punctuation.Unlike
Migrations:, the#### Schema changesheading omits a colon. Standardize on either including or omitting terminal punctuation across all headings.
24-24: Duplicate section heading detected.The
### Architecture/Environment changesheading appears twice (lines 24 and 33). Remove the redundant instance to prevent confusion.Also applies to: 33-33
28-29: Expand Bugfix entry for clarity.The OSDEV-2033 entry notes support for
slopbut could benefit from specifying theslopvalue used (e.g.,slop=3) and referencing tests or behavioral outcomes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
doc/release/RELEASE-NOTES.md(2 hunks)src/django/api/tests/test_production_locations_query_builder.py(1 hunks)src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py(1 hunks)src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py(1 hunks)src/tests/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/django/api/tests/test_production_locations_query_builder.py (2)
src/django/api/views/v1/opensearch_query_builder/opensearch_query_builder.py (1)
reset(7-8)src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1)
add_multi_match(106-132)
src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (1)
src/django/api/views/v1/parameters_list.py (1)
V1_PARAMETERS_LIST(1-37)
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
15-15: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
44-44: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🔇 Additional comments (8)
src/django/api/tests/test_production_locations_query_builder.py (1)
251-273: LGTM! Comprehensive test coverage for slop functionality.The test correctly verifies:
- Multi-match query structure with slop parameter
- Phrase type when slop is used
- Absence of fuzziness in slop-based queries
- Uses a realistic long query string (52 characters) that triggers the conditional logic
Minor suggestion for improved test clarity:
+ # Test with slop parameter for long queries self.builder.reset()This comment would help clarify the purpose of the second test case within the method.
doc/release/RELEASE-NOTES.md (2)
10-10: Use full month name for release date.The release date is now spelled out (
June) which improves clarity and consistency.
46-46: Use full month name for previous release date.Spelling out the month (
June) for Release 2.6.0 aligns with the formatting choice in Release 2.7.0.src/tests/opensearch/test_opensearch.py (2)
105-108: Good enhancement to test specificity.The updated assertion improves test quality by verifying the exact content of the search result rather than just confirming that results exist. This provides better validation of the search functionality.
110-155: Comprehensive integration test for long query functionality.This test effectively validates the new slop parameter functionality for multi_match queries. The test structure is well-designed:
- Uses a realistic long query that would trigger the slop behavior
- Tests multiple fields (name, address, description, local_name) with appropriate boosting
- Includes filtering by country to match expected production use cases
- Asserts exact address match to verify correct document retrieval
The test data and query structure align well with the PR objective of handling long strings (>50 characters/12 tokens).
src/django/api/views/v1/opensearch_query_builder/production_locations_query_builder.py (3)
106-112: Well-structured field configuration.The field definitions are appropriately organized at the beginning of the method with proper boosting weights. The
namefield receiving a higher boost (^2) makes sense for production location searches.
114-131: Correct implementation of conditional slop handling.The conditional logic properly handles both use cases:
- With slop: Uses
type: 'phrase'which is the correct query type for phrase matching with slop tolerance- Without slop: Maintains backward compatibility using the existing fuzziness approach
This implementation aligns well with OpenSearch best practices where phrase queries with slop are more appropriate for longer, more specific queries, while fuzzy matching works better for shorter queries with potential typos.
132-132:Details
✅ Verification successful
Verify integration with query director logic.
The implementation correctly appends the query to the bool must clause. Ensure that the calling code (opensearch_query_director.py) properly determines when to pass the slop parameter based on the 50-character/12-token thresholds mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Description: Verify the query director integration and slop parameter usage # Expected: Find logic that determines when to pass slop parameter based on query length/token count echo "Searching for slop parameter usage in query director..." rg -A 10 -B 5 "slop.*=.*[0-9]" src/django/api/views/v1/opensearch_query_builder/ echo -e "\nSearching for query length/token threshold logic..." rg -A 10 -B 5 "(50|12).*character|token" src/django/api/views/v1/opensearch_query_builder/ echo -e "\nSearching for add_multi_match calls with slop parameter..." rg -A 5 -B 5 "add_multi_match.*slop" src/django/api/views/v1/opensearch_query_builder/Length of output: 11188
Integration with query director logic verified
All multi‐match calls correctly apply the 12‐token/50‐character threshold and pass
slop=3, and the builder’sadd_multi_match(self, query, slop=None)signature supports an optional slop parameter:
- In
opensearch_query_director.py,__process_multi_matchuses
if len(tokens) > 12 or len(multi_match_query) > 50: self.__builder.add_multi_match(..., slop=3)
otherwise omitsslop.- In
production_locations_query_builder.py,add_multi_match(query, slop=None)accepts and applies the slop value when provided.No further changes required.
src/django/api/views/v1/opensearch_query_builder/opensearch_query_director.py
Show resolved
Hide resolved
mazursasha1990
left a comment
mazursasha1990
left a comment
There was a problem hiding this comment.
In general, looks good.
Left one comment related to the formatting improvement.
|
3 participants
Fix OSDEV-2033
Added support for the
slopparameter inmulti_matchqueries when using strings longer than 50 symbols or 12 tokens in GETv1/production-locations?query=endpoint.