Conversation
🛡️ 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 — 84% of changed lines covered, min 80% |
ameersohel45
left a comment
ameersohel45
left a comment
There was a problem hiding this comment.
Checked out the branch, built it, ran it. The transport half is solid and several of the decisions are ones I'd have got wrong: one deadline for the whole fan-out rather than per target, the degraded header carrying a count instead of hostnames, refusing offset because it means nothing across unordered sets, signing once over the merged body. The httpClientConfig.timeout diagnosis is right too — http.Client.Timeout really is ignored by ReverseProxy.
Two things I verified rather than assumed, both in your favour. The header hygiene in callTarget is correct: the list matches the stdlib's hopHeaders exactly plus Content-Length, and X-Forwarded-Host is set after the stripping, so the Connection:-header attack the stdlib warns about doesn't apply. And the timeout commit changes nothing live — none of the deployed adapter configs set httpClientConfig, so Timeout is 0 and the wrapper never engages.
The main thing
A routing rule says where to send a request. Here, a second entry in urls: also decides how replies are recombined — that the collection is at message.catalogs, that identity is .id, that ordering is round-robin, that the envelope keeps only context and message, that the first responder donates the context, and that paging is limit/offset. All six are in Go, and a deployment can change none of them. mergeCatalogs refuses anything else and there's a test pinning it, so urls: on a non-catalog action NACKs.
What we were after is the general capability: the adapter can call several endpoints and merge what comes back. What's here is that plus on_discover compiled in. The gap is one config block on the rule, next to urls::
merge:
collection: "message.catalogs" # the extraction point
identity: "id" # what makes two entries the same
order: "roundRobin"
limitParam: "limit"
rejectParams: ["offset"]Per-rule rather than per-module, since rules are already keyed by endpoints and the extraction point depends on the action. The router resolves it into Route.Merge the same way it already resolves Route.URLs. Then a reply carrying message.providers is a config edit, not a Go change — the principle the capability plugins already follow.
Raising this before line comments, because it decides whether mergeCatalogs, catalogsOf, catalogID and interleave get rewritten or deleted.
Two ways to restructure
A — keep fanout.go, move only the merge out. Those five functions (~150 lines) go to a new pkg/merge, leaving an executor that knows nothing about catalogs. fanout.go earns its place: stdHandler.go is already 908 lines, and the executor touches unexported handler internals (ackSignerStep, sendNack, isAckSigner reaching into InstrumentedResponseStep.step), so it can't become its own package without exporting handler's guts for a file move.
B — no fanout.go; extend proxy(). ReverseProxy.ServeHTTP does outreq := req.Clone(ctx), so it never mutates the request you give it and N can run concurrently. Point each at a small buffered ResponseWriter instead of the real w, collect, merge, write once — len(ctx.Route.URLs) <= 1 keeps today's path byte-for-byte. That deletes callTarget, callTargets and most of collect for a ~25-line buffer plus a goroutine loop, stops maintaining a second copy of header hygiene, and reuses modifyResponse so response steps aren't run by two different code paths. Three things to handle: r.Clone() shares the body, so each target needs its own reader over ctx.Body; write the buffer rather than importing httptest; and give each target its own ErrorHandler closure, or a rejected response step looks identical to a transport failure.
I lean B — less code, and less of it security-sensitive — but A is the smaller step and I don't feel strongly.
Blocking, either way
Every target failing returns HTTP 500 NET_INTERNAL_ERROR. Probed it against your own test helpers:
status=500 body={"error":{"code":"NET_INTERNAL_ERROR","message":"Internal server error..."}}
fmt.Errorf("no target answered: ...") is a bare error, so nackBecknError falls to default:. The caller is told this adapter is broken when the networks are unreachable. NewCodedErr(http.StatusBadGateway, CodeUpstreamUnavailable, ...) is the convention, and fanoutQuery already reaches for NewBadReqErr correctly. TestFanoutEveryTargetFailsReturnsNack asserts only != 200, which 500 satisfies as happily as 502 — worth tightening.
One trap in fixing it: CodedErr.BecknError() puts prefix + e.Error() on the wire, and that message joins degraded — the hostnames. Wrapping it as-is would publish the topology the count-not-names decision exists to hide. The count is enough; the hostnames are already in the per-target logs.
Smaller
io.ReadAll per target is uncapped — to be fair proxy()'s modifyResponse already does this, so it isn't introduced here, but fan-out holds up to maxConcurrency of them at once. internal/common/http.go has the pattern, and on option B the buffer is the natural place for it.
newHTTPClient sets both http.Client{Timeout} and the timeoutTransport wrapper at the same duration — since Client.Timeout is the one that doesn't reach ReverseProxy, the field reads as redundant.
X-Forwarded-For isn't appended where the stdlib appends it, and Te is stripped unconditionally where the stdlib keeps Te: trailers.
Whose context wins is worth deciding rather than defaulting: today the merged reply names whichever network answered first, so it claims a sender it isn't.
And urls: with several entries and no merge: block should be refused at startup — a fan-out with nowhere to extract from can't do anything useful, and failing at load beats NACKing per request.
|
1.3.3-r0 doesn't exists and it appears to be false information |
ameersohel45
left a comment
ameersohel45
left a comment
There was a problem hiding this comment.
Re-reviewed at f4e1929. Splitting this in two — architecture here, functionality separately. Everything from the last round is addressed: the merge is config-driven, all-targets-down is a 502 with a coded error, the wire carries a count and the log carries the hosts, and the per-target read is capped. The sample routing config is a good addition, and it's honest about what the loader doesn't enforce, which is the kind of thing that usually goes unsaid.
Six architectural notes, roughly in the order I'd act on them.
1. The router now validates a response envelope
if rule.MergeFieldPath != "message" && !strings.HasPrefix(rule.MergeFieldPath, "message.") {
return fmt.Errorf("... must start with \"message.\" -- it names a path under the response's message ...")
}A router resolves a destination from an action. It now also knows that replies have a message member and enforces it. The check itself is right — a typo'd path degrades every target and surfaces as a misleading "no target could be reached", so catching it at load time is worth doing. It's the placement that's inverted: the rule belongs wherever the merge is defined, with the router carrying the value opaquely.
This is the last of the original coupling. Making the merge point configurable was the important fix; it's still configured in the routing rule and now validated by the routing plugin.
2. model.Route carries three invariants and enforces none
URL is always set even for a fan-out, URLs only when there's more than one, MergeFieldPath only alongside URLs. All three live in comments, and consumers have to know to branch on len(URLs) > 1.
The type is also copied field by field in addRouteStep, which is how MergeFieldPath got silently dropped in 78d8c0d — caught by manual testing rather than a test. That isn't a one-off; it's a shape that regenerates the same bug on the next field. A struct copy, a constructor, or a small value holding (urls, mergeFieldPath) closes the class.
3. merge.go has no dependency on handler
package handler
import ("encoding/json"; "fmt"; "strings")Three stdlib imports, nothing else — and merge_test.go exercises all of it without starting a single test server. So it is already a standalone package in everything but location.
The executor genuinely belongs in handler: it touches ackSignerStep, sendNack, writeJSONResponse, none of them exported. The merge touches none of them. Leaving it here puts payload shaping in core, which is the thing this codebase otherwise keeps in mappings and plugins. Cheap to move now; a rename across a dozen test files later.
4. The duplicated error code is a package-layout symptom
// codeAllTargetsUnreachable mirrors util.CodeUpstreamUnavailable, which this
// package cannot import (internal to plugin/implementation).
const codeAllTargetsUnreachable = "NET_DOWNSTREAM_UNAVAILABLE"The workaround is correct and honestly labelled, but the cause is that a protocol-level vocabulary lives in plugin/implementation/internal/..., where core can't reach it. Two packages now hold the same string with nothing keeping them in step. It belongs in pkg/model beside the other Beckn codes.
5. Fan-out config lives in two homes
Concurrency and timeout come from the handler config; the merge path comes from the routing rule. That's defensible — one is operational, the other is per-action semantics — but right now it reads as wherever each was easiest to add. One sentence in the sample config saying which knob lives where would settle it.
6. Merge policy is half-configurable
The extraction point is config; ordering and donor selection are hardcoded. Previously all of it was hardcoded, which was at least consistent. I think the extraction point genuinely is the only part that varies per deployment — that's worth stating rather than leaving as an asymmetry someone later tries to "finish".
What's right, and worth keeping as-is: the executor/merge seam is the correct split, and the vocabulary follows it — itemsOf, mergeResponses, arrayAtPath/setAtPath carry no catalog language. The test architecture mirrors the code, with pure functions tested purely and the executor tested through real servers. And route() gaining a third arm is the right dispatch point, with the single-target path genuinely untouched.
If only some of these are worth acting on: 1 and 2 are the ones that get more expensive with time — one is a layering inversion the next feature will copy, the other regenerates a real bug on every field added. 3 is cheap now and near-free to defer. 4 is a one-line move.
Functionality review to follow separately.
There was a problem hiding this comment.
Suggestion on how fan-out is selected.
Today the routing rule says targetType: url and fan-out is inferred from how many entries urls: happens to have:
case "url":
if len(ctx.Route.URLs) > 1 {
fanoutFunc(...)The discriminator is real, it's just hidden in a cardinality check. It reads better as its own target type — plural of the one that already exists, so there is nothing to explain:
Before
- version: "2.0.0"
targetType: "url"
target:
urls:
- "http://network-adapter-bharat-vistar:9201"
- "http://network-adapter-maha-vistar:9201"
mergeFieldPath: message.catalogs
endpoints:
- discoverAfter
- version: "2.0.0"
targetType: "urls" # url -> urls
target:
urls:
- "http://network-adapter-bharat-vistar:9201"
- "http://network-adapter-maha-vistar:9201"
mergeFieldPath: message.catalogs
endpoints:
- discoverOne word. targetType: url pairs with target.url, targetType: urls pairs with target.urls.
The handler then dispatches on the type, like it does for everything else:
case "url": proxyFunc(...)
case "urls": fanoutFunc(...)
case "publisher": ...What it fixes
urls:undertargetType: bppis silently ignored today.loadRules' bpp branch never readsurlList(), andvalidateRulesonly checksTarget.URLthere — so that config parses, fans out to nothing, and reports no error. Withurlsbelonging to its own type, the loader rejects it.URLstops being set on fan-out routes just so "every existing reader of it keeps working untouched". A fan-out has no single URL; naming one of its targets as the URL reads fine now and misleads later.- Validation gets its own case instead of conditionals nested inside
url— including theelse if rule.MergeFieldPath != ""that exists only to reject a merge path on a single-target rule. - The three invariants in comments on
model.Route—URLalways set,URLsonly when >1,MergeFieldPathonly alongsideURLs— collapse into one type check.
Cost
Three places guard on route.TargetType == targetTypeURL (router.go:343, :383, and the bodyless path) and need to accept both — miss one and the caller's query stops reaching targets.
And it is a config change, so it is cheap now and awkward after a release.
A routing rule could name one target, because Route held one URL. Serving an action from several networks needs the rule to name them all, so target gains a "urls" list alongside "url" and Route carries every parsed target. The list having more than one entry is the whole switch. There is deliberately no enabled flag: a second URL and a flag saying to use it are two things that have to be kept in agreement, and a list cannot disagree with itself. A rule with one target leaves URLs nil and keeps URL set, so every existing reader and the reverse-proxy path are untouched. Two fixes fall out of the query-string handling. The clone now covers every target -- the stored route is shared by every request matching the rule, so writing RawQuery in place would leak one caller's query onto the next. And the inbound query is merged over whatever the target was configured with rather than replacing it, so a target written as "http://maha:9201?network=maha" keeps its own parameter instead of losing it on any request that arrived with a query. Nothing calls this yet: routing still forwards to the first target only.
httpClientConfig set MaxIdleConns, IdleConnTimeout and ResponseHeaderTimeout, but never a timeout on the round trip itself. ResponseHeaderTimeout bounds only the wait for response headers, so an upstream that answers promptly and then stalls mid-body was never cut off and held its connection and goroutine for as long as it cared to. Setting http.Client.Timeout is not enough on its own, and that is the part worth knowing: the forwarding path does not use the Client. proxy() builds an httputil.ReverseProxy over httpClient.Transport, which never sees a Client field -- so a Client-level bound would have covered only the fan-out path while the documentation claimed it covered everything. The same bound is therefore also applied as a RoundTripper, outermost so it covers the transport wrapper plugin as well. The deadline is released when the response body is closed rather than when RoundTrip returns. RoundTrip returns as soon as the headers are read, which is before the stall this exists to cut off. Left at zero the behaviour is unchanged: no timeout, as before.
…logs [#49] A rule naming several targets now calls them all in parallel and answers with one merged on_discover. The reverse proxy cannot do this -- its contract is that one ResponseWriter becomes one upstream's response, so it streams the body straight through and has nowhere to hold a second. The fan-out path therefore stops proxying and starts calling, which means the work ReverseProxy did silently is done explicitly: hop-by-hop stripping, including the headers named by the request's own Connection header, X-Forwarded-Host, and the host rewrite. Every target gets the same already-validated ctx.Body, each through its own reader over the same bytes. r.Body would not do: it can be read once, so the first target would drain it and the rest would send empty payloads that look valid and return nothing. The signature needs no per-target work for the same reason -- signStep covers the body and names no recipient, so one signature is valid everywhere. One budget bounds the fan-out as a whole rather than each target, because with a concurrency cap the targets run in waves and a per-target bound multiplies by the number of waves. When it expires the caller gets whatever arrived. Partial success is success. A target that fails, answers non-2xx, misses the budget or returns a body that cannot be read is counted in X-Beckn-Degraded and the rest still answer; only a total failure is a NACK, because answering 200 with an empty catalog list when nothing was reached is indistinguishable from networks that genuinely have no matches. The header carries a count and not the addresses: this is the adapter a deployment may put an Ingress in front of, and naming the targets would teach any caller the cluster's topology from one partial failure. Catalogs are interleaved round-robin, not concatenated, so a limit does not hand the whole page to whichever network is first in the config. They are deduped by id, and carried as raw JSON so a member this build does not know survives. The envelope comes from the first response that actually carried catalogs, and only its context travels. A response can be 200 while carrying a Beckn error envelope, and it must not donate that error to a merged answer. If NO response carried catalogs the merge is refused rather than inventing one: that means the rule has been pointed at several targets for an action whose replies cannot be merged, and writing an empty catalogs array into, say, an ACK would produce a body that fails its own schema and then be signed over those bytes. The ack signer runs once over the merged body instead of once per response. It signs our answer, not an upstream's, and on this path its per-response header write lands in an upstream response header that a fan-out never copies out -- the merged answer would have gone to the caller unsigned, over bytes nobody sent. offset is refused on a fan-out rule and stripped from what the targets are sent: forwarded as-is it means "skip N in EACH network", which is not page two of anything. limit is forwarded and applied again to the merged list, so a caller asking for twenty gets twenty rather than twenty per network.
…ogic out [#49] A routing rule's mergeFieldPath now names the full dot path from the envelope root to the array a multi-target rule merges -- message.catalogs for discover, message.contract.commitments for select -- instead of the code assuming discover's shape. Required whenever a rule names more than one target; refused at load if missing. The merge logic (donor selection, dedupe, interleave, path read/write) moves into its own file, unexported, still in this package: it has no dependency on anything fanout.go's executor needs from the handler package, so splitting it shrinks fanout.go without exporting handler internals to a new package. Also folds in outstanding PR review fixes: a total fan-out failure NACKs 502 (not the 500 an unclassified error fell through to), without putting target hostnames on the wire; each target's response read is capped; X-Forwarded-For and Te: trailers now match what the single-target proxy path gets for free from stdlib; Client.Timeout is no longer set alongside the transport-level timeout wrapper, since the wrapper alone already bounds both the proxy and fan-out paths.
…stions [#49] Adds coverage for: a merge path reaching a nested field two levels under message (select's contract.commitments, not just discover's catalogs); every target answering with a present-but-empty array (a valid zero-match answer, not the no-target-carried-it NACK); MaxConcurrency actually bounding in-flight calls, not just being read and forgotten; the ack signer recognized through its production telemetry wrapper, not only the bare type every other signing test used; more targets than the default concurrency cap; a target that stalls mid-body past the deadline; and duplicate target URLs in one rule. mockSigner gains a call counter (signAckCalls) -- the wrapped-ack-signer regression above is invisible in the final response, since the correct merged-body signing runs after and overwrites it; only a call count catches the extra, wasted per-target signing a broken unwrap would cause.
…e-step rejection [#49] mergeFieldPath was only checked for presence, not shape: a typo like mergeFieldPath: catalogs (missing the message. prefix) found nothing on every target and failed at request time as a confusing "no response carried catalogs" NACK, instead of a clear error when the config loads. Only message varies by action -- context is fixed envelope shape -- so a path rooted anywhere else is rejected at validateRules. Also covers a branch every existing fan-out test skipped: a configured response step other than the ack signer (validateAckSign, in production) rejecting one target's answer. That target must degrade without denying the caller the other's -- untested until now.
addRouteStep rebuilt ctx.Route field-by-field from the router's route and dropped MergeFieldPath, so every fan-out request reached the handler with an empty merge path even though the router set it correctly -- caught during manual end-to-end testing of the discover fan-out.
…ling [#49] None of this was ever part of the discover contract this branch fans out -- offset rejection, limit re-truncation after merging, and id-based dedupe were all speculative additions with no caller exercising them. Removed all three: the merged response is now exactly the union of every target's items, in interleaved order, nothing added or dropped. Mutation-verified two new regression tests (offset no longer rejected, limit no longer truncates) by reintroducing the old logic and confirming each fails, then reverting.
No worked example of the multi-endpoint split existed under config/ -- one rule per endpoint when mergeFieldPath differs (discover vs select), plus a plain single-target rule for everything else.
…nc.Once [#49] timeoutTransport/cancelOnClose had zero test coverage despite being live in quick-start config (httpClientConfig.timeout: 25s) -- added tests proving the zero-timeout path leaves the transport unwrapped, a configured timeout wraps it, and it actually cuts off a response that stalls mid-body. Mutation-verified the stall test by disabling the wrapping and confirming it hangs. Also dropped cancelOnClose's sync.Once: context.CancelFunc is documented safe to call more than once, so the guard was unnecessary. Added a test that a repeated Close does not panic.
…nforced [#49] The validation that would have enforced it was reverted earlier on this branch -- only mergeFieldPath's presence and message-rooted shape are checked at load time. The sample config's wording claimed otherwise.
…r's own timeout [#49]
…whole StepContext [#49] Neither ever read anything else off it -- .Context for logging, .Body for the outbound request. Narrower signature, same behavior; full suite still green including -race.
merge.go had zero dependency on the handler package -- three stdlib imports, and its own tests never started a server -- while fanout.go (the executor) genuinely needs to stay in handler for the unexported internals it touches (ackSignerStep, sendNack, isAckSigner's wrapper-unwrapping). Leaving payload shaping in core put it somewhere this codebase otherwise keeps out of core, in mappings and plugins. Exported KeptResponse, ItemsOf and Responses (renamed from mergeResponses to avoid the merge.merge stutter); arrayAtPath, setAtPath and interleave stay unexported, package-private. pkg/merge's own tests carry a small local copy of the onDiscover/mergedIDs/sameIDs fixture builders rather than importing handler's test-only code across the package boundary.
Both are shared foundations for the next two commits: Clone lets addRouteStep stop copying Route field by field (the exact pattern that silently dropped MergeFieldPath in 78d8c0d), and ValidMergeFieldPath lets the router validate a merge path's shape without owning the Beckn-envelope knowledge of what that shape is. Neither is wired up yet -- that's the following two commits -- so this is purely additive.
…th [#49] A router resolves destinations from an action; it shouldn't also need to know that a reply has a message member. The presence check (required when target.urls names more than one target) stays here, since that is a routing-config-completeness question -- the shape check moves to the package that actually owns Beckn-envelope semantics.
… copy [#49] router.Route() returns the SAME *Route for every request matching a rule, so ctx.Route was rebuilt from it field by field -- and that list once silently dropped MergeFieldPath when it was added to Route without the copy being updated alongside it (78d8c0d). Clone copies the whole struct value, so it cannot repeat that mistake on the next field either. Regression test mutation-verified: reintroduced the old field-by-field copy, confirmed it fails on the dropped MergeFieldPath, reverted.
…ries per deployment [#49] maxConcurrency/timeout are operational and live in the handler's own config; mergeFieldPath is per-action semantics and lives in the routing rule -- defensible, but previously undocumented as a deliberate split. Ordering and donor selection are fixed in pkg/merge on purpose, not an asymmetry someone should later 'finish' by making them configurable too.
…49] Two rounds of self-review before external review landed, each verified by reproducing the failure and confirming the fix with a mutation-verified test. Real bugs: - withRawQuery (router.go) hand-built a fresh Route and dropped MergeFieldPath -- any fan-out request carrying a query string silently lost its merge path and NACKed even though every target answered. Now uses Route.Clone(). - ValidMergeFieldPath accepted a trailing/doubled dot and whitespace-padded segments ("message.", "message..catalogs", "message. catalogs"), all of which resolve to nothing at request time despite passing the load-time guard meant to catch exactly that. - mergeQuery wiped a target's own configured query whenever the inbound query was unparseable, instead of preserving it and appending the raw string after it. - A stale doc comment (mine, from an earlier commit) described fanout's config knob at the wrong path. Simplification: - Removed the query re-merge in fanout.go's callTarget: the router already bakes the inbound query into every target before fanout runs (the same way proxy() trusts route.URL as-is for the single-target path), so redoing it per fan-out call was dead work. - isAckSigner is now filtered out of responseSteps once per fan-out call instead of once per (target, step) pair. Test coverage closed: - A real router.Route()-into-fanout() integration test (the exact seam the MergeFieldPath bug lived in -- previously each half was only tested with a hand-built stand-in for the other). - The empty-segment/whitespace rejection, now also exercised through validateRules, not just the bare model.ValidMergeFieldPath unit test. - withRawQuery's .URLs (not just .URL) now asserted against the real multi-target field fan-out reads. - A -race test proving concurrent requests through the same cached multi-target rule never leak one request's query into another.
] A fan-out reply used to echo whichever target's response donated the merged array, so it claimed to be that target's identity and changed depending on which target answered first. It now carries the caller's own inbound context, action flipped to on_<action>. Also drops the per-target full-body request log in callTarget: the body is identical at every target and stdHandler.go already logs it once for the inbound request, so it was pure duplication scaling with target count.
Fan-out (target.urls, several destinations merged into one reply) only exists for targetType 'url'. loadRules' branch for 'bpp'/'bap'/'receiver'/ 'sender' never read target.urls, so a rule setting it there parsed clean and silently routed to nothing instead of erroring.
…t from cardinality [#49] targetType "url" doubled as both the single-target and the fan-out case, distinguished only by how many entries target.urls had. That let target.urls parse clean and silently route to nothing under targetType bpp/bap/receiver/sender (whose loader branch never read it), and forced Route.URL to stay set even for a fan-out route just so older single-target readers kept working. targetType "urls" (plural, target.urls, at least 2 entries, mergeFieldPath always required) is now its own type, a sibling of "url". Route.URL is nil for it, Route.URLs is nil for "url". The handler dispatches on the type directly instead of len(URLs) > 1. Mixing target.url/target.urls with the wrong type, or urls under bpp/bap/receiver/sender, is now a load-time error.
…erleave [#49] Interleaving added complexity for a fairness property (no single network's results permanently monopolizing the front of the list) nobody asked to keep. Concatenation is simpler and just as correct: same items, same no-dedupe behavior, only the ordering changes to "everything from the first target, then the second, and so on."
7661cc1 to
d401dd6
Compare
nisargabd
changed the title
3 participants
What
A
discoversent to the consumer adapter reaches every configured network in parallel and comes back as one merged catalog response, instead of exactly one network as before.Fan-out is its own routing type, not inferred from how many URLs happen to be listed:
targetType: "url"(unchanged) is a single target:target.url, nomergeFieldPath.targetType: "urls"is fan-out:target.urlswith at least 2 entries,mergeFieldPathalways required. Mixing the two (url:under"urls",urls:under"url"or underbpp/bap/receiver/sender) is a load-time config error, not a silent no-op.The executor (
core/module/handler/fanout.go) calls every target concurrently with the caller's own body and headers, under a concurrency cap (maxConcurrency, default 8) and one shared deadline for the whole fan-out (timeout, default 30s) rather than per target — with a concurrency cap, targets run in waves, and a per-target timeout would becomewaves × timeout. Response steps run once per kept target; the ack signer is skipped there and runs once over the merged body instead.The merge (
pkg/merge, its own package with no handler dependency) concatenates each target's items in rule order — everything from the first target, then the second, and so on. It does not dedupe — the same catalog id from two networks is two different providers' offer of the same resource, so both are kept. The merged envelope carries onlycontextandmessage, since a v2 action isadditionalProperties: falseand anything else would fail the caller's own schema.The reply's
contextcomes from the caller's own request, action flipped toon_<action>, not from whichever target's response happened to donate the merged array — a target is the adapter's proxy for one catalog, not this adapter's identity, so echoing a donor's context would have the reply claim to be whichever network answered first (and change depending on who was up). Onlymessageis taken from a donor.Partial success: a target that errors, answers non-2xx, fails a response step, or has an unreadable body at
mergeFieldPathis excluded and counted, not failed outright. The wire only ever sees a count (X-Beckn-Degraded-Count) — never which network — but the logs name the full target URL per degraded target, so an operator can actually tell which network is unhealthy. Only when every target fails is the response a NACK (502, codedNET_DOWNSTREAM_UNAVAILABLE, not a generic 500).limit/offsetare not handled by the adapter at all — forwarded to every target unchanged, and never re-applied or truncated after the merge. Each network is free to honor or ignore them; the merged result can be larger than what the caller asked for. This is deliberate, not a gap: there's no ordering across independent networks that would make a truncation or an offset mean anything, so the adapter doesn't pretend to.Safety limits: each target's response body is capped (10MB) so a runaway upstream can't be held open indefinitely under the concurrency cap; a separate earlier fix bounds every outbound call (including the single-target proxy path) with a real round-trip timeout, since
http.Client.Timeoutis silently ignored byhttputil.ReverseProxy.Why
Today's routing rule shape had
targetType: "url"do double duty — single target and fan-out told apart only by countingtarget.urlsin Go. That lettarget.urlsparse clean and do nothing underbpp/bap/receiver/sender(whose loader never reads it), and forced invariants ("URLalways set even for fan-out", "MergeFieldPathonly alongsideURLs") to live in comments instead of the type system — which already caused one real bug (MergeFieldPathsilently dropped by a field-by-field struct copy). Splitting fan-out intotargetType: "urls"makes the distinction a config fact instead of a Go implementation detail, and lets the loader reject the configs that used to fail silently.The context-donation issue (a fan-out reply claiming to be whichever network answered first) was a correctness bug independent of the schema question — fixed by always building the reply's context from the caller's own request.
Testing
go testacross every package) green,gofmt/go vetclean, no-raceissues in the fan-out executor's concurrency tests.pkg/plugin/implementation/router— theurl/urlstype split, cardinality and field-mixing rejections,mergeFieldPathshape validation, query-string propagation to every fan-out target without mutating the stored rule, concurrent requests through the same rule not sharing state.pkg/merge— concatenation order, no-dedupe, field-path configurability (catalogs vs. select's nestedcontract.commitments), context built from the request rather than a donor, unknown response members surviving the merge, a target's error envelope never donating.core/module/handler/fanout.go— partial degrade, total failure as a coded 502, per-target response cap, no goroutine leaks after a timeout, header hygiene (hop-by-hop stripped,X-Forwarded-For/Hostset,Te: trailerspreserved), the signature covering the merged body, the router-to-fanout composition (not a hand-builtRoutestanding in for what the router actually produces).quick-startcompose stack with a freshly built image each time: baseline single-target discover, a real 2-target fan-out merge with logs confirming both targets called and the reply count, the newtargetTypevalidation firing at load time (both the missing-mergeFieldPath and the fewer-than-2-targets cases crash the container with the expected error before it ever serves a request), and the context-from-request fix confirmed against a target whose own response carries no context at all.Closes #49