Conversation
📝 WalkthroughWalkthroughIntroduces a boolean config to enable routing pattern-based (wildcard) string searches to dedicated Elasticsearch wildcard-type fields. Adds multimethods and resolvers for wildcard and lowercase-wildcard field mappings and updates StringCondition conversion to use them when the feature flag is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryProcessor as Query Processor
participant FieldMapper as Field Mapper
participant ES as Elasticsearch
Client->>QueryProcessor: Submit StringCondition (pattern or exact)
QueryProcessor->>QueryProcessor: Is pattern? Check and read config
alt Pattern & Config Enabled
QueryProcessor->>FieldMapper: Resolve wildcard field
FieldMapper->>FieldMapper: Lookup wildcard / lowercase-wildcard mapping
FieldMapper-->>QueryProcessor: Return wildcard field name
else Non-pattern or Config Disabled
QueryProcessor->>FieldMapper: Resolve standard field
FieldMapper->>FieldMapper: Lookup keyword / lowercase mapping
FieldMapper-->>QueryProcessor: Return standard field name
end
QueryProcessor->>ES: Send built query using selected field
ES-->>Client: Return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
zimzoom
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elastic-utils-lib/src/cmr/elastic_utils/config.clj (1)
106-112: Missing(declare enable-wildcard-field-searches)for consistency with file convention.Every other
defconfigin this file is preceded by a(declare …)form (e.g., lines 8, 20, 100). Add the matching declare to keep this file consistent and avoid clj-kondo warnings about forward references.♻️ Proposed fix
+(declare enable-wildcard-field-searches) (defconfig enable-wildcard-field-searches "When true, pattern searches (wildcards) will use dedicated Elasticsearch wildcard-type fields for improved performance. When false, pattern searches will use the standard keyword fields. This should be enabled only after all indices have been reindexed to populate the wildcard fields." {:default false :type Boolean})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elastic-utils-lib/src/cmr/elastic_utils/config.clj` around lines 106 - 112, Add a forward declare for the defconfig symbol to match the file convention and avoid clj-kondo warnings: insert a (declare enable-wildcard-field-searches) immediately before the defconfig for enable-wildcard-field-searches so the symbol is declared prior to its definition (consistent with other declarations like the ones near lines 8, 20, 100).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@elastic-utils-lib/src/cmr/elastic_utils/config.clj`:
- Around line 106-112: Add a forward declare for the defconfig symbol to match
the file convention and avoid clj-kondo warnings: insert a (declare
enable-wildcard-field-searches) immediately before the defconfig for
enable-wildcard-field-searches so the symbol is declared prior to its definition
(consistent with other declarations like the ones near lines 8, 20, 100).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ef0cb63-d675-445c-b273-d64f1d6d0611
📒 Files selected for processing (4)
elastic-utils-lib/src/cmr/elastic_utils/config.cljelastic-utils-lib/src/cmr/elastic_utils/search/es_query_to_elastic.cljsearch-app/src/cmr/search/data/query_to_elastic.cljsearch-app/test/cmr/search/test/unit/data/query_to_elastic_converters/granule_wildcard_fields.clj
|
|
||
| (defmethod field->wildcard-field-mappings :default | ||
| [_] | ||
| ;; No wildcard field mappings specified by default |
There was a problem hiding this comment.
comment doesn't really add much
There was a problem hiding this comment.
This comment follows the style of the other default methods in this area of the code base
There was a problem hiding this comment.
if you really like the comments you may keep them
|
|
||
| (defmethod field->lowercase-wildcard-field-mappings :default | ||
| [_] | ||
| ;; No lowercase wildcard field mappings specified by default |
There was a problem hiding this comment.
comment doesn't really add much
There was a problem hiding this comment.
This comment follows the style of the other default methods in this area of the code base
| (cond | ||
| case-sensitive? (field->wildcard-field concept-type field) | ||
| :else (field->lowercase-wildcard-field concept-type field)) | ||
| ;; Original behavior: use standard fields |
There was a problem hiding this comment.
these comments don't add much
There was a problem hiding this comment.
These comments delineate a feature flag that will be removed soon
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2415 +/- ##
=======================================
Coverage 57.90% 57.90%
=======================================
Files 1067 1067
Lines 73475 73502 +27
Branches 2129 2127 -2
=======================================
+ Hits 42544 42564 +20
- Misses 28950 28953 +3
- Partials 1981 1985 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
search-app/test/cmr/search/test/unit/data/query_to_elastic_converters/granule_wildcard_fields.clj (1)
31-40: Consider adding test coverage for:producer-gran-idalias.The wildcard field mappings define both
:producer-gran-idand:producer-granule-idaliases (lines 313-314 and 319-320 in the implementation file), but the tests only exercise:producer-granule-id. Adding a test for:producer-gran-idwould verify both aliases work correctly.🧪 Suggested additional test cases
(testing "producer-gran-id alias uses same wildcard field" (is (= {:wildcard {"producer-granule-id-lowercase-wildcard" "prod*"}} (q2e/condition->elastic (qm/string-condition :producer-gran-id "Prod*" false true) :granule)))) (testing "producer-gran-id alias case-sensitive uses same wildcard field" (is (= {:wildcard {:producer-granule-id-wildcard "Prod*"}} (q2e/condition->elastic (qm/string-condition :producer-gran-id "Prod*" true true) :granule))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@search-app/test/cmr/search/test/unit/data/query_to_elastic_converters/granule_wildcard_fields.clj` around lines 31 - 40, Add parallel tests for the :producer-gran-id alias mirroring the existing :producer-granule-id tests: call qm/string-condition with :producer-gran-id and the same inputs used in the two tests (case-insensitive and case-sensitive wildcard forms) and assert q2e/condition->elastic returns the same wildcard maps ("producer-granule-id-lowercase-wildcard" for case-insensitive and "producer-granule-id-wildcard" for case-sensitive) so both aliases are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@search-app/test/cmr/search/test/unit/data/query_to_elastic_converters/granule_wildcard_fields.clj`:
- Around line 31-40: Add parallel tests for the :producer-gran-id alias
mirroring the existing :producer-granule-id tests: call qm/string-condition with
:producer-gran-id and the same inputs used in the two tests (case-insensitive
and case-sensitive wildcard forms) and assert q2e/condition->elastic returns the
same wildcard maps ("producer-granule-id-lowercase-wildcard" for
case-insensitive and "producer-granule-id-wildcard" for case-sensitive) so both
aliases are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33262db1-9f98-42f8-84bc-e6da9784ecb5
📒 Files selected for processing (4)
elastic-utils-lib/src/cmr/elastic_utils/config.cljelastic-utils-lib/src/cmr/elastic_utils/search/es_query_to_elastic.cljsearch-app/src/cmr/search/data/query_to_elastic.cljsearch-app/test/cmr/search/test/unit/data/query_to_elastic_converters/granule_wildcard_fields.clj
🚧 Files skipped from review as they are similar to previous changes (2)
- elastic-utils-lib/src/cmr/elastic_utils/config.clj
- elastic-utils-lib/src/cmr/elastic_utils/search/es_query_to_elastic.clj
3 participants
Overview
What is the objective?
Adds support for using Elasticsearch wildcard-type fields for granule wildcard searches, gated behind a feature flag so the behavior can be enabled only after the relevant indices are reindexed.
What are the changes?
Add feature flag defconfig
enable-wildcard-field-searches-- when enabled, granule pattern searches now target dedicated wildcard fields for granule-ur and producer-granule-id, with separate case-sensitive and lowercase mappings. When the flag is disabled, wildcard searches continue using the existing standard fields, preserving prior behavior. Exact non-pattern searches are unchanged.Adds wildcard-field mapping helpers in elastic-utils-lib, granule-specific wildcard mappings in search-app, and unit coverage for enabled and disabled toggle behavior, including readable granule name handling.
What areas of the application does this impact?
search-app, elastic-utils-lib
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Tests