Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant QuerySvc as QueryService
participant ParamVal as ParameterValidator
participant ShpConv as ShapefileConverter
participant GeoConv as GeometryConverter
participant DB as Database
Client->>QuerySvc: HTTP search request (may include force-cartesian)
QuerySvc->>ParamVal: create-and-validate-concepts-query-parameters
ParamVal-->>QuerySvc: validated-params (includes force-cartesian)
QuerySvc->>QuerySvc: assoc :params into context
QuerySvc->>ShpConv: parse-parameter-query (context with :params)
ShpConv->>GeoConv: features->conditions / geometry->conditions (options includes force-cartesian)
GeoConv->>GeoConv: derive coord-sys from options (:cartesian or :geodetic)
GeoConv-->>ShpConv: geometry conditions (with chosen coord system)
ShpConv-->>QuerySvc: assembled query conditions
QuerySvc->>DB: execute query with conditions
DB-->>QuerySvc: results
QuerySvc-->>Client: search response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 5
🤖 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/src/cmr/search/services/parameters/converters/geometry.clj`:
- Around line 80-87: The issue is that geometry->condition still calls the
one-arity line->shape so :force-cartesian in options is ignored for "LineString"
and "LinearRing"; update geometry->condition to pass the same options map
through to line->shape (call the two-arity version) when handling "LineString"
and "LinearRing" so coord-sys respects :force-cartesian, and make the same
change for the other occurrences noted (around the code at the referenced 92-93
region) to consistently forward options.
In `@search-app/src/cmr/search/services/parameters/converters/shapefile.clj`:
- Around line 367-374: The force-cartesian handling in the
p/parameter->condition :shapefile method currently only treats the exact string
"true" as true, so mixed-case inputs like "TRUE" are ignored; update the logic
that computes force-cartesian from force-cartesian-value (in the
p/parameter->condition method when enable-shapefile-parameter-flag is true) to
perform a case-insensitive boolean check (e.g., normalize strings to lower-case
or use a case-insensitive comparison) and still accept actual boolean true
values, then pass the normalized boolean in shapefile-context to
shapefile->conditions.
In `@search-app/src/cmr/search/services/parameters/parameter_validation.clj`:
- Around line 41-42: The :force-cartesian parameter is declared as a boolean but
missing from boolean-value-validation, so non-boolean values slip through;
update the boolean-value-validation set to include :force-cartesian (and any
other validation sets where it should be present) so requests like
force_cartesian=maybe are rejected with a 400, and ensure the validation flow
used by validate-parameters (or the function that resolves boolean params)
enforces boolean parsing/validation consistently where :force-cartesian is
handled.
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.clj`:
- Around line 147-182: The current are3 matrix in the test around the (testing
... "Search with force-cartesian parameter using %s shapefile") only verifies
parameter acceptance because every row uses the same expected-items; update the
test to include at least one shapefile case where geodetic vs cartesian
interpretation yields different results and assert different expected-items for
force-cartesian=true vs false. Specifically, add a new test row in the are3
block for a shapefile (e.g., one that triggers different matches when
interpreted cartesian) and call search/find-refs-with-multi-part-form-post with
params that include {:name "force-cartesian" :content "true"} and {:name
"force-cartesian" :content "false"} (and nil case if desired), using distinct
expected-items arrays for the true vs false cases so the test fails if the
converter ignores force-cartesian.
In
`@system-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj`:
- Around line 260-295: The test block "Search with force-cartesian parameter
using %s shapefile" currently asserts identical expected refs for nil/false/true
so it won't detect regressions; update the test (around the are3 that calls
search/find-refs-with-multi-part-form-post and uses d/assert-refs-match) to
include at least one shapefile fixture whose geodetic vs cartesian
interpretations yield different granule refs: add a new shapefile in resources
(e.g., shapefiles/<name>.<extension>) that crosses the dateline or uses
projected coordinates, then add distinct expected-items arrays for the
force-cartesian=true case versus the false/nil cases so the three branches
assert different results, and ensure the new fixture is referenced in the are3
test parameters and expected refs are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1d97d85-a227-42d7-93c9-6cfd3af18e68
📒 Files selected for processing (8)
search-app/src/cmr/search/services/parameters/conversion.cljsearch-app/src/cmr/search/services/parameters/converters/geometry.cljsearch-app/src/cmr/search/services/parameters/converters/shapefile.cljsearch-app/src/cmr/search/services/parameters/parameter_validation.cljsearch-app/src/cmr/search/services/query_service.cljsearch-app/test/cmr/search/test/unit/services/parameter_converters/geometry_force_cartesian_test.cljsystem-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.cljsystem-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
search-app/src/cmr/search/services/parameters/converters/shapefile.clj
Outdated
Show resolved
Hide resolved
system-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.clj
Show resolved
Hide resolved
Comment on lines
+260
to
+295
| (testing (format "Search with force-cartesian parameter using %s shapefile" fmt) | ||
| ;; The parameter changes how shapefile coordinates are interpreted (geodetic vs cartesian) | ||
| (are3 [shapefile force-cartesian expected-items] | ||
| (let [params (if (some? force-cartesian) | ||
| [{:name "shapefile" | ||
| :content (io/file (io/resource (str "shapefiles/" shapefile "." extension))) | ||
| :mime-type mime-type} | ||
| {:name "force-cartesian" | ||
| :content (str force-cartesian)} | ||
| {:name "provider" | ||
| :content "PROV1"}] | ||
| [{:name "shapefile" | ||
| :content (io/file (io/resource (str "shapefiles/" shapefile "." extension))) | ||
| :mime-type mime-type} | ||
| {:name "provider" | ||
| :content "PROV1"}]) | ||
| found (search/find-refs-with-multi-part-form-post :granule params)] | ||
| (d/assert-refs-match expected-items found)) | ||
|
|
||
| "box shapefile without force-cartesian (default geodetic)" | ||
| "box" nil [whole-world very-wide-cart washington-dc richmond] | ||
|
|
||
| "box shapefile with force-cartesian=false (explicit geodetic)" | ||
| "box" false [whole-world very-wide-cart washington-dc richmond] | ||
|
|
||
| "box shapefile with force-cartesian=true (cartesian interpretation)" | ||
| "box" true [whole-world very-wide-cart washington-dc richmond] | ||
|
|
||
| "southern_africa without force-cartesian" | ||
| "southern_africa" nil [whole-world polygon-with-holes polygon-with-holes-cart normal-line normal-line-cart normal-brs wide-south-cart] | ||
|
|
||
| "southern_africa with force-cartesian=false" | ||
| "southern_africa" false [whole-world polygon-with-holes polygon-with-holes-cart normal-line normal-line-cart normal-brs wide-south-cart] | ||
|
|
||
| "southern_africa with force-cartesian=true" | ||
| "southern_africa" true [whole-world polygon-with-holes polygon-with-holes-cart normal-line normal-line-cart normal-brs wide-south-cart])) |
There was a problem hiding this comment.
These assertions don't prove force-cartesian changes anything.
Every nil/false/true case expects the same refs, so a regression where the flag is ignored would still pass this entire block. Please add at least one shapefile fixture whose geodetic and cartesian interpretations return different granules.
🤖 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/granule/granule_shapefile_search_test.clj`
around lines 260 - 295, The test block "Search with force-cartesian parameter
using %s shapefile" currently asserts identical expected refs for nil/false/true
so it won't detect regressions; update the test (around the are3 that calls
search/find-refs-with-multi-part-form-post and uses d/assert-refs-match) to
include at least one shapefile fixture whose geodetic vs cartesian
interpretations yield different granule refs: add a new shapefile in resources
(e.g., shapefiles/<name>.<extension>) that crosses the dateline or uses
projected coordinates, then add distinct expected-items arrays for the
force-cartesian=true case versus the false/nil cases so the three branches
assert different results, and ensure the new fixture is referenced in the are3
test parameters and expected refs are updated accordingly.
Contributor
Author
There was a problem hiding this comment.
Won't do. Addressed during PR walkthrough discussion.
There was a problem hiding this comment.
@zimzoom, understood — I'll defer to the walkthrough discussion. No further action needed from my side.
🧠 Learnings used
Learnt from: jmaeng72
Repo: nasa/Common-Metadata-Repository PR: 2401
File: search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj:129-129
Timestamp: 2026-03-13T14:49:00.112Z
Learning: In `search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj`, the abbreviation "tar" in test descriptions refers to "target" (as in target directory), not the tar archive format.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2400 +/- ##
==========================================
+ Coverage 57.88% 57.89% +0.01%
==========================================
Files 1067 1067
Lines 73433 73454 +21
Branches 2126 2124 -2
==========================================
+ Hits 42507 42528 +21
+ Misses 28947 28944 -3
- Partials 1979 1982 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
system-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.clj (1)
185-187:⚠️ Potential issue | 🟡 MinorDocstring syntax issue.
Line 186 uses a bare string literal as a docstring, but
deftestdoesn't support docstrings this way. Use a comment instead.Suggested fix
(deftest collection-shapefile-force-cartesian-validation-test - "Test that cartesian-only shapefiles require force-cartesian=true to pass validation" + ;; Test that cartesian-only shapefiles require force-cartesian=true to pass validation (let [_ (side/eval-form `(shapefile/set-enable-shapefile-parameter-flag! true))]🤖 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_shapefile_search_test.clj` around lines 185 - 187, The test defines a bare string literal after deftest which is being treated as a misplaced docstring; replace that string with a comment so the test name remains the first form and no invalid docstring is used. In the function collection-shapefile-force-cartesian-validation-test, remove or convert the quoted string "Test that cartesian-only shapefiles require force-cartesian=true to pass validation" into a preceding or inline comment (e.g., ;; ...) so the deftest form is valid and the test body starting with (let [_ (side/eval-form ...)] remains unchanged.
🤖 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/src/cmr/search/services/parameters/converters/shapefile.clj`:
- Around line 347-355: The code in parameter->condition assumes
force-cartesian-value is a string and calls util/safe-lowercase, but the
parameter may already be a boolean; update the logic in the :shapefile method to
handle both types by reading force-cartesian-value (from context via get-in),
then if it's a boolean use it directly, otherwise coerce strings by applying
util/safe-lowercase and comparing to "true", build options as (when
force-cartesian {:force-cartesian true}), and pass options into
shapefile->conditions; reference the parameter->condition multimethod, the local
force-cartesian-value, util/safe-lowercase, and shapefile->conditions when
applying this fix.
---
Duplicate comments:
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.clj`:
- Around line 185-187: The test defines a bare string literal after deftest
which is being treated as a misplaced docstring; replace that string with a
comment so the test name remains the first form and no invalid docstring is
used. In the function collection-shapefile-force-cartesian-validation-test,
remove or convert the quoted string "Test that cartesian-only shapefiles require
force-cartesian=true to pass validation" into a preceding or inline comment
(e.g., ;; ...) so the deftest form is valid and the test body starting with (let
[_ (side/eval-form ...)] remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64a8cd74-dac4-4dfb-b623-9e86cf680901
📒 Files selected for processing (7)
search-app/src/cmr/search/services/parameters/converters/geometry.cljsearch-app/src/cmr/search/services/parameters/converters/shapefile.cljsearch-app/src/cmr/search/services/parameters/parameter_validation.cljsystem-int-test/resources/shapefiles/scotland_cartesian.jsonsystem-int-test/src/cmr/system_int_test/utils/search_util.cljsystem-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.cljsystem-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- search-app/src/cmr/search/services/parameters/parameter_validation.clj
Comment on lines
347
to
355
| (defmethod p/parameter->condition :shapefile | ||
| [_context _concept-type _param value _options] | ||
| [context _concept-type _param value _options] | ||
| (if (enable-shapefile-parameter-flag) | ||
| (shapefile->conditions value) | ||
| (let [;; Params are added to context as keywords after sanitization | ||
| force-cartesian-value (get-in context [:params :force-cartesian]) | ||
| force-cartesian (= "true" (util/safe-lowercase force-cartesian-value)) | ||
| options (when force-cartesian {:force-cartesian true})] | ||
| (shapefile->conditions value options)) | ||
| (errors/throw-service-error :bad-request "Searching by shapefile is not enabled"))) |
There was a problem hiding this comment.
Potential type mismatch: force-cartesian may arrive as boolean, not string.
The parameter is registered as :boolean in conversion.clj, which may convert the value to a boolean before it reaches this point. The current code assumes a string value:
force-cartesian (= "true" (util/safe-lowercase force-cartesian-value))If force-cartesian-value is already boolean true, util/safe-lowercase may not behave as expected.
Suggested fix to handle both types
(defmethod p/parameter->condition :shapefile
[context _concept-type _param value _options]
(if (enable-shapefile-parameter-flag)
(let [;; Params are added to context as keywords after sanitization
force-cartesian-value (get-in context [:params :force-cartesian])
- force-cartesian (= "true" (util/safe-lowercase force-cartesian-value))
+ force-cartesian (or (true? force-cartesian-value)
+ (= "true" (util/safe-lowercase force-cartesian-value)))
options (when force-cartesian {:force-cartesian true})]
(shapefile->conditions value options))
(errors/throw-service-error :bad-request "Searching by shapefile is not enabled")))#!/bin/bash
# Verify how force-cartesian parameter flows through conversion
# Check if boolean conversion happens before parameter->condition is called
echo "=== Checking parameter conversion for :force-cartesian ==="
rg -n "force-cartesian" --type clojure -C 3
echo ""
echo "=== Checking how :boolean parameters are converted ==="
ast-grep --pattern $'defmethod.*parameter->condition.*:boolean'
echo ""
echo "=== Checking util/safe-lowercase implementation ==="
rg -n "defn safe-lowercase" --type clojure -A 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@search-app/src/cmr/search/services/parameters/converters/shapefile.clj`
around lines 347 - 355, The code in parameter->condition assumes
force-cartesian-value is a string and calls util/safe-lowercase, but the
parameter may already be a boolean; update the logic in the :shapefile method to
handle both types by reading force-cartesian-value (from context via get-in),
then if it's a boolean use it directly, otherwise coerce strings by applying
util/safe-lowercase and comparing to "true", build options as (when
force-cartesian {:force-cartesian true}), and pass options into
shapefile->conditions; reference the parameter->condition multimethod, the local
force-cartesian-value, util/safe-lowercase, and shapefile->conditions when
applying this fix.
Contributor
Author
There was a problem hiding this comment.
Will not do. We know this is a string here, and this is how we process other boolean parameters we have.
There was a problem hiding this comment.
@zimzoom, understood — acknowledged and noted!
✏️ Learnings added
Learnt from: zimzoom
Repo: nasa/Common-Metadata-Repository PR: 2400
File: search-app/src/cmr/search/services/parameters/converters/shapefile.clj:347-355
Timestamp: 2026-03-18T13:00:05.286Z
Learning: In `nasa/Common-Metadata-Repository`, boolean query parameters (e.g. `:force-cartesian`, `:simplify-shapefile`) are always received as strings by the time they reach `p/parameter->condition` in `search-app/src/cmr/search/services/parameters/converters/shapefile.clj`. The correct and consistent idiom for coercing them to booleans is `(= "true" (util/safe-lowercase force-cartesian-value))`. Do not suggest handling them as native booleans at this call site.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jmaeng72
Repo: nasa/Common-Metadata-Repository PR: 2401
File: search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj:129-129
Timestamp: 2026-03-13T14:49:03.766Z
Learning: In `search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj`, the abbreviation "tar" in test descriptions refers to "target" (as in target directory), not the tar archive format.
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:02.702Z
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 do not make logical sense. In code reviews, ensure parameter handlers or validation logic do not treat collection-progress as multi-valued and avoid composing multiple progress values with AND. Treat collection-progress as a single-valued enum and validate inputs accordingly, documenting this constraint where relevant.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
system-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj (1)
260-296:⚠️ Potential issue | 🟠 MajorTest assertions don't verify that
force-cartesianactually changes behavior.All three test cases (nil, false, true) assert identical expected results for both "box" and "southern_africa" shapefiles. If the
force-cartesianflag were ignored entirely, these tests would still pass.The collection test file includes a validation test with
scotland_cartesian.jsonthat fails without force-cartesian=true, but this granule test lacks similar coverage to prove the parameter affects spatial matching.Consider adding a test case with a shapefile fixture whose geodetic vs cartesian interpretation yields different granule matches.
🤖 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/granule/granule_shapefile_search_test.clj` around lines 260 - 296, The tests in the granule_shapefile_search_test (the are3 block that builds params and calls search/find-refs-with-multi-part-form-post) use identical expected-items for force-cartesian nil/false/true, so they do not verify that the force-cartesian parameter changes spatial matching; add a new case (or replace one) that uses a shapefile fixture with known differing geodetic vs cartesian results (similar to scotland_cartesian.json used in the collection tests) and assert different expected-items for force-cartesian=true versus false/nil to prove the flag affects search results.
🤖 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/src/cmr/search/services/parameters/parameter_validation.clj`:
- Line 41: valid-parameter-options for :collection is missing the
:force-cartesian option which causes collection searches to reject that valid
flag; update the valid-parameter-options multimethod's :collection branch to
include :force-cartesian (mirroring where :force-cartesian is already present
for :granule and in the :single-value sets) so collection option validation
accepts it — locate the valid-parameter-options :collection implementation and
add :force-cartesian to the set of allowed options alongside the other
boolean/flag options.
---
Duplicate comments:
In
`@system-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj`:
- Around line 260-296: The tests in the granule_shapefile_search_test (the are3
block that builds params and calls search/find-refs-with-multi-part-form-post)
use identical expected-items for force-cartesian nil/false/true, so they do not
verify that the force-cartesian parameter changes spatial matching; add a new
case (or replace one) that uses a shapefile fixture with known differing
geodetic vs cartesian results (similar to scotland_cartesian.json used in the
collection tests) and assert different expected-items for force-cartesian=true
versus false/nil to prove the flag affects search results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2213d81f-d0f7-48ab-a086-b419b0d26fe0
📒 Files selected for processing (8)
search-app/src/cmr/search/services/parameters/converters/geometry.cljsearch-app/src/cmr/search/services/parameters/converters/shapefile.cljsearch-app/src/cmr/search/services/parameters/parameter_validation.cljsearch-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.cljsystem-int-test/resources/shapefiles/scotland_cartesian.jsonsystem-int-test/src/cmr/system_int_test/utils/search_util.cljsystem-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.cljsystem-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
Contributor
Author
|
PR feedback changes added:
|
Contributor
|
Please address the new coderabbit comments |
Contributor
Author
|
All new CodeRabbit comments addressed. |
jceaser
approved these changes
Mar 19, 2026
eereiter
approved these changes
Mar 19, 2026
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/src/cmr/search/services/parameters/converters/shapefile.clj`:
- Around line 315-319: The debug statement inside the anonymous converter
function (the fn handling shapefile-info in shapefile.clj) logs the misspelled
key :contenty-type, which yields nil; change the debug to reference the correct
key :content-type (i.e., update the debug (format "SHAPEFILE FORMAT: %s"
(:contenty-type shapefile-info)) to use (:content-type shapefile-info)) so logs
show the actual shapefile content type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f106724-4efc-40d3-8c32-5ccbf48c2de6
📒 Files selected for processing (7)
search-app/src/cmr/search/services/parameters/converters/geometry.cljsearch-app/src/cmr/search/services/parameters/converters/shapefile.cljsearch-app/src/cmr/search/services/parameters/parameter_validation.cljsearch-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.cljsystem-int-test/resources/shapefiles/scotland_cartesian.jsonsystem-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.cljsystem-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
🚧 Files skipped from review as they are similar to previous changes (5)
- search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj
- system-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
- search-app/src/cmr/search/services/parameters/parameter_validation.clj
- system-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.clj
- search-app/src/cmr/search/services/parameters/converters/geometry.clj
Comment on lines
+315
to
319
| "Converts a shapefile to query conditions based on shapefile format. | ||
| Optionally accepts an options map containing force-cartesian setting." | ||
| (fn [shapefile-info & _] | ||
| (debug (format "SHAPEFILE FORMAT: %s" (:contenty-type shapefile-info))) | ||
| (if (:in-memory shapefile-info) |
There was a problem hiding this comment.
Fix debug key typo to avoid misleading logs.
Line 318 logs :contenty-type, which is misspelled and will emit nil for normal payloads.
Suggested patch
(defmulti shapefile->conditions
"Converts a shapefile to query conditions based on shapefile format.
Optionally accepts an options map containing force-cartesian setting."
(fn [shapefile-info & _]
- (debug (format "SHAPEFILE FORMAT: %s" (:contenty-type shapefile-info)))
+ (debug (format "SHAPEFILE FORMAT: %s" (:content-type shapefile-info)))
(if (:in-memory shapefile-info)
:in-memory
(:content-type shapefile-info))))📝 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
| "Converts a shapefile to query conditions based on shapefile format. | |
| Optionally accepts an options map containing force-cartesian setting." | |
| (fn [shapefile-info & _] | |
| (debug (format "SHAPEFILE FORMAT: %s" (:contenty-type shapefile-info))) | |
| (if (:in-memory shapefile-info) | |
| "Converts a shapefile to query conditions based on shapefile format. | |
| Optionally accepts an options map containing force-cartesian setting." | |
| (fn [shapefile-info & _] | |
| (debug (format "SHAPEFILE FORMAT: %s" (:content-type shapefile-info))) | |
| (if (:in-memory shapefile-info) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@search-app/src/cmr/search/services/parameters/converters/shapefile.clj`
around lines 315 - 319, The debug statement inside the anonymous converter
function (the fn handling shapefile-info in shapefile.clj) logs the misspelled
key :contenty-type, which yields nil; change the debug to reference the correct
key :content-type (i.e., update the debug (format "SHAPEFILE FORMAT: %s"
(:contenty-type shapefile-info)) to use (:content-type shapefile-info)) so logs
show the actual shapefile content type.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
search-app/src/cmr/search/services/parameters/converters/shapefile.clj (1)
321-328:⚠️ Potential issue | 🟡 MinorFix debug key typo to avoid misleading logs.
Line 325 logs
:contenty-type, which is misspelled and will emitnilfor normal payloads.Suggested fix
(defmulti shapefile->conditions "Converts a shapefile to query conditions based on shapefile format. Optionally accepts an options map containing force-cartesian setting." (fn [shapefile-info & _] - (debug (format "SHAPEFILE FORMAT: %s" (:contenty-type shapefile-info))) + (debug (format "SHAPEFILE FORMAT: %s" (:content-type shapefile-info))) (if (:in-memory shapefile-info) :in-memory (:content-type shapefile-info))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@search-app/src/cmr/search/services/parameters/converters/shapefile.clj` around lines 321 - 328, In the shapefile->conditions multimethod, the debug log uses the misspelled key :contenty-type which results in nil logs; update the debug call inside the (fn [shapefile-info & _] ...) to reference the correct key :content-type on shapefile-info so the log prints the actual content type (symbol references: shapefile->conditions and shapefile-info).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@search-app/src/cmr/search/services/parameters/converters/shapefile.clj`:
- Around line 321-328: In the shapefile->conditions multimethod, the debug log
uses the misspelled key :contenty-type which results in nil logs; update the
debug call inside the (fn [shapefile-info & _] ...) to reference the correct key
:content-type on shapefile-info so the log prints the actual content type
(symbol references: shapefile->conditions and shapefile-info).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76de20a9-77de-4e69-a528-9a51100e93a2
📒 Files selected for processing (10)
search-app/src/cmr/search/services/parameters/conversion.cljsearch-app/src/cmr/search/services/parameters/converters/geometry.cljsearch-app/src/cmr/search/services/parameters/converters/shapefile.cljsearch-app/src/cmr/search/services/parameters/parameter_validation.cljsearch-app/src/cmr/search/services/query_service.cljsearch-app/test/cmr/search/test/unit/services/parameter_converters/geometry_force_cartesian_test.cljsearch-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.cljsystem-int-test/resources/shapefiles/scotland_cartesian.jsonsystem-int-test/test/cmr/system_int_test/search/collection/collection_shapefile_search_test.cljsystem-int-test/test/cmr/system_int_test/search/granule/granule_shapefile_search_test.clj
🚧 Files skipped from review as they are similar to previous changes (5)
- search-app/test/cmr/search/test/unit/services/parameter_converters/shapefile.clj
- search-app/src/cmr/search/services/parameters/converters/geometry.clj
- search-app/src/cmr/search/services/parameters/parameter_validation.clj
- search-app/test/cmr/search/test/unit/services/parameter_converters/geometry_force_cartesian_test.clj
- search-app/src/cmr/search/services/query_service.clj
zimzoom
dismissed
jmaeng72’s stale review
Dismissing after some time because changes requested were addressed, other reviewers approved afterward, dev is out of office and we need to advance this feature
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
5 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?
Create an optional search parameter to force cartesian instead of the default geodetic for input shapefiles. New parameter "force_cartesian" should be optional and default to false, preserving behavior.
What are the changes?
Added force_cartesian parameter to search-app validation and query construction, replacing instances of hard-coded
:geodeticin shape construction with a check for the new parameter. This is all that was needed because the required cartesian types and methods already existed.What areas of the application does this impact?
search-app
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Tests