2.14 Stream Consumer Source support

[scottf]

No description provided.

[scottf]

@claude please review

[synadia-claude-reviewer]

Claude finished @scottf's task in 1m 50s —— View job

---

Code Review

• Gather context
• Read changed files
• Analyze changes
• Post review feedback

---

Overall this is a clean, well-structured addition. The new ConsumerSource class follows the existing patterns in the codebase, serialization/deserialization is consistent, and the test coverage is solid. A few observations below.

---

Issues Found

1. validateSource doesn't assert ConsumerSource (missing test coverage)

The JSON test data in StreamConfiguration.json includes consumer entries for both s0 and s1:

"consumer": { "name": "s0_con_name", "deliver_subject": "s0_con_deliver" }

But validateSource in StreamConfigurationTests.java:685-694 doesn't call validateConsumerSource. This means a deserialization regression for ConsumerSource on a Source object would go undetected. The mirror ConsumerSource is validated, but source entries are not.

// Missing from validateSource:
validateConsumerSource(Collections.singletonList(source.getConsumerSource()), name);

Fix this →

---

2. equals/hashCode style inconsistency in ConsumerSource.java

ConsumerSource.java:106-107 and 111-113 use the old pre-Java 7 ternary null-check pattern, while the updated SourceBase.java (which this PR modifies) uses Objects.equals():

// ConsumerSource.java (old style)
if (name != null ? !name.equals(that.name) : that.name != null) return false;
return deliverSubject != null ? deliverSubject.equals(that.deliverSubject) : that.deliverSubject == null;

// SourceBase.java (new style, same PR)
if (!Objects.equals(name, that.name)) return false;
return Objects.equals(consumerSource, that.consumerSource);

Similarly for hashCode:

// ConsumerSource (old style)
int result = name != null ? name.hashCode() : 0;
result = 31 * result + (deliverSubject != null ? deliverSubject.hashCode() : 0);

This is functionally correct (and EqualsVerifier validates the contract), but inconsistent with the surrounding code.

Fix this →

---

Minor Observations

Builder field naming: In SourceBaseBuilder, the field is named consumer (matching the JSON key constant), but the builder method is consumerSource(ConsumerSource consumer). Not a bug — the public API name is clear — just worth noting the slight asymmetry with filterSubject etc., which have matching field and method names.

testSourceBase coverage gap: The existing assertNotEqualsEqualsHashcode step pattern (used for startSeq, name, startTime, etc.) isn't applied to consumerSource. The correctness of equals is covered by EqualsVerifier.simple().forClass(Mirror.class).verify() in equalsContract(), so this isn't a real gap — just a style difference compared to the other fields in the same test.

---

What Looks Good

• The optionalInstance factory pattern is consistent with External and SubjectTransform
• Copy constructor ConsumerSource(ConsumerSource consumerSource) is implemented, which is important for the deep copy in SourceBaseBuilder(SourceBase base)
• Both fields are required by the builder (validateConsumerName + validateSubject) with appropriate test coverage for each invalid case
• toJson / deserialization round-trip is covered by testSerializationDeserialization
• The CONSUMER = "consumer" constant placement in ApiConstants.java is alphabetically correct

[scottf]

@claude please re-review with the following notes:

1. validateSource doesn't assert ConsumerSource (missing test coverage)

Fixed

2. equals/hashCode style inconsistency in ConsumerSource.java

Claude Code generated that. I'll make a note to Claude. I fixed with IntelliJ which is how I usually make equals and hashCode

Builder field naming: In SourceBaseBuilder, the field is named consumer (matching the JSON

ConsumerSource better matches the field name in the server which is called StreamConsumerSource. Non Issue.

testSourceBase coverage gap: The existing assertNotEqualsEqualsHashcode step pattern (used

Fixed

[synadia-claude-reviewer]

Claude finished @scottf's task in 1m 42s —— View job

---

Re-review

• Gather context
• Read changed files
• Verify claimed fixes
• Post updated review

---

All three fixes from the previous review have been applied correctly. Here's what I verified:

---

Fix 1: validateSource now asserts ConsumerSource ✅

validateConsumerSource is now a shared helper called from both the mirror validation block and validateSource. Additionally:

• validateExternal was extracted as a helper, eliminating the duplicated external-assertion inline code — a nice bonus cleanup
• The JSON test data was renamed to a consistent underscore-separated naming pattern (mirror_ext_api, s0_ext_api, mirror_con_name, s0_con_name, etc.), which makes the assertions unambiguous and easier to read

---

Fix 2: equals/hashCode style in ConsumerSource.java ✅

ConsumerSource.equals now uses Objects.equals and hashCode uses Objects.hashCode — consistent with SourceBase:

return Objects.equals(name, that.name) && Objects.equals(deliverSubject, that.deliverSubject);

int result = Objects.hashCode(name);
result = 31 * result + Objects.hashCode(deliverSubject);

The equals method is also marked final, which is a defensible choice for a concrete value type not intended for subclassing.

Note: SourceBase.hashCode() still uses the old ternary style for the pre-existing fields, and the new consumerSource line follows that same style — which is appropriate; updating the rest of SourceBase.hashCode() would be out of scope for this PR.

---

Fix 3: testSourceBase coverage gap ✅

sb.consumerSource(m.getConsumerSource()) / mb.consumerSource(m.getConsumerSource()) are now set before the final assertEquals(s1, sb.build()) / assertEquals(m1, mb.build()) checks. Since s1 / m1 are deserialized from JSON that includes a consumer block, this round-trip verification will catch any deserialization or equality regression for consumerSource.

---

New testConsumerSource test

A dedicated test was added for ConsumerSource, which covers:

• Happy-path getter verification
• All five invalid-argument cases (no name, no deliver, no both, dotted name, spaced deliver)
• EqualsVerifier.simple().forClass(ConsumerSource.class).verify() for the equals/hashCode contract

This is solid standalone coverage for the new class.

---

Summary

The PR is clean. All previous issues are resolved, the naming refactor in the JSON makes the test data self-documenting, and the new testConsumerSource test adds good standalone coverage. No further concerns.

[MauriceVanVeen]

LGTM