feat(AgricultureFacility): add plugin for POCRA governed-facility sea… - #22
Closed
kelvinprabhu wants to merge 26 commits into
Closed
feat(AgricultureFacility): add plugin for POCRA governed-facility sea…#22kelvinprabhu wants to merge 26 commits into
kelvinprabhu wants to merge 26 commits into
Conversation
…rch [beckn#91] New plugin for the AgricultureFacility capability (KVK, custom hiring centres, soil-testing labs, warehouses) backed by POCRA, plus new support in internal/upstream and jsonmapper for splitting one Beckn payload into several upstream calls. - internal/upstream: add a "fan-out" mode (gather/fanOut/one) — a payload naming several facility types becomes one upstream call per type, run sequentially by default (FanOutConcurrency, capped by MaxFanOut = 8) since POCRA answers an overloaded search with 200 + empty catalog rather than an error. Each fan-out call gets its own UUID so POCRA's 10-minute message_id cache can't blend calls together. - pkg/plugin/definition/mapper.go: add DirectionFanOut alongside Request/Response. - jsonmapper: compile and serve the new fanOut mapping half. - config/provider-adapter.yaml: register the AgricultureFacility step (auth: none, fanOutConcurrency: 5) and wire it into providerSteps. - install/build-plugins.sh: build the new plugin. - config/mappings/pocra/: fan-out/request/response mapping for POCRA. - pkg/plugin/implementation/AgricultureFacility/: plugin, cmd entrypoint, prerequisites, conformance/live/mapping/schema-cache tests, README, and cached schema-pack testdata.
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependenciesNo findings at CRITICAL,HIGH,MEDIUM,LOW. Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
|
📊 Test Coverage: ✅ Passed — 89% of changed lines covered, min 80% |
manjudr
reviewed
Sep 8, 2026
manjudr
left a comment
manjudr
left a comment
Member
There was a problem hiding this comment.
Review
Reviewed against 71d0a56 on a local clone, base feat/41-oan-adapter-plugins. go vet clean; the full suite passes, including -race on internal/upstream. Every finding below is reproduced with a throwaway test rather than read off the diff.
Would block on two:
fanOutConcurrencyis dead config —cmd/plugin.go:parseConfignever reads the key, soprovider-adapter.yaml'sfanOutConcurrency: 5and the README row are both no-ops and the step always runs sequentially. Measured 1.21 s vs 310 ms for 4 × 300 ms calls. TheparseConfigtest case named "reads every supported setting" is the one that omits it.- A duplicated item tag fails the entire search with T0410 — three unguarded reads (
category,distance,capacity_estimate) in the same class the mapping explicitly fixes for fulfillment categories one line away. One malformed item → 502, every good facility included.
Also raised: no fail-fast or aggregate deadline across the fan-out (up to ~6 min of upstream work for an already-failed request, using this plugin's own documented registry row); duplicate facility types producing duplicate upstream calls; and four dead artefacts — the ignored yaml key, _fan.index (bound, read by nothing), and two references to docs/ files that do not exist in the repo.
The fan-out design itself reads well: the ordered-results guarantee, the per-call UUID against POCRA's 10-minute message_id union, and refusing rather than clamping MaxFanOut are all the right calls, and each is argued for in the comment where it lives.
Two were serving wrong data or crashing: fanOutConcurrency was declared but never parsed, so fan-out always ran sequentially; a duplicated item tag (category/distance/capacity_estimate) threw T0410 and failed the whole search, good facilities included. - plugin.go: wire fanOutConcurrency into Config - select.yaml: guard tag lookups against duplicate tags; refuse duplicate facility types; dedupe $requestedTypes path; bind $reading/$numericPrefix once instead of twice; fix @context comment overclaim - upstream.go: gather() now fails fast via errgroup.WithContext instead of letting every fan-out call run to completion after one has failed; drop dead _fan.index - provider-adapter.yaml: fanOutConcurrency 5 -> 4, matching its own comment - README.md, mappings_test.go: drop/reword dangling docs/ references
…anch CI cold-fetches the AgricultureFacility schema pack on every run; local runs stay green off a stale gitignored cache. schemacache_test.go pinned the fetch URL to the schema-packs-v0.1 branch NAME, which raw.githubusercontent.com serves as "whatever that branch's tip currently is" -- not a pin at all.
…, keep tracking the branch network-specs commit b76c9ad8a5 on schema-packs-v0.1 quietly dropped the `not: anyOf: [...]` clause that made an OnDemand AgricultureFacility resource reject facilityType/location/address/services/capacity/publicContact/website/ source/lastUpdatedAt. Still live on the branch tip as of this writing. Chose to keep tracking the branch (not pin to a known-good commit) and adapt this repo to what it currently says, rather than wait on an upstream fix -- see dev_docs/schema-onDemand-forbid-removed.md for the full writeup. No functional change: the runtime schemaValidator never evaluated this if/then rule anyway (provider-adapter.yaml says so explicitly), and this plugin never populated those fields on an OnDemand resource regardless. - conformance_test.go: drop the now-false forbidden-fields assertion from TestTheRequestResourceSatisfiesOnDemandMode; keep the positive OnDemand validity check - select.yaml, README.md: stop claiming the pack forbids these fields; state the fulfillment-stop convention as this plugin's own choice - schemacache_test.go: packBase stays pinned to the branch, not a commit -- a test failure here is the intended signal when upstream regresses, not something to route around
ameersohel45
reviewed
Sep 9, 2026
ameersohel45
left a comment
ameersohel45
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the AgricultureFacility plugin and the fan-out addition to internal/upstream.
The fan-out design is careful in the places that matter: sequential by default with the reason written down, a ceiling that refuses rather than truncates, sibling cancellation on first failure, a fresh UUID per call so POCRA's ten-minute message_id accumulation cannot blend the answers, and all-or-nothing results because a partial answer is the defect fan-out was built to fix. Loop-variable capture in the goroutine is safe on go 1.26.
conformance_test.go is the strongest part — validating against the commit-pinned published pack with a real validator, and then testing the validator itself (TestTheValidatorRefusesAnIncompleteFacility, TestThePacksOwnExamplesValidate) so a green run cannot be vacuous. The distance and category fixes both carry the reasoning rather than just the patch, including the subtle bit that $exists($number(...)) cannot guard a throw because the argument evaluates first.
One change I'd ask for before merge, plus two optional items below.
| // | ||
| // A mapping declaring no fan-out is called once, which is every provider | ||
| // that can answer a whole payload in one exchange. | ||
| DirectionFanOut Direction = "fanOut" |
Collaborator
There was a problem hiding this comment.
Worth stating here that fan-out changes what response holds in the response half: a single upstream body without fan-out, an array of bodies with it.
The POCRA mapping is unaffected because JSONata path navigation flattens over arrays, so response.responses.message.catalog.providers reads the same either way. But a mapping written with $count(response.hits) or an index would behave differently between the two modes, with no error — and the next mapping author has no way to know that from the direction's doc comment alone.
One sentence here would cover it. No code change.
…rrency and docs Drop fanOutConcurrency to 1 until POCRA's silent-empty-catalog failure mode under concurrency is verified live, and fix the comment's stated value (4) to match. Document that DirectionFanOut changes response's shape (single body vs array), so mapping authors don't assume it stays a bare object. Confirmed no data race on the fan-out gather path via go test -race.
…ersions golang.org/x/net v0.57.0 -> v0.58.0, google.golang.org/grpc v1.83.1 -> v1.83.2.
…c and related comments
…meters and remove fan-out tests
…concurrent.Run
Fan-out concurrency was internal/upstream's, which made POCRA's facts about
one provider the default for every capability that package serves. Now the
policy lives in the plugin that has the provider, and upstream only asks.
- fanout.go declares OneCall and Gather -- the names for the fan-out hook,
here because "gather" is a plugin-specific idea. OneCall must stay an
alias, or Gather stops being assignable to upstream.NewWithFanOut's
unnamed parameter.
- gatherFacilities carries what is true about POCRA and nothing else:
MaxFanOut is 8 because the provider is slow, the default concurrency is 1
because its failure mode under load is a 200 with an empty catalog, and
each call needs a fresh UUID because it blends answers sharing a
message_id. The bounded, ordered, fail-fast execution is
internal/concurrent.Run, which this is a thin caller of.
- Config is this package's own flat struct now, carrying fanOutConcurrency
that upstream.Config no longer has. Flat rather than embedded so every
existing &Config{BindingKeys: ...} literal, cmd/plugin.go's included,
keeps compiling.
- fanout_test.go covers the policy that moved: sequential by default, the
configured concurrency honoured, later values skipped once one fails, and
a payload over the ceiling refused.
… and enhance applyFanOutDefaults logic
…e pack AgricultureResource v0.1 made subjectCategories required (it was optional), added "Facility" to its enum, and AgricultureFacility now requires the list to contain "Facility". Every resource this plugin emits became invalid, and so did the request fixture the suite is built on. The suite tracks the schema-packs-v0.1 branch and caches it under a gitignored testdata dir, so this lands only on a cold cache -- CI always is, a developer's machine usually is not. - the mapping states subjectCategories: ["Facility"] on every facility it answers with. "Facility" alone: the pack permits further values naming the domains a facility serves, and POCRA states nothing about that, so any addition would be a guess published as a governed discovery category. - the request fixture carries it too, so the OnDemand query resource is a valid AgricultureFacility again. - schemacache_test.go's comment predicted a cold-cache failure that no longer happens, for a test that no longer asserts it. Rewritten to name the two shapes an upstream change takes and what each one asks of us. MandiPrice and WeatherObservation already state ["Market"] and ["Weather"], so this capability was the one out of step.
… it out of upstream and the mapper
- Removed FanOut handling from jsonmapper and adjusted related logic in Transform and fetchAndCompile methods. - Introduced payload.go to read facility types directly from Beckn payloads, ensuring proper handling of single and multiple facility types. - Added comprehensive tests for facility type extraction and validation in payload_test.go. - Implemented search.go to manage splitting of multi-type searches into individual requests, ensuring compliance with POCRA's API constraints. - Enhanced error handling for various malformed payload scenarios.
…e error reporting
…consistency and improve error handling
ameersohel45
added a commit
that referenced
this pull request
Sep 9, 2026
16 files, 204K of cached schema documents under pkg/plugin/implementation/AgricultureFacility/testdata/schema-cache/. They belong to PR #22, which is somebody else's work and not merged. They were sitting untracked in the checkout when I ran `git add -A` for the internal/ rename in 53b4bd7, and that swept them in. Not present on feat/7-knowledge-advisory or on development, so this branch introduced them. Nothing references them: the only matches for the name are inside the files themselves. 65 packages green after removing them. The mistake was `git add -A` on a path I did not fully own. Every commit since has staged named paths instead.
…currency comments
Collaborator
|
Not needed - Already done in other PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[beckn#91]
New plugin for the AgricultureFacility capability (KVK, custom hiring centres, soil-testing labs, warehouses) backed by POCRA, plus new support in internal/upstream and jsonmapper for splitting one Beckn payload into several upstream calls.