Repository navigation
docs: record the public-by-default audience and one-or-many networkId - #60
Conversation
…#58] Split from the code PR, which is about something a reviewer can check by reading Go; this is the plan, which is the spec and changes first. A28: an omitted visibleTo is the wildcard. A9 is NOT carved up — the default still applies in both update modes, by the same two lines. The first draft of A28 made an omitted visibleTo mean "keep what is stored" under MERGE; review took it out as a change to what MERGE means rather than to a default, and the hazard it named is now a Deferred row so the argument survives the code that was removed. A29: context.networkId is one network or several, and a search matches exactly the networks it names. Public stops being something a scoped caller is handed and becomes a network they ask for, which is what lets the five queries collapse to one array overlap with no literal in them. Two Deferred rows carry what was tried and pulled: the MERGE keep rule, and deleting Catalog.NetworkID, which gofmt punishes out of proportion to its value in a review about something else. Scenarios 26a, 29 and 29a state what the four tests assert. The scenario count stands at 37.
🛡️ 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: ➖ Not applicable — no non-test Go files changed vs |
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
The plan carries the reasoning, not just the rule, and the three scenario rows name the tests that hold each one. Two things.
1. A29 describes a marshal the code does not do, and argues for it on a premise the code rejects
Problem
A29 in docs/design/implementation-plan.md says NetworkIDs:
marshals a single id back as a string, so a caller who sent
["n1"]is echoed"n1"— identical under theoneOf, and the alternative is carrying arrival-shape state through a struct the response re-marshals.
The code in #59 does the alternative. src/beckn/types.go:32 holds a scalar bool, SomeNetworks leaves it false, and MarshalJSON only collapses to a string when it is true. A caller who sends ["n1"] is echoed ["n1"].
That is the better behaviour — an echo that changes shape is a surprise even where the schema permits it. But the plan is the spec, so a reader resolving a disagreement between the two will conclude the code is wrong.
Recommendation
Rewrite the sentence to what ships: the arrival shape is preserved, a one-element array is echoed as an array, and the scalar field is what carries it. Say why — the response should spell networkId the way the request did — instead of the rejected-alternative clause, which is now describing what was built.
2. The plan still shows the three-arm SQL that #59 deletes
Problem
The resources DDL commentary keeps:
OR r.visible_to @> ARRAY['*']with the paragraph beneath it explaining why the '*' arm is a third arm rather than a widening of the second, and that a query carrying the nullable form but not the '*' arm "hides every public catalog from every scoped caller." A29, further down the same document, replaces all of it with r.visible_to && @network_ids::text[], and #59 deletes those arms from all five queries.
Both passages are live text in the same document. The second one is now describing a failure mode of a query shape that no longer exists.
Recommendation
Strike the three-arm block and its paragraph the way C8 was struck, with a pointer to A29, or cut it outright. Leaving two SQL shapes in the plan means the next person implementing against it picks one.
Resolved
- Splitting the plan out of #59 was the ask, and this is it.
- C8's old reasoning is kept struck through rather than deleted, with the trade stated — that is the right shape for a deviation that closed.
- Both Deferred rows carry what happens if nobody does them, which is the part usually missing.
Three passages still described the shape A29 replaced, and A29's own row described a marshal the code does not perform. The row claimed NetworkIDs collapses a one-element array to a string, so a caller who sent ["n1"] is echoed "n1", and argued for it on the grounds that the alternative means carrying arrival-shape state. The code carries exactly that state — one bool — and echoes back the shape that arrived. It is the better behaviour: the response context is the request's context read back, and a field that changes spelling in it reads as something having happened to it. The oneOf permits either, so this was never a conformance question, only a surprise question. The row now says what ships and why. The resources DDL commentary still carried the three-arm `sqlc.narg(...) IS NULL OR ... @> ARRAY['*'] OR ... @> ARRAY[$1]` with two paragraphs arguing for it, while A29 further down the same document replaced all of it with one `&&`. Two SQL shapes in one plan means whoever implements against it picks one. The block is now the shipped predicate, and the prose says why overlap is the whole gate: the scope is never empty, so there is no absent-predicate case to write. force_custom_plan survives, with network_id removed from the list of clauses that need it — target_paths, spatial_op, the four box bounds and schema_contexts are still nullable, so the setting is unchanged. Task 16's test pins say what the test now reads, and the omitted-networkId scenario is restated as A29's three rules against three catalogs, which is what the shipped test asserts.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Both findings fixed in 32dede2, plus a third of the same kind found while fixing them.
Resolved
-
A29's marshal sentence now says what ships: the arrival shape is preserved,
["n1"]is echoed["n1"], and thescalarbool is what carries it. Verified live —"netA"echoes"netA",["netA"]echoes["netA"]. -
The three-arm SQL in the
resourcescommentary is the shippedr.visible_to && @network_ids::text[], and the two paragraphs arguing for thenarg/ARRAY['*']arms are replaced by why overlap is the whole gate.force_custom_plansurvives withnetwork_idstruck from the list of clauses needing it —target_paths,spatial_op, the box bounds andschema_contextsare still nullable, so the setting is unchanged. -
Two more passages said the same stale thing and are fixed with them: Task 16's test pins (which now quote the predicate the test actually reads, and restate the scenario as A29's three rules over three catalogs), and the "Every column here is on
resources" paragraph's claim about the nullable form.
The plan behind #59, split out of it at review's request. No code.
What
implementation-plan.md— A28visibleTois the wildcard. A9 is not carved up: the default still applies in both update modes, by the same two linescontext.networkIdis one network or several, and a search matches exactly the networks it names. Public becomes a network a caller asks for, which is what lets the five queries collapse to one array overlap with no literal in themCatalog.NetworkIDpublish-and-discover.mdAPP_NETWORK_ID, and what eachnetworkIdform returnsWhy
visibleTomean keep what is stored under MERGE. Review took it out as a change to what MERGE means rather than to a default. The hazard it named — a narrowed catalog going public on a small edit, with nothing said — is a Deferred row, so the argument outlives the code that was removed.Catalog.NetworkIDis deferred, not forgotten. A28 leaves it with no reader, but it is the longest key in three struct literals, so removing it makes gofmt realign ten lines and drags eleven untouched files into a review about something else.Testing
Documentation only; no build, lint or test output applies. Every statement here is asserted by a test in #59, and the scenario rows name them.
Merge after #59, or before it — nothing depends on the order.