fix(#3828): dedup duplicate publications and subscriptions by (channel, action)

[DevJonny]

Previously the producer registry and SupplementalPublications could declare the same topic and end up in the document as two send operations (send_foo + send_foo_2). Subscriptions had the same flaw on the receive side. Assembly scanning was the only path that correctly skipped duplicates.

Move the dedup into ProcessSourceAsync so all three sources go through the same gate: if (channel, action) is already covered, drop the duplicate. First source wins (producer registry beats SupplementalPublications; DI publications beat assembly scanning). Same channel with a different action (e.g. a subscription and a publication on one routing key) still produces both operations as intended.

GetUniqueOperationId and the OperationKey record become dead code with true dedup in place — both removed. The redundant CoveredChannelActions check in AddFromAssemblyScanningAsync is also dropped.

Tests:

• It_Should_Dedup_Duplicate_Subscriptions_On_The_Same_Topic (was It_Should_Produce_One_Channel_And_Two_Operations_For_Duplicate_Subscriptions, which asserted the buggy behaviour).
• It_Should_Dedup_Producer_Registry_Over_Supplemental_Publications now asserts one operation rather than two.
• It_Should_Collapse_Duplicate_DI_And_Scanned_Publications_To_One_Operation replaces the "_Uniqueified_Operation_Id" test that codified the old suffix behaviour.

46/46 AsyncAPI tests pass on net9.0 and net10.0.

Description

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

[Copilot]

Pull request overview

Fixes a bug where duplicate publications/subscriptions on the same (channel, action) produced misleading "_2"-suffixed operations in the generated AsyncAPI document. Deduplication is now centralized in ProcessSourceAsync so producer-registry, supplemental publications, and assembly scanning all go through the same first-wins gate.

Changes:

• Move (channel, action) dedup into ProcessSourceAsync; remove now-dead GetUniqueOperationId and OperationKey.
• Drop the redundant early-skip in AddFromAssemblyScanningAsync.
• Update tests to assert single operations for duplicate subscriptions, producer-registry-vs-supplemental, and DI-vs-scanned cases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Paramore.Brighter.AsyncAPI/AsyncApiDocumentGenerator.cs	Centralizes dedup in ProcessSourceAsync, removes unique-id suffix logic and OperationKey.
tests/Paramore.Brighter.AsyncAPI.Tests/When_Deduplicating_Channels_And_Messages.cs	Updates duplicate-subscription and producer-registry-vs-supplemental tests to expect a single operation.
tests/Paramore.Brighter.AsyncAPI.Tests/When_Generating_Document_From_Assembly_Scanning.cs	Renames and updates the DI-vs-scanned test to expect a single collapsed operation.

[claude]

Review: PR #4137 — dedup duplicate publications and subscriptions by (channel, action)

Thanks for the focused fix. The diagnosis is right: prior to this PR, GetUniqueOperationId papered over a real bug by minting send_foo_2 operation IDs that pointed at the same channel and the same message — producing a misleading document rather than a duplicate-free one. Centralising the dedup in ProcessSourceAsync so all three sources (subscriptions, publications, assembly scanning) go through one gate is the right shape.

A few items to consider:

🐛 Formatting regression in AsyncApiDocumentGenerator.cs

EmptyObjectSchema() at src/Paramore.Brighter.AsyncAPI/AsyncApiDocumentGenerator.cs:346 lost its indentation when neighbouring code was edited:

private static V3SchemaDefinition EmptyObjectSchema()
        {

It should align with the surrounding private static members (8-space indent). Easy fix, but worth catching before merge — every other method declaration in this class is indented consistently.

There is also a stray double blank line at :66-67 where the OperationKey record used to live. Minor.

⚠️ Silent drop when RequestTypes differ but (channel, action) matches

The new semantic is "first source wins" — but if two publications share (topic, Send) and declare different RequestTypes, the second's type is silently discarded. This is arguably the correct behaviour (AsyncAPI doesn't really cope with one operation pointing at two payload schemas), but it's also a footgun for users who genuinely have a type mismatch in their registration.

Two suggestions:

1. Add a test pinning the "first wins on type" behaviour explicitly. The current tests (It_Should_Dedup_Producer_Registry_Over_Supplemental_Publications, It_Should_Collapse_Duplicate_DI_And_Scanned_Publications_To_One_Operation) all use the same RequestType on both sides, so they don't actually prove which schema wins when they differ. A test along the lines of "two publications on dedup.topic, first with SharedEvent and second with OtherEvent, asserts the resulting message component is SharedEvent" would lock this in.
2. Consider surfacing a warning/log when a dedup happens with mismatched types. Even a debug-level log would help a user diagnose "why is my publication missing from the doc". Not a blocker — could be a follow-up.

The "first wins" precedence (producer registry > SupplementalPublications > assembly scan) is also a public-facing contract now; worth adding an XML doc comment on the class or on ProcessSourceAsync that states it explicitly, since the precedence is purely an artefact of call order in GenerateAsync today.

🧹 CoveredChannelActions is now strictly equivalent to Operations.ContainsKey

With the dedup moved into ProcessSourceAsync and the assembly-scan early-skip gone, the HashSet<(string, string)> state on GenerationContext carries exactly the same information as Operations.ContainsKey($"{actionString}_{channelId}"). It still works correctly — just noting it's now redundant state and could be removed for clarity. Not a blocker.

✅ What I like

• Genuine bug fix, not just papering over symptoms. The old _2 operation IDs were actively misleading because they referenced the same channel/message — readers would assume two distinct operations existed.
• Dead-code removal of GetUniqueOperationId and the OperationKey record is exactly the right cleanup once the dedup is real.
• Test renames (It_Should_Dedup_Duplicate_Subscriptions_On_The_Same_Topic, It_Should_Collapse_Duplicate_DI_And_Scanned_Publications_To_One_Operation) now describe behaviour rather than implementation artefacts — the previous "Uniqueified_Operation_Id" name was codifying the bug.
• It_Should_Produce_One_Channel_With_Receive_And_Send_For_Same_Topic correctly continues to assert that same-channel-different-action is not treated as a duplicate. Good guard against an over-eager dedup regression.
• The inline comment in ProcessSourceAsync explaining the dedup intent and the same-channel-different-action carve-out is clear and load-bearing.

Verdict

Solid bug fix with appropriate test updates. Address the indentation regression on EmptyObjectSchema, ideally add the mixed-RequestType test (or accept it as a follow-up), and this is good to go.

---

Review by Claude (claude-opus-4-7) — automated PR review

[Jonny-Freemarket]

gcp-ci failed on Paramore.Brighter.Gcp.Tests/MessagingGateway/Pull/Reactor/When_requeueing_redrives_to_the_dlq.cs:84 with Expected: 1, Actual: 0 — the test sleeps 5s then immediately polls the Pub/Sub emulator for the DLQ count, which is a known race. Test has no overlap with the AsyncAPI module touched by this PR. Could you rerun the failed job? I don't have admin rights to do it from here.

[iancooper]

gcp-ci failed on Paramore.Brighter.Gcp.Tests/MessagingGateway/Pull/Reactor/When_requeueing_redrives_to_the_dlq.cs:84 with Expected: 1, Actual: 0 — the test sleeps 5s then immediately polls the Pub/Sub emulator for the DLQ count, which is a known race. Test has no overlap with the AsyncAPI module touched by this PR. Could you rerun the failed job? I don't have admin rights to do it from here.

Have done. I am alright with merging with some flaky tests, it's just a release we need to review/fix them for

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Review: PR #4137 — dedup duplicate publications and subscriptions by (channel, action)

The diagnosis is correct and the fix is well-scoped. The pre-existing GetUniqueOperationId was papering over a real bug — minting send_foo_2 operation IDs that pointed at the same channel and same message produced a misleading document, not a correct one. Centralising the (channelId, actionString) gate in ProcessSourceAsync so subscriptions, publications, and assembly-scanning all flow through one dedup is the right shape.

Things I like

• Bug fix, not symptom suppression. The dead-code removal of GetUniqueOperationId and the OperationKey record is the right cleanup — those were only there to serve the buggy behaviour.
• Test renames now describe behaviour, not implementation artefacts. It_Should_Skip_Assembly_Scanned_Type_When_DI_Has_Uniqueified_Operation_Id was a name that codified the bug; It_Should_Collapse_Duplicate_DI_And_Scanned_Publications_To_One_Operation reads as a real specification.
• It_Should_Produce_One_Channel_With_Receive_And_Send_For_Same_Topic is preserved. Good guard against an over-eager dedup that would collapse a subscription + publication on the same routing key into a single operation. The (channel, action) key — rather than just channel — correctly carves this out.
• The inline comment in ProcessSourceAsync is load-bearing. It explains both the precedence (first source wins) and the same-channel-different-action carve-out, which would otherwise be subtle to a future reader.

Things worth considering

1. Test gap: mismatched RequestType on the same (channel, action)
All three dedup tests use the same RequestType on both sides of the duplicate. They prove the second operation is dropped, but they don't prove which message schema wins when the duplicates disagree. With first-wins semantics, the later type is silently discarded — arguably correct (AsyncAPI can't really express one operation pointing at two payload schemas), but the behaviour deserves a pin-down test:

