Repository navigation
Fix: filter mandi-price response to the requested market and price fields - #119
Conversation
…ields [#107] Refuse a select whose market.marketCode is absent or empty before Agmarknet is called; without it the query widens to the whole district. Each on_select resource carries market.marketCode, where catalog-ondemand.yaml puts it. Refuse a select whose supportedPriceFields lists a value outside the pack's Minimum/Maximum/Modal enum, which would otherwise return rows with no price. Supersedes the marketCode change in #98.
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependencies
Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
|
📊 Test Coverage: ✅ Passed — 91% of changed lines covered, min 80% |
|
Implemented option 1 from the open question: |
kelvinprabhu
left a comment
kelvinprabhu
left a comment
There was a problem hiding this comment.
DONE SELF REVIEW
kelvinprabhu
added
bug
|
Mandi Price: Optional marketCode & district-level search (PR #119, issue #107) market:
type: object
anyOf:
- required: [marketCode]
- required: [marketName]
- required: [district]
- required: [state]
- required: [location]
Change decisions Where Agmarknet differs from the network spec Open questions PR: #119 |
Conclusions / Decisions
Rationale: Align the adapter schema with Agmarknet's actual API requirements while retaining optional market-level filtering. |
…price select [#107] State and district are checked in separate guards so the refusal names the missing one. validity.startsAt and validity.endsAt are mapped to the upstream's from_date and to_date, so both must be dates and the start must not be after the end; an empty or free-text value used to be sent upstream as "--" or crash the date reorder. Tests cover each mandatory field, Minimum alone, and all three price fields when none are given. Refs #97
Agmarknet takes no market name, so a select with marketName and no marketCode queries the district and keeps only the named market's rows, matched trimmed and case-insensitively. A name that matches none of the markets the district reported is refused with a 400 asking for market.marketCode; a district with no rows at all is an empty answer. jsonmapper: a response-half $error starting with "[refused] " is now a 400 in the mapping's words rather than a 502, for refusals that can only be decided once the provider has answered. Other response failures stay 502. Refs #97
…as the response half's 502 [#107] The "[refused]" marker in jsonmapper is removed, so jsonmapper is as on development. A marketName that matches none of the district's markets is still refused with a message asking for market.marketCode; it is raised from the response half and so reported as a 502. Refs #97
kelvinprabhu
left a comment
kelvinprabhu
left a comment
There was a problem hiding this comment.
Make the comments shorter not more than 3 lines max
…ts [#107] Agmarknet takes no market name, so a name without a code would answer for the whole district. It is now refused before Agmarknet is called with a 400 that asks for market.marketCode, replacing the response-side name filter and its 502. Comments added by this change are kept to three lines. Refs #97
|
Review addressed: every comment added by this PR is now three lines or fewer, and a market name without a code is refused up front (400). |
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
@kelvinprabhu, the config/mappings/agmarknet/mandi-price.select.yaml file needs an update in the helm chart repository as well. Additionally, please update the API collection to include valid request and publish requests.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Redundant validations are defined.
The schema validator already refuses an unknown or empty supportedPriceFields, an empty or non-string marketCode, a validity that is not a date-time, and a commodity without a code. Keep only what the pack does not cover.
- state/district guards: $exists instead of a type check the schema covers - request: marketcode set directly; JSONata omits it when absent - response: @context read from $ra rather than the full path again - remove the schema-note comment
Open Question: Multiple Commodities in a Mandi SelectionPack: Agmarknet: Accepts only one
Current behaviour: The adapter rejects multiple commodities with HTTP 400, requiring exactly one commodity code. Without this validation, the consumer could receive a successful signed response containing only one commodity, while the others are silently dropped. Decision needed:
Which approach should we adopt? |
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Reviewed against the pack at network-specs@schema-packs-v0.1 and the adapter's validator rather than the description. Two inline findings, two here.
1. The header states a pack rule the pack does not contain
Problem
config/mappings/agmarknet/mandi-price.select.yaml:20-43 says the pack's OnDemand branch "EXCLUDES" market and validity, and builds twenty-four lines of reasoning on it. In MandiPrice/v0.1/attributes.yaml, market is on the top-level required list (line 21) and the OnDemand if/then adds only required: [supportedPriceFields] (lines 189-195). There is no exclusion anywhere in that file.
Recommendation
Delete lines 20-43 and 45-51. If the marketCode-optional decision needs recording, put it in an ADR under the repo's ADR directory and link it from this PR, not from the mapping.
2. The description claims three guards the diff does not add
Problem
The table lists marketCode non-empty, date format validation, and a supportedPriceFields enum. The file has six required checks and none of them is any of those. A reviewer reading the table approves behaviour that is not in the code.
Recommendation
Cut the table to two things: what was broken, and what this PR does about it. Drop those three rows, or say plainly that the pack enforces them.
Resolved
The validations flagged as redundant were correctly removed. The pack carries each one, and schemav2validator does enforce required, minLength and enum:
supportedCommoditiesitems --required: [code],attributes.yaml:56marketCode--minLength: 1,attributes.yaml:113supportedPriceFields--enum: [Minimum, Maximum, Modal],attributes.yaml:65
The validity existence check is not redundant and was right to keep: TimePeriod sets minProperties: 1, so the pack accepts one end on its own.
Still blocking: the multiple-commodities question from 4 Nov has no answer yet.
Decision: Multiple Commodities in a Mandi Selection After discussion, we will adopt the following approach:
Rationale: Agmarknet accepts only one commodity code per request. This approach maintains compatibility without silently implying that all requested commodities were processed. |
Team decision: several supportedCommodities are no longer refused with a 400. Only supportedCommodities[0] is sent to Agmarknet, which takes one code per call, and the plugin logs a warning naming the count.
…mmodity log [#107] - market.district and market.state are the requested codes, so an answer's market can be selected again - extra commodities are ignored without a plugin log; prerequisites.go is back to development
#107] The pack requires supportedCommodities with minItems 1 and a code on each item, and schemav2validator refuses those before the mapping runs.
Replaces the hand-written payload walk with the shared path walker.
kelvinprabhu
left a comment
kelvinprabhu
left a comment
There was a problem hiding this comment.
Done Review for Commodities
The pack's OnDemand branch only adds required: [supportedPriceFields]; it excludes neither market nor validity.
|
Re review 5796644178:
Multiple commodities: decided and implemented. |
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Re-reviewed at 91ca538. One inline, two here, and one of mine withdrawn.
Withdrawn: the date-shape finding
You are right and I was wrong. I grepped for compiler.AssertFormat() because go.mod pulls santhosh-tekuri, and missed that extended validation runs on kin-openapi. extended_schema.go:870 passes openapi3.EnableFormatValidation(), and kin-openapi registers a date-time RFC3339 validator by default (openapi3/schema_formats.go:59). format: date-time is enforced. No guard needed in the mapping.
1. The test fixture carries a validity window production would reject
Problem
mappings_test.go:80 is "validity": { "startsAt": "2025-08-20", "endsAt": "2025-08-21" }. Those are dates, not date-times. By the rule you just established, format: date-time fails them with a 400 before the mapping ever runs -- the same 400 you got live for "01-09-2026".
selectRequest is the fixture behind every test in this file, so the whole suite proves behaviour on a payload no caller can send. The helmcharts example gets this right: 2026-09-01T00:00:00+05:30.
Recommendation
Change both ends of selectRequest to RFC3339, and the same in the "validity that ends before it starts" case at line 747. The upstream query assertions stay as they are -- $ddmmyyyy reads the first ten characters either way.
2. A stale claim in the test
Problem
mappings_test.go:229 reads "Nothing was resolved before the call, which is the whole claim of this plugin having no prerequisites." The plugin now has one, registered at prerequisites.go:17.
Recommendation
Change it to say nothing is resolved before the call -- the one entry only logs.
Resolved
- The header's pack-exclusion claims are gone, and the 51 lines are now 14.
districtandstateare the requested codes alongsidemarketCode, so an answer'smarketcan be selected again.mappings_test.goasserts it.- The description no longer claims guards that are not in the code, and says plainly what the schema validator covers.
- Multiple commodities: decision recorded, implemented, and covered by
TestShippedMappingSendsOnlyTheFirstCommodity.
| // first is sent to Agmarknet, which takes one code per call; the rest are | ||
| // dropped, not refused. | ||
| func logExtraCommodities(ctx context.Context, beckn any) (map[string]any, error) { | ||
| path := "message.contract.commitments[].resources[].resourceAttributes.supportedCommodities[]" |
There was a problem hiding this comment.
The warning counts something the mapping does not read
Problem
This path sums supportedCommodities across every commitment and every resource. The mapping only ever reads commitments[0].resources[0] -- see mandi-price.select.yaml:19, :102 and :109.
The two disagree in both directions. A select with two commitments naming one commodity each sums to 2 and logs "names 2 commodities" when neither commitment lost anything. And the case that really does lose data -- a second resource, which the mapping drops whole -- produces no warning at all.
Recommendation
LeavesAt has no index syntax; walk at common/paths.go:115 only handles the [] suffix, so commitments[0] would be looked up as a literal key. Walk to the first commitment's first resource with plain type assertions and count supportedCommodities there, so the number in the log is the number in the request that is actually sent. Warn separately when there is more than one commitment or resource, since those are dropped whole and nothing reports it today.
There was a problem hiding this comment.
Fixed in d3e43ad. The warning now walks to commitments[0].resources[0] with plain type assertions, the same entry the mapping reads, and counts supportedCommodities there. Extra commitments and extra resources get their own warning, since they are dropped whole. Covered by TestDroppedSelectionsCountsWhatTheMappingReads, including the two-commitments case.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Re-reviewed at 91ca538. No new commits since the last round, so the three open findings are unchanged and none has a reply. One new finding below, inline.
1. The state guard is weaker on this branch than on development
Problem
Inline on config/mappings/agmarknet/mandi-price.select.yaml:30.
Still open from 6 Nov, no reply
prerequisites.go:24 -- the warning counts supportedCommodities across every commitment and every resource, while the mapping reads commitments[0].resources[0] only. Thread r4490298102.
mappings_test.go:80 -- "startsAt": "2025-08-20" is a date, not a date-time. By the rule you established in r4490145955, format: date-time rejects it with a 400 before the mapping runs. selectRequest is the fixture behind every test in the file.
mappings_test.go:229 -- still reads "the whole claim of this plugin having no prerequisites". The plugin has one, at prerequisites.go:17.
Merge readinessNot yet. One blocker. The state guard ( Worth doing in the same pass, none blocking on its own:
Let through: the warning count at Order: this PR first, then re-sync the mapping into helmcharts#123 and merge that. |
] Refs OpenAgriNet/helmcharts#123 (review comment r4509945972)
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Re-reviewed at d3e43ad. The blocker is cleared. Two test-hygiene items left, neither affects shipped behaviour.
Resolved
The state guard (62f3d2b). $match($ra.market.state, /^[A-Za-z]{2,4}$/) is back, the message names the shape, and "a state name where the upstream wants a code" covers it. "Chattisgarh" is refused; "CG" passes.
The warning count (d3e43ad). droppedSelections now walks commitments[0].resources[0] with plain type assertions -- the same entry the mapping reads -- and warns separately about dropped commitments, dropped resources and dropped commodities. prerequisites_test.go covers all four shapes including the empty one. I ran the suites: MandiPrice, MandiPrice/cmd and MandiPrice/publish all pass.
Still open
1. The test fixture carries a validity window production rejects
mappings_test.go:80 is still "validity": { "startsAt": "2025-08-20", "endsAt": "2025-08-21" }. The pack reaches TimePeriod through $ref and both ends are format: date-time (AgricultureResource/v0.1/attributes.yaml:181, :186), which kin-openapi enforces -- the rule you established in r4490145955.
selectRequest is the fixture behind every test in the file, so the suite passes on a payload no caller can send. Change both ends to RFC3339, and the same at line 745. The from_date/to_date assertions are unaffected -- $ddmmyyyy reads the first ten characters either way.
2. A stale claim in the test
mappings_test.go:229 still reads "which is the whole claim of this plugin having no prerequisites". The plugin has one, registered at prerequisites.go:18. The package comment above it was updated in d3e43ad; this line was missed.
Approving. Both are a few minutes of work -- please fold them in before merge rather than in a follow-up.
Refs #119 (Security Scan run 42578912940)
Refs #97, #98
Closes #107
Spec: MandiPrice/v0.1 attributes.yaml
Changes
marketCodeis optional. When provided, it is sent upstream and echoed in results. Otherwise, district-wide results are returned with each row's own market. Each row'smarketcarries the requesteddistrictandstatecodes.marketcan be selected again.marketNamewithoutmarketCodeis refused before Agmarknet is called (HTTP 400): sendmarketCodefor that market, or omitmarketNamefor a district search.supportedCommoditiesare not refused: only the first is sent upstream, and the adapter logs a warning with the count.marketCode, date format and price-field values are left to the schema validator.validity.startsAtandvalidity.endsAtare mandatory and validated for correct order.from_dateandto_date; prevents reversed ranges being sent upstream.supportedPriceFieldsare returned; absent selection returns all three.marketName.