CMR-10528: As a CMR Search API user I only want to see active collections in my search results.#2385
Open
daniel-zamora wants to merge 2 commits intomasterfrom
Open
CMR-10528: As a CMR Search API user I only want to see active collections in my search results.#2385daniel-zamora wants to merge 2 commits intomasterfrom
daniel-zamora wants to merge 2 commits intomasterfrom
Conversation
…erational filter with feature flag
📝 WalkthroughWalkthroughAdds a computed Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Search API
participant Config as Config Service
participant QueryParser as Param Parser
participant ES as Elasticsearch
Client->>API: GET /collections?include_non_operational=...
API->>Config: read enable-non-operational-collection-filter
API->>QueryParser: parse params (apply defaulting using config & aliases)
QueryParser->>API: parameter conditions (may include collection-progress-active filter)
API->>ES: search query (includes/excludes non-operational based on condition)
ES-->>API: results
API-->>Client: filtered collection results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj (1)
51-52: String literal afterdeftestname is not a docstring.
deftestdoesn't support docstrings the waydefndoes. The string on line 52 is evaluated and discarded at runtime. If you want to document the test, use a;; commentinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj` around lines 51 - 52, The string literal placed immediately after the deftest name in search-collection-progress-active-filter is not a docstring and is being evaluated/discarded; remove that string literal and replace it with an inline comment (;; ...) or move the explanatory text outside the deftest, ensuring the test definition begins with its body (e.g., setup/assertions) and not a quoted string so the test runs as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj`:
- Around line 68-91: The test sets the feature flag via
(cmr.search.config/set-enable-non-operational-collection-filter! true) but only
resets it at the end, so a failing assertion will leave the flag enabled; wrap
the block of assertions between the enable and disable calls in a try/finally so
the flag is always reset: call
(cmr.search.config/set-enable-non-operational-collection-filter! true), run the
tests (the testing forms that call search/find-refs), and in a finally call
(cmr.search.config/set-enable-non-operational-collection-filter! false) to
ensure the flag is cleared even on failure.
---
Nitpick comments:
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj`:
- Around line 51-52: The string literal placed immediately after the deftest
name in search-collection-progress-active-filter is not a docstring and is being
evaluated/discarded; remove that string literal and replace it with an inline
comment (;; ...) or move the explanatory text outside the deftest, ensuring the
test definition begins with its body (e.g., setup/assertions) and not a quoted
string so the test runs as intended.
Comment on lines
+68
to
+91
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true)) | ||
|
|
||
| (testing "flag ON, no params - only active collections returned (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)" | ||
| (d/refs-match? [coll1 coll3 coll5 coll8] | ||
| (search/find-refs :collection {}))) | ||
|
|
||
| (testing "flag ON + include-non-operational=true - all 8 collections returned" | ||
| (d/refs-match? [coll1 coll2 coll3 coll4 coll5 coll6 coll7 coll8] | ||
| (search/find-refs :collection {:include-non-operational "true"}))) | ||
|
|
||
| (testing "flag ON + include-non-operational=false - only active collections returned" | ||
| (d/refs-match? [coll1 coll3 coll5 coll8] | ||
| (search/find-refs :collection {:include-non-operational "false"}))) | ||
|
|
||
| (testing "flag ON + explicit collection-progress=PLANNED - PLANNED returned (no synthetic filter injected)" | ||
| (d/refs-match? [coll2] | ||
| (search/find-refs :collection {:collection-progress "PLANNED"}))) | ||
|
|
||
| (testing "flag ON + collection-progress=PLANNED + include-non-operational=false - empty (PLANNED is non-operational)" | ||
| (d/refs-match? [] | ||
| (search/find-refs :collection {:collection-progress "PLANNED" | ||
| :include-non-operational "false"}))) | ||
|
|
||
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false)))) |
There was a problem hiding this comment.
Feature flag not reset on test failure — can poison subsequent tests.
If any assertion between set-...-filter! true (line 68) and set-...-filter! false (line 91) throws an exception, the flag stays enabled and can cause cascading failures in later tests. Wrap in try/finally:
🛡️ Proposed fix
(side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true))
- (testing "flag ON, no params - only active collections returned (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)"
- (d/refs-match? [coll1 coll3 coll5 coll8]
- (search/find-refs :collection {})))
-
- (testing "flag ON + include-non-operational=true - all 8 collections returned"
- (d/refs-match? [coll1 coll2 coll3 coll4 coll5 coll6 coll7 coll8]
- (search/find-refs :collection {:include-non-operational "true"})))
-
- (testing "flag ON + include-non-operational=false - only active collections returned"
- (d/refs-match? [coll1 coll3 coll5 coll8]
- (search/find-refs :collection {:include-non-operational "false"})))
-
- (testing "flag ON + explicit collection-progress=PLANNED - PLANNED returned (no synthetic filter injected)"
- (d/refs-match? [coll2]
- (search/find-refs :collection {:collection-progress "PLANNED"})))
-
- (testing "flag ON + collection-progress=PLANNED + include-non-operational=false - empty (PLANNED is non-operational)"
- (d/refs-match? []
- (search/find-refs :collection {:collection-progress "PLANNED"
- :include-non-operational "false"})))
-
- (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false))))
+ (try
+ (testing "flag ON, no params - only active collections returned (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)"
+ (d/refs-match? [coll1 coll3 coll5 coll8]
+ (search/find-refs :collection {})))
+
+ (testing "flag ON + include-non-operational=true - all 8 collections returned"
+ (d/refs-match? [coll1 coll2 coll3 coll4 coll5 coll6 coll7 coll8]
+ (search/find-refs :collection {:include-non-operational "true"})))
+
+ (testing "flag ON + include-non-operational=false - only active collections returned"
+ (d/refs-match? [coll1 coll3 coll5 coll8]
+ (search/find-refs :collection {:include-non-operational "false"})))
+
+ (testing "flag ON + explicit collection-progress=PLANNED - PLANNED returned (no synthetic filter injected)"
+ (d/refs-match? [coll2]
+ (search/find-refs :collection {:collection-progress "PLANNED"})))
+
+ (testing "flag ON + collection-progress=PLANNED + include-non-operational=false - empty (PLANNED is non-operational)"
+ (d/refs-match? []
+ (search/find-refs :collection {:collection-progress "PLANNED"
+ :include-non-operational "false"})))
+ (finally
+ (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false))))))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true)) | |
| (testing "flag ON, no params - only active collections returned (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)" | |
| (d/refs-match? [coll1 coll3 coll5 coll8] | |
| (search/find-refs :collection {}))) | |
| (testing "flag ON + include-non-operational=true - all 8 collections returned" | |
| (d/refs-match? [coll1 coll2 coll3 coll4 coll5 coll6 coll7 coll8] | |
| (search/find-refs :collection {:include-non-operational "true"}))) | |
| (testing "flag ON + include-non-operational=false - only active collections returned" | |
| (d/refs-match? [coll1 coll3 coll5 coll8] | |
| (search/find-refs :collection {:include-non-operational "false"}))) | |
| (testing "flag ON + explicit collection-progress=PLANNED - PLANNED returned (no synthetic filter injected)" | |
| (d/refs-match? [coll2] | |
| (search/find-refs :collection {:collection-progress "PLANNED"}))) | |
| (testing "flag ON + collection-progress=PLANNED + include-non-operational=false - empty (PLANNED is non-operational)" | |
| (d/refs-match? [] | |
| (search/find-refs :collection {:collection-progress "PLANNED" | |
| :include-non-operational "false"}))) | |
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false)))) | |
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true)) | |
| (try | |
| (testing "flag ON, no params - only active collections returned (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)" | |
| (d/refs-match? [coll1 coll3 coll5 coll8] | |
| (search/find-refs :collection {}))) | |
| (testing "flag ON + include-non-operational=true - all 8 collections returned" | |
| (d/refs-match? [coll1 coll2 coll3 coll4 coll5 coll6 coll7 coll8] | |
| (search/find-refs :collection {:include-non-operational "true"}))) | |
| (testing "flag ON + include-non-operational=false - only active collections returned" | |
| (d/refs-match? [coll1 coll3 coll5 coll8] | |
| (search/find-refs :collection {:include-non-operational "false"}))) | |
| (testing "flag ON + explicit collection-progress=PLANNED - PLANNED returned (no synthetic filter injected)" | |
| (d/refs-match? [coll2] | |
| (search/find-refs :collection {:collection-progress "PLANNED"}))) | |
| (testing "flag ON + collection-progress=PLANNED + include-non-operational=false - empty (PLANNED is non-operational)" | |
| (d/refs-match? [] | |
| (search/find-refs :collection {:collection-progress "PLANNED" | |
| :include-non-operational "false"}))) | |
| (finally | |
| (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false)))))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj`
around lines 68 - 91, The test sets the feature flag via
(cmr.search.config/set-enable-non-operational-collection-filter! true) but only
resets it at the end, so a failing assertion will leave the flag enabled; wrap
the block of assertions between the enable and disable calls in a try/finally so
the flag is always reset: call
(cmr.search.config/set-enable-non-operational-collection-filter! true), run the
tests (the testing forms that call search/find-refs), and in a finally call
(cmr.search.config/set-enable-non-operational-collection-filter! false) to
ensure the flag is cleared even on failure.
Contributor
Author
There was a problem hiding this comment.
other tests use this pattern, and if any exception or error is thrown by the apps its handled gracefull by the apps and the dev system itself remains running
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for similar patterns where side/eval-form is used to toggle feature flags
# Look for patterns where flags are set and then reset at the end without try/finally
rg -n -C5 'side/eval-form.*set-enable' --type=clj
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Also search for any try/finally patterns in test files to see if they're used
rg -n -C3 '\(try\b' system-int-test/test/ --type=clj | head -100
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find other tests that toggle configuration or state without cleanup protection
rg -n -A10 'side/eval-form' system-int-test/test/ --type=clj | head -150
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for similar patterns where side/eval-form is used to toggle feature flags
rg -n -C5 'side/eval-form' -g '*.clj' system-int-test/test/ | head -200
Length of output: 28251
🏁 Script executed:
#!/bin/bash
# Check if try/finally is commonly used in test files
rg -n -C3 '\(try\b' -g '*.clj' system-int-test/test/ | head -100
Length of output: 8313
@daniel-zamora, thank you for the clarification! I can confirm that pattern exists throughout the test suite. I found numerous examples where side/eval-form is used to toggle configuration without try/finally blocks, including tests like multipolygon-shapefile-granule-counts-test, subscription-count-exceeds-limit-test, and others.
Given that this is an established pattern in the codebase and you've confirmed the exception handling works gracefully in your environment, I understand the current approach is intentional and appropriate for your testing infrastructure. I appreciate you taking the time to explain the context!
🧠 Learnings used
Learnt from: daniel-zamora
Repo: nasa/Common-Metadata-Repository PR: 2361
File: search-app/src/cmr/search/services/parameters/conversion.clj:473-487
Timestamp: 2025-12-30T13:06:11.311Z
Learning: The collection-progress field in CMR collections is a single-valued enum. A collection can only have one progress value at a time, so AND operations on collection-progress parameters don't make logical sense even though the implementation supports them for consistency with other parameter handlers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@search-app/docs/api.md`:
- Line 2386: Replace the indented code block containing the curl example ("curl
\"%CMR-ENDPOINT%/collections?include_non_operational=true\"") with a fenced code
block labeled with the language (e.g., bash) to satisfy markdownlint MD046;
locate the curl snippet in the API docs and wrap it with triple backticks and
the language tag (bash) so the snippet is a proper fenced block.
| * `false` — restrict results to operational collections only (`ACTIVE`, `COMPLETE`, `NOT PROVIDED`, `SUPERSEDED`, and collections with no `CollectionProgress` set) | ||
| * `unset` (default) — behavior is determined by the server-side feature flag | ||
|
|
||
| curl "%CMR-ENDPOINT%/collections?include_non_operational=true" |
There was a problem hiding this comment.
Use fenced code block to satisfy markdownlint (MD046).
Line 2386 is an indented code block; markdownlint expects fenced blocks.
📌 Suggested fix
- curl "%CMR-ENDPOINT%/collections?include_non_operational=true"
+```bash
+curl "%CMR-ENDPOINT%/collections?include_non_operational=true"
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| curl "%CMR-ENDPOINT%/collections?include_non_operational=true" |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 2386-2386: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@search-app/docs/api.md` at line 2386, Replace the indented code block
containing the curl example ("curl
\"%CMR-ENDPOINT%/collections?include_non_operational=true\"") with a fenced code
block labeled with the language (e.g., bash) to satisfy markdownlint MD046;
locate the curl snippet in the API docs and wrap it with triple backticks and
the language tag (bash) so the snippet is a proper fenced block.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2385 +/- ##
==========================================
- Coverage 57.94% 57.93% -0.01%
==========================================
Files 1066 1067 +1
Lines 73211 73228 +17
Branches 2121 2119 -2
==========================================
+ Hits 42420 42424 +4
- Misses 28818 28828 +10
- Partials 1973 1976 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
2 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.
Overview
What is the objective?
Add a default filter that excludes non-operational collections (those with CollectionProgress of PLANNED, DEPRECATED, PREPRINT, or INREVIEW) from collection search results. The filter is controlled by a feature flag and is off by default, allowing it to be enabled without a code deployment.
What are the changes?
What areas of the application does this impact?
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Documentation
Tests