Skip to content

Fix the handling of subscriptions with multiple topics and extra fields#2425

Open
abalarev wants to merge 1 commit intoeclipse-ditto:masterfrom
boschglobal:bugfix/multple-topics-per-target
Open

Fix the handling of subscriptions with multiple topics and extra fields#2425
abalarev wants to merge 1 commit intoeclipse-ditto:masterfrom
boschglobal:bugfix/multple-topics-per-target

Conversation

@abalarev
Copy link
Copy Markdown
Contributor

@abalarev abalarev commented Apr 24, 2026

Summary
Resolves incorrect handling of subscriptions for multiple topics, containing at least one extra field.

Issue

  • multiple topics without extraFields works ok - sends to the matching ones
  • having multiple topics with extraFields works for only one of them (when it's matching), but not deterministic for which one
  • having a topic or topics with extraFields and some without extraFields works again only for one of the topics with extraFields (when it's matching and not for each topic), but for none of the topics without extraFields

What's changed
OutboundMaooingProcessorActor flow changed as follows:

  • retrieving of extra fields for all topics (per target) combined in a single request
  • the filter is now applied using already retrieved extra fields as it needs them to filter correctly. Only those extra fields defined in the matching filter are left in the outbound signal
  • splitting of targets with and without extra fields is now removed as it became redundant
  • the new flow is still optimized for targets with no extra fields in the topics.
  • outbound signal ordering is now changed, therefore the order of unit tests expectations is also changed
  • more unit tests are created to verify the handling of the problem cases described above

Added system tests: eclipse-ditto/ditto-testing#33

@abalarev abalarev changed the title Handling of subscriptions with multiple topics with some extraFields [WIP] Handling of subscriptions with multiple topics with some extraFields Apr 27, 2026
@abalarev abalarev force-pushed the bugfix/multple-topics-per-target branch from f26bbaf to 01fd8d8 Compare April 27, 2026 08:52
@abalarev abalarev changed the title [WIP] Handling of subscriptions with multiple topics with some extraFields Handling of subscriptions with multiple topics with some extraFields Apr 27, 2026
@abalarev abalarev changed the title Handling of subscriptions with multiple topics with some extraFields Handling of subscriptions with multiple topics with extraFields Apr 27, 2026
@abalarev abalarev changed the title Handling of subscriptions with multiple topics with extraFields Handling of subscriptions with multiple topics with extra fields Apr 27, 2026
@abalarev abalarev changed the title Handling of subscriptions with multiple topics with extra fields Fix the handling of subscriptions with multiple topics with extra fields Apr 27, 2026
@thjaeckle
Copy link
Copy Markdown
Member

@abalarev will you also provide a system-test which tests this fix - seems important enough to me to cover it with an integration-test

@thjaeckle thjaeckle added this to the 3.9.0 milestone Apr 29, 2026
@thjaeckle thjaeckle added the bug label Apr 29, 2026
@abalarev
Copy link
Copy Markdown
Contributor Author

@thjaeckle Sounds reasonable, I'll add some system test(s)

@abalarev abalarev changed the title Fix the handling of subscriptions with multiple topics with extra fields Fix the handling of subscriptions with multiple topics and extra fields Apr 29, 2026
@thjaeckle
Copy link
Copy Markdown
Member

@abalarev thanks for the added system tests 👍
I just ran them and seems there are some related test failures for MQTT, could you check them please?
https://github.com/eclipse-ditto/ditto/actions/runs/25307537926

@abalarev
Copy link
Copy Markdown
Contributor Author

abalarev commented May 4, 2026

@thjaeckle Please run the system tests again, I've pushed a fix.

@abalarev abalarev force-pushed the bugfix/multple-topics-per-target branch from 01fd8d8 to 340ea56 Compare May 5, 2026 08:55
Copy link
Copy Markdown
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this — the refactor direction looks right and the headline scenarios are well-covered. A few correctness concerns and polish items inline.

One overarching note on test coverage: the four correctness concerns flagged below (shared-root trimming, cross-streaming-type early return, non-deterministic findFirst over a Set, and catch-all / extras precedence) are not yet exercised by the new tests. Once the intended behavior for each is settled, please add regression tests that lock it in.

final var builder = extra.toBuilder();
allExtraFields.getPointers().stream()
.filter(pointer -> !neededFields.getPointers().contains(pointer))
.forEach(pointer -> pointer.getRoot().ifPresent(builder::remove));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trim removes by pointer.getRoot(), which deletes the whole top-level object. With allExtraFields = ["/features/f1", "/features/f2", "/attributes"] and neededFields = ["/features/f1", "/attributes"], the loop hits /features/f2, removes the features root, and also wipes /features/f1 that the matched topic actually requested.

The trimming should operate at full-pointer depth (remove the value at the exact pointer, or rebuild the JSON containing only neededFields) rather than at the root segment.

.map(FilteredTopic::getExtraFields)
.flatMap(Optional::stream)
.toList();
boolean topicWithNoFilterNoExtraFieldsExists = topics.stream().anyMatch(topic -> topic.getFilter().isEmpty() && topic.getExtraFields().isEmpty());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

topics is target.getTopics() unfiltered by streaming type — pairTargetsWithTopics now passes all of a target's topics whenever any one of them has extra fields. So a target with e.g. a TWIN_EVENTS extras-topic plus a LIVE_MESSAGES topic with no filter and no extras would short-circuit enrichment for a TWIN_EVENTS signal, even though the LIVE_MESSAGES topic is irrelevant to that signal.

This anyMatch should apply the same streamingType == StreamingType.fromTopic(topic.getTopic().getPubSubTopic()) guard used in pairTargetsWithTopics.

.flatMap(Optional::stream)
.toList();
boolean topicWithNoFilterNoExtraFieldsExists = topics.stream().anyMatch(topic -> topic.getFilter().isEmpty() && topic.getExtraFields().isEmpty());
if (allExtraFields.isEmpty() || topicWithNoFilterNoExtraFieldsExists) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change worth pinning down: a target that mixes { TWIN_EVENTS, no filter, no extras } with { TWIN_EVENTS, filter=X, extras=definition } will now be delivered without extras (the catch-all wins via this early return). Previously the same signal also produced an enriched copy.

Deduplication is good, but is it intentional that the catch-all topic wins over the topic that explicitly requested enrichment? Either way, please document the precedence rule in the javadoc of enrichAndFilterSignal so the semantics are unambiguous.

.map(signal -> setTrimmedExtra(signal, topic, expressionResolver,
extra, allExtraFieldsOptional.get()))
.stream())
.findFirst()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

topics is a Set<FilteredTopic>, which has no defined iteration order. If a signal matches more than one topic on the target, findFirst() picks an arbitrary one, so the trimmed extras delivered downstream become non-deterministic — the very symptom this PR sets out to fix.

Consider iterating in a deterministic order: derive from target.getTopics() via a structure that preserves insertion order, or sort topics by a stable key, so the chosen match is reproducible.

Signed-off-by: Andrey Balarev <andrey.balarev@bosch.com>
@abalarev abalarev force-pushed the bugfix/multple-topics-per-target branch from 340ea56 to c4216eb Compare May 7, 2026 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants