CMR-11048: Update UMM-S to support a new output format that will allow me to request OPeNDAP URL.#2382
Conversation
📝 WalkthroughWalkthroughAdds UMM‑S v1.5.4: new JSON schemas and example data, migration logic for 1.5.3↔1.5.4 (including removal of a non‑valid format), tests for schema and migrations, and updates ingest default Changes
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
🤖 Fix all issues with AI agents
In
`@umm-spec-lib/resources/json-schemas/service/umm/v1.5.4/umm-s-search-results-json-schema.json`:
- Around line 103-164: Fix the misspelling "assciated" → "associated" in the
JSON Schema descriptions for the following properties: "variables",
"collections", "tools", "data-quality-summaries", "order-options",
"service-options", "service-entries", and "grids" inside the schema (replace the
description strings that currently read "The concept ids of ... assciated with
the service" with "The concept ids of ... associated with the service"); update
each description entry where the typo appears (search for the property names
above to locate the exact description fields such as the description for
"variables", "collections", etc.) and save the schema file.
In `@umm-spec-lib/src/cmr/umm_spec/migration/version/service.clj`:
- Around line 642-658: The migration method interface/migrate-umm-version for
:service "1.5.4" -> "1.5.3" can leave an outer {:ServiceOptions nil} key because
util/remove-nil-keys is only applied inside the :ServiceOptions map; fix by
removing the dangling nil on the outer record: either (a) after the (update
:ServiceOptions #(when (seq %) %)) step call util/remove-nil-keys on the whole
umm-s before calling m-spec/update-version, or (b) change the :ServiceOptions
update to dissoc the key when empty (i.e. replace the (when (seq %) %) approach
with logic that dissocs :ServiceOptions when it would be nil) so the outer map
never contains :ServiceOptions nil.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj`:
- Around line 512-527: The test shows the downgrade in
migrate-1_5_4-down-to-1_5_3 expects "OPeNDAP" to be preserved but current
migration maps it to "SWODLR"; update the migration logic in vm/migrate-umm for
service downgrades from "1.5.4" to "1.5.3" so it no longer remaps Type "OPeNDAP"
→ "SWODLR" and instead leaves the :Type value unchanged; also ensure only the
ServiceOptions values are adjusted (remove "NETCDF-4 (OPeNDAP URL)" entries from
:SupportedInputFormats/:SupportedOutputFormats) so the Type remains non-lossy
and reversible.
🧹 Nitpick comments (5)
umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4.json (1)
58-63: Consider adding"NETCDF-4 (OPeNDAP URL)"to this example'sSupportedReformattings.This v1.5.4 example file exercises the new
"OPeNDAP"service type but doesn't use the new"NETCDF-4 (OPeNDAP URL)"format enum in itsSupportedReformattings. Including it here would make this canonical example more representative of the v1.5.4 additions and would ensure the example data file itself exercises both new features against the schema.umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj (2)
351-354: Behavior onnilinput differs from analogous function.
remove-non-valid-formats-1_5_4-to-1_5_3returns[]whensupported-formatsisnil, whereas the similarremove-non-valid-formats-1_3_3-to-1_3_2(line 210) returnsnilviaseq. The caller inservice.cljcompensates with explicit(empty? ...)/dissoclogic, so this is functionally correct—but returning(seq (vec ...))would be more consistent with the existing convention and simplify the caller.Not blocking since the caller handles it correctly.
356-363: Missing docstring on private helper.The analogous function
remove-reformattings-when-input-not-valid(line 220) also lacks a docstring, but adding one here would improve maintainability for future migration authors.umm-spec-lib/test/cmr/umm_spec/test/migration/service_service_options.clj (1)
255-262: Consider adding anilinput edge case.The function under test returns
[]fornilinput (unlike the analogousremove-non-valid-formats-1_3_3-to-1_3_2which returnsnil). Adding a test case like(is (= [] (service/remove-non-valid-formats-1_5_4-to-1_5_3 nil)))would document this behavior explicitly and guard against regressions if the implementation is later aligned with the older convention.umm-spec-lib/test/cmr/umm_spec/test/service_schema_test.clj (1)
9-38: Consider building JSON from a Clojure map instead of string concatenation.The inline JSON string is hard to read and maintain. Using
cheshire.core/generate-stringon a Clojure map would be more idiomatic and less error-prone while still testing the same JSON validation path.♻️ Suggested refactor
+(require '[cheshire.core :as json]) + (deftest service-supported-format-type-enum-test (testing "Validation of UMM-S with NETCDF-4 (OPeNDAP URL) format (CMR-11048)" - (let [valid-service-json (str "{" - " \"Name\": \"TestValidService\"," - ... - "}") + (let [valid-service-json (json/generate-string + {:Name "TestValidService" + :LongName "Long Name Test Valid Service" + :Type "OPeNDAP" + :Version "1.0" + :Description "Description of service" + :URL {:URLValue "https://example.com" + :Description "URL Description"} + :ServiceKeywords [{:ServiceCategory "DATA ANALYSIS AND VISUALIZATION" + :ServiceTopic "VISUALIZATION/IMAGE PROCESSING"}] + :ServiceOrganizations [{:Roles ["SERVICE PROVIDER"] + :ShortName "ESA/ED"}] + :MetadataSpecification {:URL "https://cdn.earthdata.nasa.gov/umm/service/v1.5.4" + :Name "UMM-S" + :Version "1.5.4"} + :ServiceOptions {:SupportedReformattings [{:SupportedInputFormat "NETCDF-4 (OPeNDAP URL)" + :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}]}}) validation-errors (json-schema/validate-umm-json valid-service-json :service "1.5.4")]
umm-spec-lib/resources/json-schemas/service/umm/v1.5.4/umm-s-search-results-json-schema.json
Outdated
Show resolved
Hide resolved
umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2382 +/- ##
===========================================
+ Coverage 58.04% 76.07% +18.03%
===========================================
Files 1065 44 -1021
Lines 72992 6182 -66810
Branches 2118 289 -1829
===========================================
- Hits 42365 4703 -37662
+ Misses 28659 1254 -27405
+ Partials 1968 225 -1743 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
umm-spec-lib/resources/example-data/umm-json/service/v1.5.3/Service_v1.5.3-to-v1.5.4.json
Outdated
Show resolved
Hide resolved
umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4-to-v1.5.3.json
Outdated
Show resolved
Hide resolved
| :granule ["1.4" "1.5" "1.6" "1.6.1" "1.6.2" "1.6.3" "1.6.4" "1.6.5" "1.6.6"] | ||
| :variable ["1.0" "1.1" "1.2" "1.3" "1.4" "1.5" "1.6" "1.7" "1.8" "1.8.1" "1.8.2" "1.9.0"] | ||
| :service ["1.0" "1.1" "1.2" "1.3" "1.3.1" "1.3.2" "1.3.3" "1.3.4" "1.4" "1.4.1" "1.5.0" "1.5.1" "1.5.2" "1.5.3"] | ||
| :service ["1.0" "1.1" "1.2" "1.3" "1.3.1" "1.3.2" "1.3.3" "1.3.4" "1.4" "1.4.1" "1.5.0" "1.5.1" "1.5.2" "1.5.3" "1.5.4"] |
There was a problem hiding this comment.
this list is over 100 characters, you could break it up my minor versions, that is all the 1.3.x on a line, followed by 1.4 and then 1.5, all on their own line. Collections should be doing that
umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4.json
Outdated
Show resolved
Hide resolved
|
|
||
| (deftest service-supported-format-type-enum-test | ||
| (testing "Validation of UMM-S with NETCDF-4 (OPeNDAP URL) format (CMR-11048)" | ||
| (let [valid-service-json (str "{" |
There was a problem hiding this comment.
could you not write this in EDN and then convert it to JSON or save it as a file and load it?
There was a problem hiding this comment.
I was actually doing that at first and went down this path while fixing a failing test. I'll update accordingly.
There was a problem hiding this comment.
you can use one of the example files that are already being used for test such as umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4.json
There was a problem hiding this comment.
it looks like you are alread doing this below on line 42.
jceaser
left a comment
jceaser
left a comment
There was a problem hiding this comment.
I have some suggestions but I think you should do what the AI said on at least it's first comment.
| {"SupportedInputFormat": "HDF5", "SupportedOutputFormats": ["GEOTIFFFLOAT32", "GEOTIFFINT16", "COG"]}, | ||
| {"SupportedInputFormat": "WKT", "SupportedOutputFormats": ["GEOTIFFFLOAT32", "GEOTIFFINT16"]}, | ||
| {"SupportedInputFormat": "HDF4", "SupportedOutputFormats": ["HDF4", "NETCDF-3", "NETCDF-4", "BINARY", "ASCII", "ZARR", "GeoJSON"]}, | ||
| {"SupportedInputFormat": "Shapefile", "SupportedOutputFormats": ["HDF4", "NETCDF-3", "NETCDF-4", "BINARY", "ASCII", "ZARR", "GeoJSON"]} |
There was a problem hiding this comment.
I think this example is supposed to test going from 1.5.4 to 1.5.3, but none of the Supported Formats use the new Format name of "NETCDF-4 (OPeNDAP URL)". This file will only test existing values, but not the new 1.5.4 values.
There was a problem hiding this comment.
Good catch, I'll fix this.
| (defmethod interface/migrate-umm-version [:service "1.5.4" "1.5.3"] | ||
| [_context umm-s & _] | ||
| (-> umm-s | ||
| (update :Type #(if (= % "OPeNDAP") "SWODLR" %)) |
There was a problem hiding this comment.
The Type should not be updated going from 1.5.4 to 1.5.3. None of the types changed between these versions.
There was a problem hiding this comment.
Got it and will update accordingly. Thanks!
| (update :ServiceOptions | ||
| #(-> % | ||
| (update :SupportedReformattings service-options/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3) | ||
| (update :SupportedInputFormats service-options/remove-non-valid-formats-1_5_4-to-1_5_3) |
There was a problem hiding this comment.
If you look at the schema you will notice that SupportedInputFormats does not exist under service options, therefore this line is not needed, please delete.
| #(-> % | ||
| (update :SupportedReformattings service-options/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3) | ||
| (update :SupportedInputFormats service-options/remove-non-valid-formats-1_5_4-to-1_5_3) | ||
| (update :SupportedOutputFormats service-options/remove-non-valid-formats-1_5_4-to-1_5_3) |
There was a problem hiding this comment.
If you look at the schema you will notice that SupportedOutputFormats does not exist under the service options, therefore this line is not needed, please delete.
| (update :SupportedOutputFormats service-options/remove-non-valid-formats-1_5_4-to-1_5_3) | ||
| (as-> opts | ||
| (cond-> opts | ||
| (empty? (:SupportedInputFormats opts)) (dissoc :SupportedInputFormats) |
There was a problem hiding this comment.
Please delete this line as SuppportedInputFormats at the service option element does not exist
| (as-> opts | ||
| (cond-> opts | ||
| (empty? (:SupportedInputFormats opts)) (dissoc :SupportedInputFormats) | ||
| (empty? (:SupportedOutputFormats opts)) (dissoc :SupportedOutputFormats) |
There was a problem hiding this comment.
Please delete this line as SuppportedOutputFormats at the service option element does not exist
| (defn remove-non-valid-formats-1_5_4-to-1_5_3 | ||
| "Remove the non valid Supported Format enumerations when migrating from 1.5.4 to 1.5.3." | ||
| [supported-formats] | ||
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" %) supported-formats))) |
There was a problem hiding this comment.
Depending on how the stewardship team responds to my question, this function may change.
There was a problem hiding this comment.
instead of removing "NETCDF-4 (OPeNDAP URL)" just replace it with "NETCDF-4".
| :Name "UMM-S" | ||
| :Version "1.5.3"} | ||
| :ServiceOptions {:SupportedInputFormats ["HDF4"] | ||
| :SupportedOutputFormats ["HDF4"]}} |
There was a problem hiding this comment.
This test is not valid as SupportedInputFormats and SupportedOutputFormats do not exist under ServiceOptions. Only SupportedReformattings exists under ServiceOptions that have changed.
| :MetadataSpecification {:URL "https://cdn.earthdata.nasa.gov/umm/service/v1.5.4" | ||
| :Name "UMM-S" | ||
| :Version "1.5.4"} | ||
| :ServiceOptions {:SupportedInputFormats ["HDF4" "NETCDF-4 (OPeNDAP URL)"] |
There was a problem hiding this comment.
this test is not valid as SupportedInputFormats and SupportedOutputFormats do not exist and SupportedReformattings does. SupportedReformattings needs to be tested.
| :Name "UMM-S" | ||
| :Version "1.5.4"} | ||
| :ServiceOptions {:SupportedInputFormats ["HDF4"] | ||
| :SupportedOutputFormats ["HDF4"]}} |
There was a problem hiding this comment.
SupportedInputFormats and SupportedOutputFormats both do not exist in the schema under ServiceOptions. SupportedReformattings should be tested.
| :MetadataSpecification {:URL "https://cdn.earthdata.nasa.gov/umm/service/v1.5.3" | ||
| :Name "UMM-S" | ||
| :Version "1.5.3"} | ||
| :ServiceOptions {:SupportedInputFormats ["HDF4"] |
There was a problem hiding this comment.
same here, SupportedInputFormats and SupportedOutputFormats do not exist under service options.
If you look at your code changes you only updated the metadata specifications when going from 1.5.3 to 1.5.4.
| {"ProjectionName": "Geographic", "ProjectionAuthority": "ep"} | ||
| ], | ||
| "InterpolationTypes": ["Elliptical Weighted Averaging"], | ||
| "SupportedReformattings": [ |
There was a problem hiding this comment.
Since this file is being used to test 1.5.4 down migration to 1.5.3, a supported input and out format should include NETCDF-4 (OPeNDAP URL)
eereiter
left a comment
eereiter
left a comment
There was a problem hiding this comment.
The major problem that I see is the tests are testing not valid records and not testing what actually changed in the schema.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4-to-v1.5.3.json`:
- Line 5: The expected-output JSON contains the "Type" field set to "SWODLR"
which is inconsistent with the migration and the source JSON; update the
expected output so the "Type" value is "OPeNDAP". Locate the JSON entry with
"Type": "SWODLR" in the Service_v1.5.4-to-v1.5.3 expected-output and replace the
value with "OPeNDAP" to match the source Service_v1.5.4.json and the
migrate-1_5_4-down-to-1_5_3 unit test expectations.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj`:
- Around line 673-680: The test's expected output for the 1.5.4→1.5.3 migration
is wrong: update the expected JSON in "v1.5.4/Service_v1.5.4-to-v1.5.3.json" so
the "Type" field matches the source "v1.5.4/Service_v1.5.4.json" ("OPeNDAP")
instead of "SWODLR", or alternatively change the test to expect the current
non-transforming behavior; adjust the file referenced by the migration test (the
"Migrating down from 1.5.4 to 1.5.3" case) so the expected output matches the
actual migration output.
umm-spec-lib/resources/example-data/umm-json/service/v1.5.4/Service_v1.5.4-to-v1.5.3.json
Outdated
Show resolved
Hide resolved
umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj`:
- Around line 369-386: The three functions
remove-non-valid-formats-1_5_4-to-1_5_3,
remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3, and
remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 are defined twice; remove
the duplicate definitions (the second set) so each function is declared only
once, leaving the original implementations in place and ensuring no silent
replacement occurs.
In `@umm-spec-lib/src/cmr/umm_spec/migration/version/service.clj`:
- Around line 636-650: The migration currently in the
interface/migrate-umm-version multimethod for [:service "1.5.4" "1.5.3"] only
filters SupportedReformattings by SupportedInputFormat and misses removing
"NETCDF-4 (OPeNDAP URL)" from SupportedOutputFormats; replace the inline update
logic under :ServiceOptions that updates :SupportedReformattings with a call to
the existing remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 function
(from service_service_options.clj) so both SupportedInputFormat and
SupportedOutputFormats are cleaned, keep the surrounding
util/remove-nil-keys/seq checks and then proceed to dissoc
:MetadataSpecification and call m-spec/update-version :service "1.5.3".
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/service_service_options.clj`:
- Around line 263-297: The file contains duplicate test definitions:
remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test and
remove-non-valid-formats-1-5-4-to-1-5-3-test are defined twice; remove the
second copy (the duplicate block starting at the second occurrence) so each
deftest appears only once. Locate the tests that call
service/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 and
service/remove-non-valid-formats-1_5_4-to-1_5_3 and delete the duplicate
definitions, leaving the original tests intact.
🧹 Nitpick comments (1)
umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj (1)
586-612:load-service-filenow throws on missing files, making the nil guards inmigrations-up-and-downdead code.The new
try/catchinload-service-file(line 593) re-throws viaex-info, so it will never returnnil. The(when (and expected source) ...)guard on line 606 and the skip-message block on lines 609–612 are now unreachable for the nil case — a missing file will cause an exception before either branch executes.Consider either removing the nil guards (since exceptions will surface naturally) or changing
load-service-fileto returnnilon failure (removing the re-throw) if silent skipping is the desired behavior.
| (defn remove-non-valid-formats-1_5_4-to-1_5_3 | ||
| "Remove the non valid Supported Format enumerations when migrating from 1.5.4 to 1.5.3." | ||
| [supported-formats] | ||
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" %) supported-formats))) | ||
|
|
||
| (defn- remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 | ||
| [reformatting] | ||
| (let [input (remove-non-valid-formats-1_5_4-to-1_5_3 [(:SupportedInputFormat reformatting)]) | ||
| outputs (remove-non-valid-formats-1_5_4-to-1_5_3 (:SupportedOutputFormats reformatting))] | ||
| (when (and (seq input) (seq outputs)) | ||
| (assoc reformatting | ||
| :SupportedInputFormat (first input) | ||
| :SupportedOutputFormats outputs)))) | ||
|
|
||
| (defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 | ||
| "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3" | ||
| [reformattings] | ||
| (vec (keep remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 reformattings))) |
There was a problem hiding this comment.
Duplicate function definitions — remove the second copy.
The three functions (remove-non-valid-formats-1_5_4-to-1_5_3, remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3, remove-reformattings-non-valid-formats-1_5_4-to-1_5_3) are defined twice: lines 351–368 and again at lines 369–386. The second defn silently replaces the first. Since both copies are identical there's no runtime bug, but this copy-paste duplication should be removed.
Proposed fix — delete lines 369–386
-(defn remove-non-valid-formats-1_5_4-to-1_5_3
- "Remove the non valid Supported Format enumerations when migrating from 1.5.4 to 1.5.3."
- [supported-formats]
- (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" %) supported-formats)))
-
-(defn- remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3
- [reformatting]
- (let [input (remove-non-valid-formats-1_5_4-to-1_5_3 [(:SupportedInputFormat reformatting)])
- outputs (remove-non-valid-formats-1_5_4-to-1_5_3 (:SupportedOutputFormats reformatting))]
- (when (and (seq input) (seq outputs))
- (assoc reformatting
- :SupportedInputFormat (first input)
- :SupportedOutputFormats outputs))))
-
-(defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3
- "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3"
- [reformattings]
- (vec (keep remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 reformattings)))📝 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.
| (defn remove-non-valid-formats-1_5_4-to-1_5_3 | |
| "Remove the non valid Supported Format enumerations when migrating from 1.5.4 to 1.5.3." | |
| [supported-formats] | |
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" %) supported-formats))) | |
| (defn- remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 | |
| [reformatting] | |
| (let [input (remove-non-valid-formats-1_5_4-to-1_5_3 [(:SupportedInputFormat reformatting)]) | |
| outputs (remove-non-valid-formats-1_5_4-to-1_5_3 (:SupportedOutputFormats reformatting))] | |
| (when (and (seq input) (seq outputs)) | |
| (assoc reformatting | |
| :SupportedInputFormat (first input) | |
| :SupportedOutputFormats outputs)))) | |
| (defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 | |
| "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3" | |
| [reformattings] | |
| (vec (keep remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 reformattings))) |
🤖 Prompt for AI Agents
In `@umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj` around
lines 369 - 386, The three functions remove-non-valid-formats-1_5_4-to-1_5_3,
remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3, and
remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 are defined twice; remove
the duplicate definitions (the second set) so each function is declared only
once, leaving the original implementations in place and ensuring no silent
replacement occurs.
| (deftest remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test | ||
| (testing "CMR-11048: Test 1.5.4 -> 1.5.3 migration removes NETCDF-4 (OPeNDAP URL)" | ||
| (are3 [supported-reformattings expected-supported-reformattings] | ||
| (is (= expected-supported-reformattings | ||
| (service/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 supported-reformattings))) | ||
|
|
||
| "Testing the removal in Supported Output Formats." | ||
| [{:SupportedInputFormat "HDF4" | ||
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4" "NETCDF-3"]} | ||
| {:SupportedInputFormat "Shapefile" | ||
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}] | ||
| [{:SupportedInputFormat "HDF4" | ||
| :SupportedOutputFormats ["HDF4" "NETCDF-3"]}] | ||
|
|
||
| "Testing the removal in Supported Input Formats." | ||
| [{:SupportedInputFormat "NETCDF-4 (OPeNDAP URL)" | ||
| :SupportedOutputFormats ["HDF4"]} | ||
| {:SupportedInputFormat "Shapefile" | ||
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4"]}] | ||
| [{:SupportedInputFormat "Shapefile" | ||
| :SupportedOutputFormats ["HDF4"]}] | ||
|
|
||
| "Testing removal of pair when outputs become empty" | ||
| [{:SupportedInputFormat "HDF4" | ||
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}] | ||
| []))) | ||
|
|
||
| (deftest remove-non-valid-formats-1-5-4-to-1-5-3-test | ||
| (testing "Removal of NETCDF-4 (OPeNDAP URL) from supported formats list" | ||
| (is (= ["HDF4" "NETCDF-3"] | ||
| (service/remove-non-valid-formats-1_5_4-to-1_5_3 | ||
| ["HDF4" "NETCDF-4 (OPeNDAP URL)" "NETCDF-3"]))) | ||
| (is (= [] | ||
| (service/remove-non-valid-formats-1_5_4-to-1_5_3 | ||
| ["NETCDF-4 (OPeNDAP URL)"]))))) |
There was a problem hiding this comment.
Duplicate test definitions — remove the second copy.
remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test and remove-non-valid-formats-1-5-4-to-1-5-3-test are each defined twice (lines 228–262 and again at lines 263–297). In Clojure, the second deftest with the same name silently overwrites the first. While both copies are identical so no tests are lost, this is a clear copy-paste artifact that should be cleaned up.
Proposed fix — delete lines 263–297
-(deftest remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test
- (testing "CMR-11048: Test 1.5.4 -> 1.5.3 migration removes NETCDF-4 (OPeNDAP URL)"
- (are3 [supported-reformattings expected-supported-reformattings]
- (is (= expected-supported-reformattings
- (service/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 supported-reformattings)))
-
- "Testing the removal in Supported Output Formats."
- [{:SupportedInputFormat "HDF4"
- :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4" "NETCDF-3"]}
- {:SupportedInputFormat "Shapefile"
- :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}]
- [{:SupportedInputFormat "HDF4"
- :SupportedOutputFormats ["HDF4" "NETCDF-3"]}]
-
- "Testing the removal in Supported Input Formats."
- [{:SupportedInputFormat "NETCDF-4 (OPeNDAP URL)"
- :SupportedOutputFormats ["HDF4"]}
- {:SupportedInputFormat "Shapefile"
- :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4"]}]
- [{:SupportedInputFormat "Shapefile"
- :SupportedOutputFormats ["HDF4"]}]
-
- "Testing removal of pair when outputs become empty"
- [{:SupportedInputFormat "HDF4"
- :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}]
- [])))
-
-(deftest remove-non-valid-formats-1-5-4-to-1-5-3-test
- (testing "Removal of NETCDF-4 (OPeNDAP URL) from supported formats list"
- (is (= ["HDF4" "NETCDF-3"]
- (service/remove-non-valid-formats-1_5_4-to-1_5_3
- ["HDF4" "NETCDF-4 (OPeNDAP URL)" "NETCDF-3"])))
- (is (= []
- (service/remove-non-valid-formats-1_5_4-to-1_5_3
- ["NETCDF-4 (OPeNDAP URL)"])))))📝 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.
| (deftest remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test | |
| (testing "CMR-11048: Test 1.5.4 -> 1.5.3 migration removes NETCDF-4 (OPeNDAP URL)" | |
| (are3 [supported-reformattings expected-supported-reformattings] | |
| (is (= expected-supported-reformattings | |
| (service/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 supported-reformattings))) | |
| "Testing the removal in Supported Output Formats." | |
| [{:SupportedInputFormat "HDF4" | |
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4" "NETCDF-3"]} | |
| {:SupportedInputFormat "Shapefile" | |
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}] | |
| [{:SupportedInputFormat "HDF4" | |
| :SupportedOutputFormats ["HDF4" "NETCDF-3"]}] | |
| "Testing the removal in Supported Input Formats." | |
| [{:SupportedInputFormat "NETCDF-4 (OPeNDAP URL)" | |
| :SupportedOutputFormats ["HDF4"]} | |
| {:SupportedInputFormat "Shapefile" | |
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)" "HDF4"]}] | |
| [{:SupportedInputFormat "Shapefile" | |
| :SupportedOutputFormats ["HDF4"]}] | |
| "Testing removal of pair when outputs become empty" | |
| [{:SupportedInputFormat "HDF4" | |
| :SupportedOutputFormats ["NETCDF-4 (OPeNDAP URL)"]}] | |
| []))) | |
| (deftest remove-non-valid-formats-1-5-4-to-1-5-3-test | |
| (testing "Removal of NETCDF-4 (OPeNDAP URL) from supported formats list" | |
| (is (= ["HDF4" "NETCDF-3"] | |
| (service/remove-non-valid-formats-1_5_4-to-1_5_3 | |
| ["HDF4" "NETCDF-4 (OPeNDAP URL)" "NETCDF-3"]))) | |
| (is (= [] | |
| (service/remove-non-valid-formats-1_5_4-to-1_5_3 | |
| ["NETCDF-4 (OPeNDAP URL)"]))))) |
🤖 Prompt for AI Agents
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/service_service_options.clj`
around lines 263 - 297, The file contains duplicate test definitions:
remove-reformattings-non-valid-formats-1-5-4-to-1-5-3-test and
remove-non-valid-formats-1-5-4-to-1-5-3-test are defined twice; remove the
second copy (the duplicate block starting at the second occurrence) so each
deftest appears only once. Locate the tests that call
service/remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 and
service/remove-non-valid-formats-1_5_4-to-1_5_3 and delete the duplicate
definitions, leaving the original tests intact.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
umm-spec-lib/test/cmr/umm_spec/test/service_schema_test.clj (1)
7-11: Test description mentions OPeNDAP but loads the generic v1.5.4 example.The
testingstring on line 8 says "NETCDF-4 (OPeNDAP URL)" but line 9 loadsService_v1.5.4.json. The directory contains a dedicatedService_v1.5.4_NETCDF4_OPeNDAP.jsonfile that explicitly tests that format. Load the OPeNDAP-specific file or update the test description to match what's actually being validated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@umm-spec-lib/test/cmr/umm_spec/test/service_schema_test.clj` around lines 7 - 11, The test service-supported-format-type-enum-test's description mentions NETCDF-4 (OPeNDAP) but the code loads the generic Service_v1.5.4.json; update the test to load the OPeNDAP-specific fixture or change the testing string to reflect the generic file. Specifically, in the deftest service-supported-format-type-enum-test replace the resource loaded into valid-service-json with "example-data/umm-json/service/v1.5.4/Service_v1.5.4_NETCDF4_OPeNDAP.json" to match the testing description, or alternatively change the testing message to describe validating the generic Service_v1.5.4.json so the testing string and the valid-service-json variable are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@umm-spec-lib/src/cmr/umm_spec/json_schema.clj`:
- Around line 256-257: Replace the ad-hoc println debug statements in
umm_spec.json_schema.clj (the lines printing "Debug: Loading schema for" with
concept-type and umm-version, and "Debug: Schema path:" with schema-path, as
well as the other printlns at the same block) with the project's logging call
(e.g., cmr.common.log/debug) so diagnostics go through the logging framework;
update the calls to use the logging API and include the same contextual values
(concept-type, umm-version, schema-path) and remove the printlns entirely so
stdout is not polluted on schema load.
In `@umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj`:
- Around line 387-390: Remove the duplicate, incomplete definition of
remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 and the surrounding
conflicting block so the original implementation (which delegates to the helper
that strips "NETCDF-4 (OPeNDAP URL)" from both :SupportedInputFormat and
:SupportedOutputFormats and drops entries when either side is empty) remains the
active one; specifically delete the later redefinition that only checks
:SupportedInputFormat and also remove the dead helper redefinition
remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 so there is a single
correct implementation of remove-reformattings-non-valid-formats-1_5_4-to-1_5_3.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj`:
- Around line 590-605: load-service-file currently catches all exceptions and
always rethrows via ex-info, making the caller-side when guards in
migrations-up-and-down dead; change load-service-file to return nil for
missing/nonexistent resource instead of rethrowing (e.g., detect io/resource or
FileNotFoundException and return nil), and only throw for unexpected errors, so
the (when (and expected source) ...) guard in migrations-up-and-down becomes
effective; alternatively, if you prefer to let errors propagate, remove the
try/catch entirely and delete the unreachable when guards in
migrations-up-and-down—refer to load-service-file and migrations-up-and-down to
locate the changes.
- Around line 741-754: Remove the test-local redefinition: delete the helper
function update-service-options-1_5_4->1_5_3 and the defmethod for
interface/migrate-umm-version for [:service "1.5.4" "1.5.3"] from this test
namespace so the tests use the real migration in
cmr.umm-spec.migration.version.service (which contains util/remove-nil-keys and
proper nil guards) rather than shadowing/overwriting production logic.
- Around line 611-619: The skip-message is dead and missing files currently
cause exceptions; to implement silent skipping change load-service-file to catch
file-not-found/parse exceptions and return nil instead of rethrowing, and
replace the outer when that currently guards the migration call with an if/else:
if (and expected source) call vm/migrate-umm and assert equality, else print the
skip message using destination-file/source-file; reference load-service-file and
vm/migrate-umm so you update both the loader to return nil on missing files and
the test block (expected/source/source-file/destination-file) to use if/else
rather than nested when forms.
---
Duplicate comments:
In `@umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj`:
- Around line 369-386: The block defines duplicate functions
remove-non-valid-formats-1_5_4-to-1_5_3,
remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3, and
remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 that are already declared
earlier; remove the repeated copy (the entire duplicated block shown) so only
the original definitions remain and ensure any references use the single set of
functions.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj`:
- Around line 727-761: The file defines deftest type-preservation-test three
times causing the first two to be overwritten; remove the duplicate test
definitions and keep a single deftest type-preservation-test (or rename others
if you intended distinct tests) so the assertions around load-service-file,
vm/migrate-umm, and the Type checks run exactly once; ensure the migrate method
interface/migrate-umm-version and helper update-service-options-1_5_4->1_5_3
remain unchanged.
---
Nitpick comments:
In `@umm-spec-lib/test/cmr/umm_spec/test/service_schema_test.clj`:
- Around line 7-11: The test service-supported-format-type-enum-test's
description mentions NETCDF-4 (OPeNDAP) but the code loads the generic
Service_v1.5.4.json; update the test to load the OPeNDAP-specific fixture or
change the testing string to reflect the generic file. Specifically, in the
deftest service-supported-format-type-enum-test replace the resource loaded into
valid-service-json with
"example-data/umm-json/service/v1.5.4/Service_v1.5.4_NETCDF4_OPeNDAP.json" to
match the testing description, or alternatively change the testing message to
describe validating the generic Service_v1.5.4.json so the testing string and
the valid-service-json variable are consistent.
| _ (println "Debug: Loading schema for" concept-type "version" umm-version) | ||
| _ (println "Debug: Schema path:" schema-path) |
There was a problem hiding this comment.
Remove debug println statements before merging.
Lines 256–257 (and 263, 272) use println for debug output. These will write to stdout on every first schema load, polluting server logs. If diagnostic logging is needed, use the project's logging framework (e.g., cmr.common.log/debug) instead.
Proposed fix
([concept-type umm-version]
(let [schema-name (concept-schema-name concept-type)
schema-path (umm-schema-path concept-type schema-name umm-version)
- _ (println "Debug: Loading schema for" concept-type "version" umm-version)
- _ (println "Debug: Schema path:" schema-path)
resource (umm-schema-resource concept-type schema-name umm-version)]
(if resource
(try
(load-schema concept-type schema-name umm-version)
(catch Exception e
- (println "Error loading schema:" (.getMessage e))
(throw (ex-info (str "Error loading schema for " concept-type " version " umm-version)
{:concept-type concept-type
:umm-version umm-version
:schema-name schema-name
:schema-path schema-path
:error (.getMessage e)}
e))))
(do
- (println "Schema resource not found for" concept-type "version" umm-version)
(throw (ex-info (str "Unable to find schema resource for " concept-type " version " umm-version)
{:concept-type concept-type
:umm-version umm-version
:schema-name schema-name
:schema-path schema-path})))))))📝 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.
| _ (println "Debug: Loading schema for" concept-type "version" umm-version) | |
| _ (println "Debug: Schema path:" schema-path) | |
| ([concept-type umm-version] | |
| (let [schema-name (concept-schema-name concept-type) | |
| schema-path (umm-schema-path concept-type schema-name umm-version) | |
| resource (umm-schema-resource concept-type schema-name umm-version)] | |
| (if resource | |
| (try | |
| (load-schema concept-type schema-name umm-version) | |
| (catch Exception e | |
| (throw (ex-info (str "Error loading schema for " concept-type " version " umm-version) | |
| {:concept-type concept-type | |
| :umm-version umm-version | |
| :schema-name schema-name | |
| :schema-path schema-path | |
| :error (.getMessage e)} | |
| e)))) | |
| (do | |
| (throw (ex-info (str "Unable to find schema resource for " concept-type " version " umm-version) | |
| {:concept-type concept-type | |
| :umm-version umm-version | |
| :schema-name schema-name | |
| :schema-path schema-path})))))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@umm-spec-lib/src/cmr/umm_spec/json_schema.clj` around lines 256 - 257,
Replace the ad-hoc println debug statements in umm_spec.json_schema.clj (the
lines printing "Debug: Loading schema for" with concept-type and umm-version,
and "Debug: Schema path:" with schema-path, as well as the other printlns at the
same block) with the project's logging call (e.g., cmr.common.log/debug) so
diagnostics go through the logging framework; update the calls to use the
logging API and include the same contextual values (concept-type, umm-version,
schema-path) and remove the printlns entirely so stdout is not polluted on
schema load.
| (defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 | ||
| "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3" | ||
| [reformattings] | ||
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" (:SupportedInputFormat %)) reformattings))) |
There was a problem hiding this comment.
Third redefinition of remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 silently wins with incomplete logic.
In Clojure the last defn for a given name wins. This third definition (Line 387) replaces the version on Line 365 (and Line 383). The two implementations differ:
- Lines 365–368 (intended): delegates to the helper that filters
"NETCDF-4 (OPeNDAP URL)"from both input and output formats, and drops the entire reformatting entry when either side becomes empty. - Lines 387–390 (active): only removes reformatting entries whose
:SupportedInputFormatequals"NETCDF-4 (OPeNDAP URL)". It does not strip the invalid format from:SupportedOutputFormats, so output lists can still contain"NETCDF-4 (OPeNDAP URL)"after migration — producing an invalid 1.5.3 record.
Additionally, this makes remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 dead code.
Delete lines 369–390 entirely, keeping only the first set of definitions (lines 351–368).
Proposed fix — remove all duplicate/conflicting definitions
(defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3
"Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3"
[reformattings]
(vec (keep remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 reformattings)))
-(defn remove-non-valid-formats-1_5_4-to-1_5_3
- "Remove the non valid Supported Format enumerations when migrating from 1.5.4 to 1.5.3."
- [supported-formats]
- (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" %) supported-formats)))
-
-(defn- remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3
- [reformatting]
- (let [input (remove-non-valid-formats-1_5_4-to-1_5_3 [(:SupportedInputFormat reformatting)])
- outputs (remove-non-valid-formats-1_5_4-to-1_5_3 (:SupportedOutputFormats reformatting))]
- (when (and (seq input) (seq outputs))
- (assoc reformatting
- :SupportedInputFormat (first input)
- :SupportedOutputFormats outputs))))
-
-(defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3
- "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3"
- [reformattings]
- (vec (keep remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 reformattings)))
-(defn remove-reformattings-non-valid-formats-1_5_4-to-1_5_3
- "Remove the non valid formats going from UMM-S version 1.5.4 to UMM-S version 1.5.3"
- [reformattings]
- (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" (:SupportedInputFormat %)) reformattings)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@umm-spec-lib/src/cmr/umm_spec/migration/service_service_options.clj` around
lines 387 - 390, Remove the duplicate, incomplete definition of
remove-reformattings-non-valid-formats-1_5_4-to-1_5_3 and the surrounding
conflicting block so the original implementation (which delegates to the helper
that strips "NETCDF-4 (OPeNDAP URL)" from both :SupportedInputFormat and
:SupportedOutputFormats and drops entries when either side is empty) remains the
active one; specifically delete the later redefinition that only checks
:SupportedInputFormat and also remove the dead helper redefinition
remove-reformattings-when-input-not-valid-1_5_4-to-1_5_3 so there is a single
correct implementation of remove-reformattings-non-valid-formats-1_5_4-to-1_5_3.
| (defn- load-service-file | ||
| "Load a test data file for services" | ||
| [version-file] | ||
| (decode (->> version-file | ||
| (format "example-data/umm-json/service/%s") | ||
| io/resource | ||
| io/file | ||
| slurp) | ||
| true)) | ||
| (try | ||
| (decode (->> version-file | ||
| (format "example-data/umm-json/service/%s") | ||
| io/resource | ||
| io/file | ||
| slurp) | ||
| true) | ||
| (catch Exception e | ||
| (println (str "Error loading file: " version-file)) | ||
| (println (.getMessage e)) | ||
| (throw (ex-info (str "Failed to load service file: " version-file) | ||
| {:cause (.getMessage e) | ||
| :file version-file}))))) |
There was a problem hiding this comment.
load-service-file catches and rethrows — the when guards in the caller are unreachable.
The try/catch here prints the error then rethrows via ex-info. This means load-service-file will never return nil on a missing file — it always throws. The (when (and expected source) ...) guard in migrations-up-and-down (line 613) therefore provides no protection; the test will fail with an exception before the guard is checked.
If silent skipping is not intended, simplify by removing the try/catch wrapper (let it throw naturally) and remove the dead when guards in the caller. If silent skipping is intended, return nil instead of rethrowing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj` around
lines 590 - 605, load-service-file currently catches all exceptions and always
rethrows via ex-info, making the caller-side when guards in
migrations-up-and-down dead; change load-service-file to return nil for
missing/nonexistent resource instead of rethrowing (e.g., detect io/resource or
FileNotFoundException and return nil), and only throw for unexpected errors, so
the (when (and expected source) ...) guard in migrations-up-and-down becomes
effective; alternatively, if you prefer to let errors propagate, remove the
try/catch entirely and delete the unreachable when guards in
migrations-up-and-down—refer to load-service-file and migrations-up-and-down to
locate the changes.
| (let [expected (load-service-file destination-file) | ||
| source (load-service-file source-file) | ||
| actual (vm/migrate-umm {} :service source-version destination-version source)] | ||
| (is (= expected actual))) | ||
| source (load-service-file source-file)] | ||
| (when (and expected source) | ||
| (let [actual (vm/migrate-umm {} :service source-version destination-version source)] | ||
| (is (= expected actual))) | ||
| (when (or (nil? expected) (nil? source)) | ||
| (println (str "Skipping test due to missing file(s): " | ||
| (when (nil? expected) destination-file) | ||
| (when (nil? source) source-file)))))) |
There was a problem hiding this comment.
Bug: skip-notification logic is dead code — missing files silently pass.
The inner when on line 616 checks (or (nil? expected) (nil? source)), but it's nested inside the outer when on line 613 that requires (and expected source). These conditions are mutually exclusive, so the skip message on lines 617–619 will never print.
More importantly, when a test data file is missing, load-service-file will throw (line 603), so the when guard on line 613 won't help anyway — the test will error out rather than skip. If silent skipping is truly desired, the try/catch in load-service-file would need to return nil instead of rethrowing, and the control flow here should use if/else:
Proposed fix (if skipping is intended)
(let [expected (load-service-file destination-file)
source (load-service-file source-file)]
- (when (and expected source)
- (let [actual (vm/migrate-umm {} :service source-version destination-version source)]
- (is (= expected actual)))
- (when (or (nil? expected) (nil? source))
- (println (str "Skipping test due to missing file(s): "
- (when (nil? expected) destination-file)
- (when (nil? source) source-file))))))
+ (if (and expected source)
+ (let [actual (vm/migrate-umm {} :service source-version destination-version source)]
+ (is (= expected actual)))
+ (println (str "Skipping test due to missing file(s): "
+ (when (nil? expected) destination-file)
+ (when (nil? source) source-file)))))However, since load-service-file rethrows exceptions (line 603), returning nil never happens. Consider whether you actually want silent skipping or prefer a hard failure on missing files (the simpler and safer choice).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj` around
lines 611 - 619, The skip-message is dead and missing files currently cause
exceptions; to implement silent skipping change load-service-file to catch
file-not-found/parse exceptions and return nil instead of rethrowing, and
replace the outer when that currently guards the migration call with an if/else:
if (and expected source) call vm/migrate-umm and assert equality, else print the
skip message using destination-file/source-file; reference load-service-file and
vm/migrate-umm so you update both the loader to return nil on missing files and
the test block (expected/source/source-file/destination-file) to use if/else
rather than nested when forms.
| (defn update-service-options-1_5_4->1_5_3 | ||
| [service-options] | ||
| (if service-options | ||
| (update service-options :SupportedReformattings | ||
| (fn [reformattings] | ||
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" (:SupportedInputFormat %)) reformattings)))) | ||
| service-options)) | ||
|
|
||
| (defmethod interface/migrate-umm-version [:service "1.5.4" "1.5.3"] | ||
| [_context umm-s & _] | ||
| (-> umm-s | ||
| (update :ServiceOptions update-service-options-1_5_4->1_5_3) | ||
| (dissoc :MetadataSpecification) | ||
| (m-spec/update-version :service "1.5.3"))) |
There was a problem hiding this comment.
Critical: Production migration logic redefined inside the test namespace.
Lines 741–748 define a helper update-service-options-1_5_4->1_5_3 and lines 749–754 redefine the interface/migrate-umm-version [:service "1.5.4" "1.5.3"] multimethod — all inside the test file. When this test namespace is loaded, it overwrites the real migration implementation from cmr.umm-spec.migration.version.service with this simplified version (which lacks util/remove-nil-keys, the (when % ...) nil guard, etc.).
This means:
- Any test running after this ns is loaded will use the test-file implementation instead of the production one.
- The production migration logic is effectively untested — tests pass against this local redefinition, not the real code.
Remove this defn and defmethod entirely. The tests should exercise the production implementation, not a shadow copy.
-(defn update-service-options-1_5_4->1_5_3
- [service-options]
- (if service-options
- (update service-options :SupportedReformattings
- (fn [reformattings]
- (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" (:SupportedInputFormat %)) reformattings))))
- service-options))
-
-(defmethod interface/migrate-umm-version [:service "1.5.4" "1.5.3"]
- [_context umm-s & _]
- (-> umm-s
- (update :ServiceOptions update-service-options-1_5_4->1_5_3)
- (dissoc :MetadataSpecification)
- (m-spec/update-version :service "1.5.3")))📝 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.
| (defn update-service-options-1_5_4->1_5_3 | |
| [service-options] | |
| (if service-options | |
| (update service-options :SupportedReformattings | |
| (fn [reformattings] | |
| (vec (remove #(= "NETCDF-4 (OPeNDAP URL)" (:SupportedInputFormat %)) reformattings)))) | |
| service-options)) | |
| (defmethod interface/migrate-umm-version [:service "1.5.4" "1.5.3"] | |
| [_context umm-s & _] | |
| (-> umm-s | |
| (update :ServiceOptions update-service-options-1_5_4->1_5_3) | |
| (dissoc :MetadataSpecification) | |
| (m-spec/update-version :service "1.5.3"))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@umm-spec-lib/test/cmr/umm_spec/test/migration/version/service.clj` around
lines 741 - 754, Remove the test-local redefinition: delete the helper function
update-service-options-1_5_4->1_5_3 and the defmethod for
interface/migrate-umm-version for [:service "1.5.4" "1.5.3"] from this test
namespace so the tests use the real migration in
cmr.umm-spec.migration.version.service (which contains util/remove-nil-keys and
proper nil guards) rather than shadowing/overwriting production logic.
| (concept-schema* concept-type (ver/current-version concept-type))) | ||
| ([concept-type umm-version] | ||
| (load-schema concept-type (concept-schema-name concept-type) umm-version))) | ||
| (let [schema-name (concept-schema-name concept-type) |
There was a problem hiding this comment.
The changes to this file are for debugging. Please revert the changes.
4 participants
Overview
What is the objective?
The objective is to implement OPeNDAP URL support in UMM-S 1.5.4 and create a migration path from 1.5.4 to 1.5.3.
What are the changes?
What areas of the application does this impact?
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Chores
Tests