Skip to content

CMR-11098: Add force_cartesian optional param to search #2400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zimzoom merged 3 commits into from
Mar 25, 2026
Merged

CMR-11098: Add force_cartesian optional param to search #2400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14e39e3
CMR-11098: Add force_cartesian optional param to search
zimzoom Mar 4, 2026
127d7af
CMR-11098: pr feedback
zimzoom Mar 12, 2026
33a78c2
CMR-11098: pr feedback
zimzoom Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions search-app/src/cmr/search/services/parameters/conversion.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
:facets-size :string
:shapefile :shapefile
:simplify-shapefile :boolean
:force-cartesian :boolean

;; Tag parameters
:tag-data :tag-query
Expand Down Expand Up @@ -152,7 +153,8 @@
:cycle :int
:passes :passes
:shapefile :shapefile
:simplify-shapefile :boolean})
:simplify-shapefile :boolean
:force-cartesian :boolean})

(defmethod common-params/param-mappings :tag
[_]
Expand Down Expand Up @@ -570,7 +572,7 @@
[(dissoc params
:boosts :include-granule-counts :include-has-granules :include-facets :echo-compatible
:hierarchical-facets :include-highlights :include-tags :all-revisions :facets-size
:simplify-shapefile)
:simplify-shapefile :force-cartesian)
(-> query-attribs
(merge {:boosts (:boosts params)
:result-features (seq result-features)
Expand All @@ -594,7 +596,7 @@
:granule params lp/param-aliases)
result-features (when (= "v2" (util/safe-lowercase (:include-facets params)))
[:facets-v2])
regular-params (dissoc params :echo-compatible :include-facets :simplify-shapefile)
regular-params (dissoc params :echo-compatible :include-facets :simplify-shapefile :force-cartesian)
{:keys [page-size offset]} query-attribs
concept-id (:concept-id regular-params)
concept-ids (when concept-id
Expand Down
39 changes: 24 additions & 15 deletions search-app/src/cmr/search/services/parameters/converters/geometry.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,27 @@

(defn line-string-ring->ring
"Convert a JTS LineString to a spatial lib ring"
[^LineString line-string]
(rr/ords->ring :geodetic (coords->ords (.getCoordinates line-string))))
([^LineString line-string]
(line-string-ring->ring line-string {}))
([^LineString line-string options]
(let [coord-sys (if (:force-cartesian options) :cartesian :geodetic)]
(rr/ords->ring coord-sys (coords->ords (.getCoordinates line-string))))))

(defn force-ccw-orientation
"Forces a LineString to be in counter-clockwise orientation"
[^LineString line-string winding]
(let [coords (.getCoordinates line-string)]
(if (and
(> (count coords) 3)
(= winding :cw))
(if (and
(> (count coords) 3)
(= winding :cw))
(.reverse line-string)
line-string)))

(defn polygon->shape
"Convert a JTS Polygon to a spatial lib shape that can be used in a Spatial query.
The `options` map can be used to provide information about winding. Accepted keys
are `:boundary-winding` and `:hole-winding`. Accepted values are `:cw` and `:ccw`."
are `:boundary-winding` and `:hole-winding`. Accepted values are `:cw` and `:ccw`.
The `:force-cartesian` key can be used to force cartesian coordinate system."
[^Polygon polygon options]
(let [boundary-ring (.getExteriorRing polygon)
_ (debug (format "BOUNDARY RING BEFORE FORCE-CCW: %s" boundary-ring))
Expand All @@ -61,10 +65,12 @@
(for [i (range num-interior-rings)]
(force-ccw-orientation (.getInteriorRingN polygon i) (:hole-winding options)))
[])
all-rings (concat [boundary-ring] interior-rings)]
all-rings (concat [boundary-ring] interior-rings)
coord-sys (if (:force-cartesian options) :cartesian :geodetic)]
(debug (format "NUM INTERIOR RINGS: [%d]" num-interior-rings))
(debug (format "RINGS: [%s]" (vec all-rings)))
(poly/polygon :geodetic (map line-string-ring->ring all-rings))))
(debug (format "COORDINATE SYSTEM: %s" coord-sys))
(poly/polygon coord-sys (map #(line-string-ring->ring % options) all-rings))))

(defn point->shape
"Convert a JTS Point to a spatial lib shape that can be used in a Spatial query"
Expand All @@ -73,16 +79,19 @@

(defn line->shape
"Convert a LineString or LinearRing to a spatial lib shape that can be used in a Spatial query"
[^LineString line]
(let [ordinates (coords->ords (.getCoordinates line))]
(l/ords->line-string :geodetic ordinates)))
([^LineString line]
(line->shape line {}))
([^LineString line options]
(let [ordinates (coords->ords (.getCoordinates line))
coord-sys (if (:force-cartesian options) :cartesian :geodetic)]
(l/ords->line-string coord-sys ordinates))))

(defmulti geometry->condition
"Convert a Geometry object to a query condition.
The `options` map can be used to provided additional information."
(fn [^Geometry geometry options]
(fn [^Geometry geometry options]
(.getGeometryType geometry)))

(defmethod geometry->condition "MultiPolygon"
[geometry options]
(let [shape (polygon->shape geometry options)]
Expand All @@ -100,10 +109,10 @@

(defmethod geometry->condition "LineString"
[geometry options]
(let [shape (line->shape geometry)]
(let [shape (line->shape geometry options)]
(qm/->SpatialCondition shape)))

(defmethod geometry->condition "LinearRing"
[geometry options]
(let [shape (line->shape geometry)]
(let [shape (line->shape geometry options)]
(qm/->SpatialCondition shape)))
100 changes: 59 additions & 41 deletions search-app/src/cmr/search/services/parameters/converters/shapefile.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,22 @@
{:default 5000 :type Long})

(defn winding-opts
"Get the opts for a call to `normalize-polygon-winding` based on file type"
[mime-type]
(case mime-type
"application/shapefile+zip" {:boundary-winding :cw}
"application/vnd.google-earth.kml+xml" {}
"application/geo+json" {:hole-winding :cw}))
"Get the opts for a call to `normalize-polygon-winding` based on file type.
Optionally merges in force-cartesian option if provided in options.

Note: For GeoJSON, we only normalize hole winding. Boundary winding is not normalized
because GeoJSON coordinates come from GeoTools already validated. For small local polygons
(the typical cartesian use case), geodetic CCW and cartesian CCW have the same orientation,
so no boundary reversal is needed."
[mime-type options]
(let [base-opts (case mime-type
"application/shapefile+zip" {:boundary-winding :cw}
"application/vnd.google-earth.kml+xml" {}
"application/geo+json" {:hole-winding :cw})
final-opts (if (:force-cartesian options)
(assoc base-opts :force-cartesian true)
base-opts)]
final-opts))

(defn validate-entry-dir
[target-dir entry]
Expand Down Expand Up @@ -148,8 +158,8 @@
(cond
(= point-count 0) "Shapefile has no points"
(> point-count (max-shapefile-points))
(format "Number of points in shapefile exceeds the limit of %d"
(max-shapefile-points))
(format "Number of points in shapefile exceeds the limit of %d"
(max-shapefile-points))
:else nil)))

(defn- validate-feature-count
Expand All @@ -159,9 +169,9 @@
(cond
(= feature-count 0) "Shapefile has no features"
(> feature-count (max-shapefile-features))
(format "Shapefile feature count [%d] exceeds the %d feature limit"
feature-count
(max-shapefile-features))
(format "Shapefile feature count [%d] exceeds the %d feature limit"
feature-count
(max-shapefile-features))
:else nil)))

(defn validate-features
Expand All @@ -172,8 +182,9 @@

(defn feature->conditions
"Process the contents of a Feature to return query conditions along with number of points in
the processed Feature. The `context` map can be used to pass along additional info."
[feature context]
the processed Feature. The `options` map can be used to pass along additional info including
winding options and force-cartesian setting."
[feature options]
(let [crs (when (.getDefaultGeometryProperty feature)
(-> feature .getDefaultGeometryProperty .getDescriptor .getCoordinateReferenceSystem))
properties (.getProperties feature)
Expand All @@ -183,20 +194,22 @@
geometries (map #(-> % .getValue (transform-to-epsg-4326 crs)) geometry-props)
_ (debug (format "Transformed [%d] geometries" (count geometries)))
point-count (apply + (map geometry-point-count geometries))
conditions (mapcat (fn [g] (geometry->conditions g context)) geometries)]
conditions (mapcat (fn [g] (geometry->conditions g options)) geometries)]
(debug (format "CONDITIONS: %s" conditions))
[conditions point-count]))

(defn features->conditions
"Converts a list of features into a vector of SpatialConditions"
[features mime-type]
"Converts a list of features into a vector of SpatialConditions.
The options map can contain force-cartesian setting which will be merged into the winding options."
[features mime-type options]
(validate-features features)
(let [iterator (.iterator features)]
;; Loop overall all the Features in the list, building up a vector of conditions
(let [iterator (.iterator features)
winding-options (winding-opts mime-type options)]
;; Loop overall all the Features in the list, building up a vector of conditions
(loop [conditions []]
(if (.hasNext iterator)
(let [feature (.next iterator)
[feature-conditions _] (feature->conditions feature (winding-opts mime-type))]
[feature-conditions _] (feature->conditions feature winding-options)]
(if (> (count feature-conditions) 0)
;; if any conditions were created for the feature add them to the current conditions
(recur (conj conditions (gc/or-conds feature-conditions)))
Expand All @@ -216,7 +229,7 @@

(defn esri-shapefile->condition-vec
"Converts a shapefile to a vector of SpatialConditions"
[shapefile-info]
[shapefile-info options]
(try
(let [file (:tempfile shapefile-info)
^File temp-dir (unzip-file file)
Expand All @@ -234,7 +247,7 @@
(while (.hasNext iterator)
(let [feature (.next iterator)]
(.add feature-list feature)))
(features->conditions feature-list mt/shapefile)
(features->conditions feature-list mt/shapefile options)
(finally
(.close iterator)
(-> data-store .getFeatureReader .close)
Expand All @@ -247,7 +260,7 @@

(defn geojson->conditions-vec
"Converts a geojson file to a vector of SpatialConditions"
[shapefile-info]
[shapefile-info options]
(try
(let [file (:tempfile shapefile-info)
_ (geojson/sanitize-geojson file)
Expand All @@ -266,7 +279,7 @@
(while (.hasNext iterator)
(let [feature (.next iterator)]
(.add feature-list feature)))
(features->conditions feature-list mt/geojson)
(features->conditions feature-list mt/geojson options)
(finally
(.close iterator)
(-> data-store .getFeatureReader .close)
Expand All @@ -279,17 +292,17 @@

(defn kml->conditions-vec
"Converts a kml file to a vector of SpatialConditions"
[shapefile-info]
[shapefile-info options]
(try
(let [file (:tempfile shapefile-info)
input-stream (FileInputStream. file)
parser (PullParser. (KMLConfiguration.) input-stream SimpleFeature)
feature-list (ArrayList.)]
(try
(util/while-let [feature (.parse parser)]
(when (> (feature-point-count feature) 0)
(.add feature-list feature)))
(features->conditions feature-list mt/kml)
(when (> (feature-point-count feature) 0)
(.add feature-list feature)))
(features->conditions feature-list mt/kml options)
(finally
(.delete file))))
(catch Exception e
Expand All @@ -300,45 +313,50 @@

(defn in-memory->conditions-vec
"Converts a group of features produced by simplification to a vector of SpatialConditions"
[shapefile-info]
[shapefile-info options]
(let [^ArrayList features (:tempfile shapefile-info)
mime-type (:content-type shapefile-info)]
(features->conditions features mime-type)))
(features->conditions features mime-type options)))

(defmulti shapefile->conditions
"Converts a shapefile to query conditions based on shapefile format"
(fn [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" (:contenty-type shapefile-info)))
(if (:in-memory shapefile-info)
Comment on lines +322 to 326
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

:in-memory
(:content-type shapefile-info))))

;; ESRI shapefiles
(defmethod shapefile->conditions mt/shapefile
[shapefile-info]
(let [conditions-vec (esri-shapefile->condition-vec shapefile-info)]
[shapefile-info options]
(let [conditions-vec (esri-shapefile->condition-vec shapefile-info options)]
(gc/or-conds (flatten conditions-vec))))

;; GeoJSON
(defmethod shapefile->conditions mt/geojson
[shapefile-info]
(let [conditions-vec (geojson->conditions-vec shapefile-info)]
[shapefile-info options]
(let [conditions-vec (geojson->conditions-vec shapefile-info options)]
(gc/or-conds (flatten conditions-vec))))

;; KML
(defmethod shapefile->conditions mt/kml
[shapefile-info]
(let [conditions-vec (kml->conditions-vec shapefile-info)]
[shapefile-info options]
(let [conditions-vec (kml->conditions-vec shapefile-info options)]
(gc/or-conds (flatten conditions-vec))))

;; Simplfied and stored in memory
(defmethod shapefile->conditions :in-memory
[shapefile-info]
(let [conditions-vec (in-memory->conditions-vec shapefile-info)]
[shapefile-info options]
(let [conditions-vec (in-memory->conditions-vec shapefile-info options)]
(gc/or-conds (flatten conditions-vec))))

(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")))
Comment on lines 354 to 362
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Copy Markdown
Contributor Author

@zimzoom zimzoom Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will not do. We know this is a string here, and this is how we process other boolean parameters we have.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Loading
Loading