CMR-10801: As a metadata steward, I should not get a CMR validation error for a keyword long name when it’s not provided and not required #2350
Conversation
WalkthroughThis change extends KMS lookup functionality to support long-name validation for platforms, instruments, and projects. It treats nil, blank, or "Not provided" long-names as optional by introducing generic lookup logic that conditionally strips long-name fields from comparison maps to enable correct matching. Changes
Sequence DiagramsequenceDiagram
participant Lookup as lookup-by-umm-c-keyword
participant Router as Dispatch Router
participant Generic as lookup-by-umm-c-keyword-with-long-name
participant Validator as skip-long-name-validation?
participant Cache as KMS Cache
Lookup->>Router: Receive :instruments/:projects
Router->>Generic: Delegate with transform-fn
Generic->>Validator: Check if long-name should skip
alt Skip Long-name
Generic->>Generic: Remove :long-name from comparison map
else Keep Long-name
Generic->>Generic: Use comparison map as-is
end
Generic->>Cache: Fetch with (normalized) comparison map
Cache-->>Generic: Return keyed cache entry or nil
Generic-->>Lookup: Return result or handle Carmine error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common-app-lib/test/cmr/common_app/test/services/kms_lookup.clj (1)
243-310: Long-name tests thoroughly exercise nil/blank/'Not provided' semanticsThe new
blank-and-not-provided-long-name-validation-test:
- Verifies that nil/empty/
"Not provided"all match KMS entries missing long-names for platforms, instruments, and projects.- Confirms that non-blank, non-
"Not provided"long-names still fail, even when short-name is valid.- Reasserts CMRS-4400 behavior for platforms with long-names in KMS (PLAT1) for both nil and exact long-name cases.
This gives solid unit-level coverage of the new lookup behavior. If we ever need to widen guarantees, optional follow-up would be to add analogous “KMS has long-name, UMM says 'Not provided'” tests for instruments/projects, but that’s not required for this PR.
common-app-lib/src/cmr/common_app/services/kms_lookup.clj (1)
279-333: Generic long-name-aware lookup and wrappers are sound; consider minor doc/perf clarificationsThe new
lookup-by-umm-c-keyword-with-long-nameplus the platform/instrument/project wrappers achieves:
- Consistent normalization (
kebab-case+ optional transform) across the three keyword schemes.- Conditional stripping of
:long-nameon both the UMM keyword and KMS index whenskip-long-name-validation?is true, so nil/blank/"Not provided"no longer cause false negatives.- Preservation of strict matching whenever
:long-nameis any other non-blank value, as verified by the new tests.A couple of small, non-blocking follow-ups you might consider:
- The
remove-long-name-from-kms-indexdocstring still talks only about “nil” long-names; updating it to mention blank/"Not provided"and all three schemes would better reflect current use.remove-long-name-from-kms-indexrebuilds the entire scheme index map on each lookup when skipping long-names. This was already true for the older platform path, but it now applies to instruments/projects as well. If lookups on these schemes become hot paths, precomputing or caching a “no-long-name” variant per scheme could avoid repeated work.- If KMS ever contained multiple entries that differ only by
:long-namefor the same short-name (and other fields), stripping:long-namecould collapse them; worth confirming that KMS schema/constraints prevent such duplicates.These are refinements rather than correctness issues; the current behavior matches the tests and PR intent.
Also applies to: 335-357, 367-368
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common-app-lib/src/cmr/common_app/services/kms_lookup.clj(3 hunks)common-app-lib/test/cmr/common_app/test/services/kms_lookup.clj(2 hunks)system-int-test/test/cmr/system_int_test/ingest/collection/collection_keyword_validation_test.clj(1 hunks)
🔇 Additional comments (3)
system-int-test/test/cmr/system_int_test/ingest/collection/collection_keyword_validation_test.clj (1)
477-529: New 'Not provided' integration tests are well-targeted and aligned with existing patternsThe added tests cleanly cover:
- Valid cases for Platforms/Instruments/Projects when long-name is
"Not provided".- Negative cases ensuring arbitrary non-blank long names still fail even when short names are valid.
Assertions reuse existing helpers and message formats, so they’ll stay in sync with prior behavior. No changes needed here.
common-app-lib/test/cmr/common_app/test/services/kms_lookup.clj (1)
14-19: Sample KMS additions for PLAT2/INST2/PROJ2 look consistentThe extra platform/instrument/project entries cleanly model the “no long-name in KMS” scenarios without disturbing existing tests. Keys follow the same shape and casing as the rest of
sample-map.common-app-lib/src/cmr/common_app/services/kms_lookup.clj (1)
270-277: skip-long-name-validation? correctly captures the intended 'Not provided' semanticsThe helper cleanly treats nil, blank, and case/whitespace-insensitive
"Not provided"as “no long-name,” which matches the PR objective and keeps the behavior centralized. No changes needed here.
daniel-zamora
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2350 +/- ##
==========================================
- Coverage 58.15% 58.13% -0.02%
==========================================
Files 1063 1063
Lines 72169 72186 +17
Branches 2086 2081 -5
==========================================
- Hits 41967 41963 -4
- Misses 28264 28283 +19
- Partials 1938 1940 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview
What is the objective?
Fix KMS keyword validation to properly handle "Not provided" as a valid long-name value for Platforms, Instruments, and Projects. Previously, only nil long-names were handled correctly - any non-nil value (including "Not provided") would fail validation if the KMS entry lacked a long-name field.
What are the changes?
What areas of the application does this impact?
common-app-lib: KMS keyword lookup service and validation logic)
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.