Feat/91 agriculture facility - #25
Merged
Merged
Conversation
…evelopment [beckn#91] PR #22's work, replayed onto development and adapted to it. Squashed from 22 commits: 12 of them modified internal/upstream/upstream.go, which development split into seven files, so each would have conflicted as a modify/delete. The net change to the shared package across all 22 was one exported function; the rest was churn -- fan-out added then removed, BindingPaths renamed three times. WHAT THE ADAPTATION NEEDED. development has since renamed internal/upstream to internal/common, folded capabilitybinding into it, and made auth per provider: internal/upstream -> internal/common capabilitybinding.From -> common.BindingFrom, exported for this plugin bindingPaths -> common.BindingPaths, exported for this plugin Config's flat auth fields -> Config.AuthByProvider, one profile per provider authScheme: none -> a pocra: block, nested under the participant id Two of those are exports the plugin needs and this branch adds, with the reason recorded at each: it answers "is this payload mine?" for a multi-type search, and it must answer it the same way serve does rather than reading the payload a second way. The config entry and the steps list gain AgricultureFacility alongside the three that were already there; build-plugins.sh gains it too, rather than replacing KnowledgeAdvisory as the original diff did -- that branch predated the knowledge plugin. Their tests needed the same adaptation. Every helper that builds a Config now declares pocra's auth, since a served provider without a block is refused at startup -- which is the new validation working, not a defect. 69 packages green, vet clean, no races. The other three capabilities were run separately to confirm the two new exports changed nothing for them. Original commits are preserved on feat/91-AgricultualFacility(Plugin) and on PR #22, which is untouched.
…y hand [beckn#91] facilityTypesFrom walked the document in Go -- dig(message, contract, commitments), then [0], then resources, then [0], then resourceAttributes -- with a type assertion and an error message at each level. That compiles the payload's shape into the binary: a spec change needs a rebuild, and eleven lines re-check what a walker already checks. The shape is the network's convention, not this adapter's, so it is now a PATH, defaulting to the Beckn v2 location and overridable per deployment -- exactly as common.Config's providerIdAt and capabilityCodeAt already are: facilityTypesAt: message.contract.commitments[].resources[]. resourceAttributes.supportedFacilityTypes[] The reading is common's walker, the same one that finds a binding key, so the traversal and its absent-field handling live in one place. A test proves the point: a payload carrying the types at search.facilities[].kinds[] is read correctly with nothing but a config path. WHAT THIS NEARLY BROKE, caught by their own tests. ValuesAt drops a non-string leaf, which is right for a binding key and wrong here: supportedFacilityTypes ["KrishiVigyanKendra", 42] would have searched for one type and reported success, a partial answer with nothing recording the loss -- the same class of bug the fan-out work existed to fix. common now exposes LeavesAt alongside ValuesAt: one walker, two views, type policy left to the caller. The plugin takes the leaves and refuses a non-string with the message it always used. An empty supportedFacilityTypes needed care too: it reads as zero leaves like an absent one, and the bare-string fallback would then hand back the empty list itself, reported as "[] is not a facility type" -- true and useless. A leaf that is itself a list is skipped, so an empty list keeps the "names no facility type" message. common's own tests pass untouched, so binding-key extraction is unchanged. 69 packages green, vet clean, no races. dig() remains for now: search.go still uses it to WRITE into a payload when splitting by type, which is a different primitive from reading and not something common offers.
…beckn#91] The plugin stamped a fresh UUID on every part it split out, so POCRA would not blend the answers. That put the reason in Go and the consequence in the mapping, which then had a comment explaining what the Go code had done for it. The mapping derives the id itself now, from the caller's messageId with its last hex digit replaced by a slot the facility type owns. The TYPE picks the slot rather than a call index, because every payload reaching the request half names exactly one type -- so no index has to be passed, and a retry reuses its own slot instead of taking a fresh one. WHY THE CALLER'S OWN ID CANNOT BE SENT, verified live against POCRA: its cache is keyed on message_id, holds for PT10M, and IGNORES LOCATION. The same id asked twice 350km apart returned the first location's facility alongside the second's, which no type filter can catch because the type matches. Reusing it would answer a farmer in Nagpur with a facility near Ahmednagar. Replacing the last character leaves a valid v4 UUID -- it sits in the node field, not the version or variant nibbles -- which POCRA's schema requires. Derived rather than random because JSONata here has no $uuid. That deletes, in order: the UUID stamping, the merged answer's messageId restore (parts never lose the caller's id now), messageIDOf, the uuid import, and dig -- the last hand-written traversal in this package's production code. splitByType writes through the SAME configured path the types were read from, so the read and the write cannot disagree about where they live. setAt and containersAt are local to this package, not added to common: common's step reads payloads and never rewrites one, and a write raises questions a read does not. They create nothing, so a path that does not resolve is a refusal rather than a payload invented to fit. Their own TestEachSearchCallCarriesItsOwnRequestId already asserted the exact property this change has to keep -- two calls, distinct ids, both valid UUIDs, neither the caller's. Negative control: giving two types the same slot fails it with "both calls used message_id ... so POCRA would blend their answers". 69 packages green, vet clean, no races.
… package [beckn#91] internal/concurrent was 116 lines of generic bounded fail-fast machinery plus 288 lines of tests to run one loop, with a single caller, at a configured concurrency of 1. golang.org/x/sync is already a dependency and errgroup does the same job here in a method that reads alongside the code it serves. The clamp moves into searchConcurrency and gains a reason: SetLimit reads a non-positive limit as UNBOUNDED, so a misread setting would fan out over every facility type at once against a provider that rate-limits into silent empty answers. concurrent.Bound treated zero the same way; the comment now says why it matters at the point where it is enforced. Order is kept by indexing the answers rather than appending, so each slot is written by exactly one goroutine and the merge does not depend on which call finished first. No behaviour change: the five search tests that pin sequential-by-default, the configured limit, fail-fast skipping calls not yet issued, one-type-one-call and the ceiling all pass unmodified, under -race.
…kn#91] Both lines were added by the facility plugin's branch and ignore nothing: no docs/ or dev_docs/ directory exists and neither has a tracked file. /docs/ is worse than unnecessary -- it is the conventional place for documentation, so the first person to add any there would find it silently uncommittable, with a rule in an unrelated feature branch to explain it. testdata/.gitignore stays. It ignores schema-cache/, which does exist, and is how schemacache_test.go keeps the fetched schema pack out of the repo instead of vendoring a copy.
|
📊 Test Coverage: ✅ Passed — 87% of changed lines covered, min 80% |
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependenciesNo findings at CRITICAL,HIGH,MEDIUM,LOW. Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
…heir own [beckn#91] For an upstream that mints short-lived tokens from its own endpoint rather than an OAuth2 one. The live Agmarknet Vistaar API is the case: POST a JSON body of credentials, get {"token":"<uuid>"} back, send it as a QUERY PARAMETER. Neither existing scheme can serve that, and bending one would have been wrong in five separate ways: oauth2 posts form-encoded client_id/client_secret, reads access_token and expires_in, sets Authorization: Bearer tokenQuery posts JSON under CONFIGURED keys, reads a CONFIGURED key, sets a query parameter -- and the response carries NO expiry The field names are configured rather than fixed because access_name and password are one provider's spelling, not a standard. WHY tokenTtl IS REQUIRED. The response says nothing about how long the token lives, so there is nothing to read and guessing would be inventing a lifetime. An operator states what they believe instead. A ttl at or below the refresh skew is refused: every token would already be expired on arrival, making two round trips per request and hitting the token endpoint at the request rate. AND WHY A WRONG tokenTtl IS NOT AN OUTAGE. Because the lifetime is an estimate, a too-generous one leaves a dead token cached -- and without help every call would fail until it lapsed. A 401 or 403 from the provider now drops the held token, so the next call exchanges a fresh one. One request pays; the next recovers. This applies to oauth2 too, where an issuer can revoke a token before its expires_in runs out. REDACTION follows the placement, not the scheme it borrows from. A query string is the exposed spot -- proxies log it and Go's transport errors quote the whole URL -- so the token's URL-ESCAPED form is covered as well, or one containing + or = survives redaction. The identifier is left readable, as a client id is: it identifies, it does not authenticate. The HTTP half of the exchange is now shared by both schemes in postForToken. The interesting part there is the retry decision -- unreachable, unreadable, 5xx and 429 retry; a 4xx is configuration -- and two copies of that would drift. VERIFIED AGAINST THE LIVE PROVIDER, not just in unit tests: exchanged a real 36-character UUID from the actual token endpoint, placed it as ?token=<uuid>, and confirmed the log line reads token=REDACTED. That check read the endpoint and credentials from the environment and is not committed -- no IP and no credential is written down anywhere. Twenty test cases, and the self-heal one carries a negative control: with the forgetToken call removed it fails with "the rejected token was reused instead of re-exchanged", so it is testing the behaviour rather than passing by accident. Full suite green under -race.
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
2 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.
feat(AgricultureFacility): serve openagrinet:AgricultureFacility via POCRA [beckn#91]
New provider step plugin answering
pocra|openagrinet:AgricultureFacilityforselect. PR #22's work replayed onto currentdevelopment(that branch predatedthe
internal/upstream→internal/commonsplit, so every commit would haveconflicted as a modify/delete).
Why it needs its own step
Three POCRA facts, all verified live, none of them documented:
200with no providers, an array is refused. A payload naming three facility types
must become three calls.
message_idfor PT10M and returns the union of everythingasked under it, so two calls sharing an id each return the other's facilities.
the first location's facility alongside the second's. So the caller's own id
cannot be sent as it stands.
search.gosplits one payload into one single-type payload per type, runs theordinary one-call step over each, and merges. Fail-fast: one failed call fails
the request, because a partial answer is the defect this exists to fix.
The id POCRA sees is derived in the mapping — the caller's id with its last hex
digit replaced by a slot the type owns. That character is in the UUID's node
field, so the result still validates. Each part keeps the caller's own
messageId.searchConcurrencydefaults to 1, capped at 8. The cap matters:SetLimitreadsnon-positive as unbounded.
Reviewer attention:
internal/commonOn every capability's path, so read this part closely. Two renames to exported
(
BindingFrom,BindingPaths) — the plugin must answer "is this payload mine?"the same way
servedoes, not by reading the same config twice. Plus one walker,two views:
ValuesAtkeeps strings (right for a binding key), newLeavesAtkeeps every leaf (right when a non-string is a mistake to report, not a leaf to
skip).
internal/common's own tests pass untouched; the other three capabilitieswere run separately.
Config