Publication A: topic "x", RequestType = TypeA
Publication B: topic "x", RequestType = TypeB
→ assert result.Components.Messages contains "TypeA" and NOT "TypeB"

Without this, a future refactor that re-orders the dedup or changes the wins-policy can pass all current tests while silently swapping which schema appears in the document.

2. CoveredChannelActions is now redundant state
With the dedup moved into ProcessSourceAsync and the assembly-scan early-skip gone, context.CoveredChannelActions and context.Operations.ContainsKey($"{actionString}_{channelId}") carry identical information at every program point. It still works correctly — just noting it's now a parallel structure that could be removed for clarity. Not a blocker; could be a follow-up cleanup.

3. The first-wins precedence is now an implicit public-facing contract
"Producer registry beats SupplementalPublications, DI publications beat assembly scanning" is currently a pure artefact of the call order in GenerateAsync (subscriptions → publications → assembly scanning). The PR description states this contract, but a reader of the code would have to derive it from call order. Worth either an XML doc on the class or on ProcessSourceAsync stating it explicitly, since it's now a behaviour users may rely on (and a behaviour anyone re-ordering GenerateAsync could silently break).

4. Minor: stray double blank line at AsyncApiDocumentGenerator.cs:66-67
Where the OperationKey record used to live. Cosmetic only — dotnet format would resolve it.

(Note: I checked the indentation of EmptyObjectSchema at :346 flagged in an earlier comment on this PR — it's actually consistent with the surrounding private static members, all aligned at 8 spaces. No regression there.)

Verdict

Solid, targeted bug fix. The behaviour change is in the right direction and the test updates correctly capture the new semantics. The mixed-RequestType test is the most valuable follow-up; the rest is minor.

---

Review by Claude (claude-opus-4-7) — automated PR review

[DevJonny]

I've just resolved the outstanding Copilot comment so I'll keep and eye and merge when it's ready.