Repository navigation
fix(data): the collections GetStream overload names what it could not reduce - #3007
Merged
Merged
Conversation
… reduce #3004 gave Workspace.ReduceLocalStream a diagnostic that names the reference and the owning hub, because the bare InvalidOperationException("Failed to create stream") is exactly why three deliberate fault classifiers in SubscribeWithReEstablish could not recognise a boot-time probe's own-node read (#2990). The adjacent GetStream(params Type[] types) overload kept the old bare message, so the same ReduceStream failure stayed unclassifiable from one of the two call paths — the inconsistency Copilot's review of #3004 caught. It now names the resolved collection names and the hub, matching its sibling. The collections array is hoisted to a local so the message can quote it; behaviour is otherwise unchanged (the unknown-type ArgumentException still throws from the projection, before any reduce). MeshWeaver.Data.Test: 419/419 green. Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to diagnostic clarity and matches the PR description, with no functional behavior changes for valid inputs.
Pull request overview
This PR aligns Workspace.GetStream(params Type[] types) with the improved diagnostic contract introduced for Workspace.ReduceLocalStream, so ReduceStream failures are no longer reported with an unclassifiable bare "Failed to create stream" message.
Changes:
- Replace the bare
InvalidOperationException("Failed to create stream")with a message that includes the resolved collection names and owning hub address. - Hoist the resolved collection names into a local variable so they can be quoted in the failure message.
- Add XML doc remarks stating the shared diagnostic contract with
ReduceLocalStream.
File summaries
| File | Description |
|---|---|
| src/MeshWeaver.Data/Workspace.cs | Improves GetStream(Type[]) reduce-failure diagnostics to include collections + hub, matching ReduceLocalStream behavior. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…asserted Copilot's review of #3007 read `collections!` as redundant on the grounds that ToArray() never returns null. The suppression was not about the array: TryGetCollectionName's out parameter is `string?`, so the projection is `string?` and the array is `string?[]`, which CollectionsReference (IReadOnlyCollection<string>) rejects with CS8620 — an error under CI's -warnaserror. Removing the `!` alone would red the build. The finding is still right that a suppression is the wrong shape here, so the invariant is now CHECKED rather than asserted: `&& name is not null` makes the projection a real `string`, the array a `string[]`, and the suppression unnecessary. It also closes the case the suppression silently allowed — a `true` return with a null name would have gone into the reference as a null collection name; it now throws the same unknown-type ArgumentException every other unresolvable type gets. Deliberately NOT changed: argument validation for a null `types` array or a null element. That NRE is pre-existing, is a programming error at the call site, and validating it is a separate change from this one's subject (an unclassifiable diagnostic message). MeshWeaver.Data.Test: 419/419 green. Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
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
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.
Follow-up to #3004 (which closed #2990). Lands the one review finding that arrived after
auto-merge had already taken #3004.
#3004 gave
Workspace.ReduceLocalStreama diagnostic that names the reference and the owning hub —because the bare
InvalidOperationException("Failed to create stream")is precisely why threedeliberate fault classifiers in
SubscribeWithReEstablishcould not recognise a boot-time probe'sown-node read, and reported it as a transient production ERROR.
The adjacent
GetStream(params Type[] types)overload kept the old bare message, so the sameReduceStreamfailure stayed unclassifiable from one of the two call paths. It now names theresolved collection names and the hub, matching its sibling.
Behaviour is otherwise unchanged: the collections array is hoisted to a local purely so the message
can quote it, and the unknown-type
ArgumentExceptionstill throws from the projection, before anyreduce.
MeshWeaver.Data.Test419/419 green;dotnet build src/MeshWeaver.Data -c Release -warnaserrorclean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa