CMR-10886: Granule counts are different for certain shapefiles when comparing include_granule_counts with the collections endpoint vs the granules endpoint #2348
Conversation
WalkthroughDetects OR groups composed solely of SpatialCondition nodes and preserves those OR groups during spatial-condition extraction; adds unit tests, a MultiPolygon GeoJSON fixture, and an integration test that verifies shapefile-based granule counts. Changes
Sequence Diagram(s)sequenceDiagram
participant Q as Query AST
participant E as extract-spatial-conditions
participant V as path-validator
participant R as Result
Note over Q,E: Preserve OR groups composed only of SpatialCondition nodes
Q->>E: provide query tree
E->>E: traverse tree, identify OR groups & SpatialCondition nodes
alt OR group contains only SpatialCondition
E->>V: validate group's parent path(s)
V-->>E: validated
E->>R: include entire OR group in results
else single/ungrouped SpatialCondition
E->>V: validate condition's parent path
V-->>E: validated
E->>R: include single SpatialCondition in results
end
Note right of R: returns concatenated [spatial OR groups + ungrouped SpatialConditions]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 (2)
system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj (1)
757-794: MultiPolygon shapefile test is solid; consider resetting the shapefile flag to avoid state leakage.The test correctly:
- enables the shapefile parameter,
- ingests granules inside and outside the MultiPolygon,
- compares direct granule hits to collection-level
include_granule_counts.One concern:
set-enable-shapefile-parameter-flag!is set totrueviaside/eval-formand never reset, which can leak configuration into subsequent system-int tests.Wrapping the body in a
try/finallyand resetting the flag keeps the test self-contained:(deftest multipolygon-shapefile-granule-counts-test - (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! true)) - - (let [coll (make-coll 1 m/whole-world nil)] - ;; Granule in first polygon of MultiPolygon - (make-gran coll (p/point 20.0 5.0) nil) - ;; Granule in second polygon of MultiPolygon - (make-gran coll (p/point 47.0 -20.0) nil) - ;; Granule outside MultiPolygon - (make-gran coll (p/point -100.0 40.0) nil) - ... - (testing "MultiPolygon shapefile granule counts match direct granule search" - ... - (is (gran-counts/granule-counts-match? :xml {coll granule-hits} collection-response) - "MultiPolygon shapefile: collection granule counts should match granule search hits")))))) + (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! true)) + (try + (let [coll (make-coll 1 m/whole-world nil)] + ;; Granule in first polygon of MultiPolygon + (make-gran coll (p/point 20.0 5.0) nil) + ;; Granule in second polygon of MultiPolygon + (make-gran coll (p/point 47.0 -20.0) nil) + ;; Granule outside MultiPolygon + (make-gran coll (p/point -100.0 40.0) nil) + ... + (testing "MultiPolygon shapefile granule counts match direct granule search" + ... + (is (gran-counts/granule-counts-match? :xml {coll granule-hits} collection-response) + "MultiPolygon shapefile: collection granule counts should match granule search hits"))) + (finally + (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! false)))))(You can inline the
...portions from the existing test body.)This keeps global shapefile configuration from accidentally affecting other tests.
search-app/test/cmr/search/test/unit/services/query_execution/granule_counts_results_feature_test.clj (1)
12-139: Good coverage of spatial grouping semantics; consider an optional negative-path test.The helper
make-spatial-conditionand the suite of tests aroundis-spatial-or-group?andextract-spatial-conditionsthoroughly exercise:
- OR vs AND ConditionGroups,
- mixed spatial/non-spatial groups,
- nested conditions, and
- duplicate polygon scenarios.
Once
extract-spatial-conditionsrestoresvalidate-path-to-conditionfor ungroupedSpatialConditions (see comment ingranule-counts-results-feature.clj), you might optionally add a small test that constructs a query where a spatial condition is wrapped in an unsupported parent (e.g., negation or other disallowed operator) and asserts that extraction fails as expected. That would fully lock in the intended safety behavior around parent-path validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
search-app/src/cmr/search/services/query_execution/granule_counts_results_feature.clj(1 hunks)search-app/test/cmr/search/test/unit/services/query_execution/granule_counts_results_feature_test.clj(1 hunks)system-int-test/resources/shapefiles/multipolygon_test.geojson(1 hunks)system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj(2 hunks)
🔇 Additional comments (2)
system-int-test/resources/shapefiles/multipolygon_test.geojson (1)
1-32: GeoJSON MultiPolygon test resource looks structurally sound.Rings are closed, coordinates are in lon/lat order, and the MultiPolygon matches the two-rectangle description used in the tests. No changes needed.
system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj (1)
4-9: New test dependencies align with MultiPolygon shapefile usage.The added requires (
io,mt,side,shapefile) match the new shapefile-based granule count test and look appropriate.
search-app/src/cmr/search/services/query_execution/granule_counts_results_feature.clj
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2348 +/- ##
==========================================
+ Coverage 58.17% 58.20% +0.02%
==========================================
Files 1063 1063
Lines 72197 72230 +33
Branches 2084 2081 -3
==========================================
+ Hits 42003 42038 +35
+ Misses 28254 28252 -2
Partials 1940 1940 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eereiter
left a comment
eereiter
left a comment
There was a problem hiding this comment.
Please check the coderabbit suggestion of inserting (validate-path-to-condition query condition-path) on line 95 in search-app/src/cmr/search/services/query_execution/granule_counts_results_feature.clj to make sure that wasn't missed, and possilbly write a test that covers that scenario if it was missed.
daniel-zamora
force-pushed
the
CMR-10886
branch
2 times, most recently
from
731c7ea to
10928ad
Compare
daniel-zamora
force-pushed
the
CMR-10886
branch
from
10928ad to
7b2b754
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj (1)
757-794: Test logic is sound; consider adding cleanup for the global shapefile flag.The test correctly verifies that MultiPolygon shapefile-based granule counts in collection searches match the counts from direct granule searches. The test structure follows existing patterns and uses appropriate assertions.
However, line 758 enables the shapefile parameter flag globally without cleanup. If this flag persists across tests, it could affect subsequent test behavior.
Consider adding cleanup to restore the original flag state:
(deftest multipolygon-shapefile-granule-counts-test - (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! true)) - - (let [coll (make-coll 1 m/whole-world nil)] + (let [original-flag (side/eval-form `(shapefile/get-enable-shapefile-parameter-flag)) + _ (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! true))] + (try + (let [coll (make-coll 1 m/whole-world nil)] + ;; ... rest of test logic ... + ) + (finally + (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! ~original-flag))))))Note: If the
use-fixturesalready handles this flag reset, or if a getter for the flag doesn't exist, then this cleanup may not be necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
search-app/src/cmr/search/services/query_execution/granule_counts_results_feature.clj(1 hunks)search-app/test/cmr/search/test/unit/services/query_execution/granule_counts_results_feature_test.clj(1 hunks)system-int-test/resources/shapefiles/multipolygon_test.geojson(1 hunks)system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- system-int-test/resources/shapefiles/multipolygon_test.geojson
- search-app/test/cmr/search/test/unit/services/query_execution/granule_counts_results_feature_test.clj
🔇 Additional comments (3)
search-app/src/cmr/search/services/query_execution/granule_counts_results_feature.clj (2)
66-74: LGTM! Clean predicate for identifying spatial OR groups.The function correctly identifies OR groups that should preserve their structure for MultiPolygon geometries. The logic validates all necessary conditions: instance type, OR operation, non-empty conditions, and that every child is a SpatialCondition.
76-98: LGTM! Two-step extraction preserves OR semantics and includes proper validation.The refactored logic correctly:
- Preserves OR groups containing only SpatialConditions (fixing the MultiPolygon issue)
- Extracts standalone SpatialConditions not part of such groups
- Validates paths for both spatial OR groups (line 86) and ungrouped conditions (line 95)
- Prevents double-extraction by checking parent type (line 94)
This addresses the past review concern about missing path validation while maintaining the fix for MultiPolygon granule counts.
system-int-test/test/cmr/system_int_test/search/granule/granule_counts_search_test.clj (1)
4-9: LGTM! Necessary imports for shapefile test.The new imports support the MultiPolygon shapefile test:
iofor resource loading,mtfor MIME types,sidefor remote flag configuration, andshapefilefor the parameter flag.
Overview
What is the objective?
Fix incorrect granule counts when searching collections with MultiPolygon shapefiles. The collections endpoint with include_granule_counts=true was returning 0 or incorrect counts for MultiPolygon shapefiles, while the granules endpoint correctly found matching granules. This occurred because MultiPolygon OR semantics were being broken during spatial condition extraction, causing the query to incorrectly require granules to match ALL polygons instead of ANY polygon.
What are the changes?
Refactored the extract-spatial-conditions function to preserve OR group structure for MultiPolygon shapefiles.
What areas of the application does this impact?
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.