feat: Added subscribeTopic for TopicClient#147
Conversation
manishdait
force-pushed
the
feat/subscribe-topic-impl
branch
from
d48e9b7 to
f387a73
Compare
aceppaluni
added
the
status: ready-for-review
There was a problem hiding this comment.
Pull request overview
This PR adds a subscribeTopic() API to TopicClient (backed by Hedera TopicMessageQuery.subscribe) and updates configuration to distinguish mirror-node gRPC addresses (for HCS subscriptions) from the mirror-node REST base URL (for Mirror Node REST queries) across base, Spring, and MicroProfile modules.
Changes:
- Introduces
TopicClient.subscribeTopic(...)overloads plus protocol-layer request/response types to support topic message subscriptions. - Splits mirror-node configuration into gRPC addresses vs REST URL (
getMirrorNodeGrpcAddresses()+getMirrorNodeRestUrl()), updating config implementations and auto-configuration wiring. - Adds/extends unit and integration tests for topic subscription behavior in base, Spring, and MicroProfile modules.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| hiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java | Updates test network settings to provide mirror-node gRPC address + REST URL separately. |
| hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.java | Comment formatting adjustment in tests (no functional change). |
| hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.java | Adds Spring integration tests covering subscribeTopic() overloads. |
| hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroNetworkProperties.java | Reworks Spring properties to model mirror node as nested REST/grpc config. |
| hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroConfigImpl.java | Maps Spring properties/network settings to new mirror-node REST + gRPC fields. |
| hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroAutoConfiguration.java | Switches mirror-node client creation to use REST URL from HieroConfig. |
| hiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.java | Adds MicroProfile integration tests for subscribeTopic(). |
| hiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/HieroConfigImplTest.java | Updates tests for new mirror-node config shape. |
| hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.java | Updates MicroProfile config impl to use mirror-node REST URL + gRPC addresses. |
| hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.java | Introduces nested mirror-node config parsing for MicroProfile. |
| hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.java | Updates mirror-node client producer to read REST URL from HieroConfig. |
| hiero-enterprise-base/src/test/java/org/hiero/base/test/TopicClientImplTest.java | Adds unit tests validating subscription request construction and validation. |
| hiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.java | Updates base test settings for separated mirror-node REST/grpc config. |
| hiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.java | Uses mirror-node gRPC addresses when configuring SDK Client.setMirrorNetwork(...). |
| hiero-enterprise-base/src/main/java/org/hiero/base/TopicClient.java | Adds subscribeTopic() overloads and refines several javadocs. |
| hiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageResult.java | Returns SubscriptionHandle from topic message query execution. |
| hiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.java | Adds factory overloads to include limit and optional time range. |
| hiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.java | Implements subscribeTopic() and validates limit/time range parameters. |
| hiero-enterprise-base/src/main/java/org/hiero/base/implementation/ProtocolLayerClientImpl.java | Executes Hedera TopicMessageQuery.subscribe() and returns the handle. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/NetworkSettings.java | Renames mirror-node address method to gRPC + adds REST URL accessor. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/NetworkSettingsBasedHieroConfig.java | Adapts config wrapper to new mirror-node REST/gRPC accessors. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.java | Splits mirror-node env config into gRPC address + REST URL env vars. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/HieroConfig.java | Updates client creation to use mirror-node gRPC addresses and exposes REST URL. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaTestnetSettings.java | Provides Hedera testnet mirror-node gRPC address + REST URL separately. |
| hiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaMainnetSettings.java | Provides Hedera mainnet mirror-node gRPC address + REST URL separately. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Objects.requireNonNull(handler, "handler must not be null"); | ||
|
|
||
| if (limit < -1) { | ||
| throw new IllegalArgumentException("limit must be -1 (infinite) or greater than 0"); |
| */ | ||
| default SubscriptionHandle subscribeTopic( | ||
| @NonNull String topicId, @NonNull Consumer<TopicMessage> handler, long limit) | ||
| throws HieroException { |
| * @param topicId the topicId of topic | ||
| * @param handler the handler to call when a message is receive | ||
| * @return SubscriptionHandle for the Topic |
| .findFirst() | ||
| hieroConfig | ||
| .getMirrorNodeRestUrl() | ||
| .orElseThrow(() -> new IllegalStateException("No mirror node addresses configured")); |
| mirrorNodeGrpcAddress = getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").orElse(null); | ||
| mirrorNodeRestUrl = getEnv("HEDERA_MIRROR_NODE_REST_URL").orElse(null); |
| @ConfigProperty(name = "hiero.network.mirrornode.rest-url") | ||
| Optional<String> restUrl = Optional.empty(); | ||
|
|
||
| @ConfigProperty(name = "hiero.network.mirrornode.grpc-addresses") | ||
| public Optional<String> grpcAddresses = Optional.empty(); |
| topicClient.subscribeTopic( | ||
| topicId, | ||
| (message) -> { | ||
| messages.add(new String(message.contents)); | ||
| }); |
| topicClient.submitMessage(topicId, msg); | ||
| hieroTestUtils.waitForMirrorNodeRecords(); | ||
| Thread.sleep(5000); // Make sure to wait after message get recorded in mirrornode | ||
|
|
||
| Assertions.assertNotNull(handler); |
| final SubscriptionHandle handler = | ||
| topicClient.subscribeTopic( | ||
| topicId, | ||
| (message) -> { | ||
| messages.add(new String(message.contents)); | ||
| }); |
| topicClient.submitMessage(topicId, msg); | ||
| hieroTestUtils.waitForMirrorNodeRecords(); | ||
| Thread.sleep(5000); // Make sure to wait after message get recorded in mirrornode | ||
|
|
||
| Assertions.assertNotNull(handler); |
|
|
||
| topicClient.submitMessage(topicId, msg); | ||
| hieroTestUtils.waitForMirrorNodeRecords(); | ||
| Thread.sleep(5000); // Make sure to wait after message get recorded in mirrornode |
There was a problem hiding this comment.
For the
Thread.sleep(5000);could/should this be updated to either a polling with timeout or latch/countdown mechanism to avoid flakiness?
Something like
await().atMost(10, SECONDS)
.until(() -> messages.size() == 1);| Assertions.assertNotNull(handler); | ||
| Assertions.assertEquals(1, messages.size()); | ||
| Assertions.assertEquals(msg, messages.getFirst()); | ||
| handler.unsubscribe(); |
There was a problem hiding this comment.
Should these be verified?
|
@manishdait could you resolve the conflicts! |
manishdait
force-pushed
the
feat/subscribe-topic-impl
branch
from
f387a73 to
933afda
Compare
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Topic Subscription Interface Contract hiero-enterprise-base/src/main/java/org/hiero/base/TopicClient.java |
TopicClient adds subscription overloads and corrects @throws Javadoc text for update, delete, and submit message methods. |
Topic Subscription Implementation and Validation hiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.java |
TopicClientImpl implements subscription overloads, validates limit and time ordering, builds TopicMessageRequest, and returns the subscription handle. |
Protocol Layer Subscription Support hiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.java, hiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageResult.java, hiero-enterprise-base/src/main/java/org/hiero/base/implementation/ProtocolLayerClientImpl.java |
TopicMessageRequest gains subscription factories, TopicMessageResult now carries SubscriptionHandle, and ProtocolLayerClientImpl preserves the returned handle. |
Base Module Subscription Tests hiero-enterprise-base/src/test/java/org/hiero/base/test/TopicClientImplTest.java |
TopicClientImplTest adds unit coverage for subscription overloads, captured request fields, and invalid limit and time-window validation. |
Mirror Node Configuration API Contracts hiero-enterprise-base/src/main/java/org/hiero/base/config/NetworkSettings.java, hiero-enterprise-base/src/main/java/org/hiero/base/config/HieroConfig.java |
NetworkSettings and HieroConfig replace the single mirror-node address accessor with separate gRPC and REST accessors, and HieroConfig#createClient() uses the gRPC set. |
Hedera Preset Network Configurations hiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaMainnetSettings.java, hiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaTestnetSettings.java |
HederaMainnetSettings and HederaTestnetSettings expose mirror-node gRPC addresses and REST URLs through the split API. |
Base Module Configuration Implementations hiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.java, hiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/NetworkSettingsBasedHieroConfig.java, hiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.java, hiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.java |
EnvBasedHieroConfig, NetworkSettingsBasedHieroConfig, HieroTestContext, and SoloActionNetworkSettings switch to gRPC and REST mirror-node accessors. |
MicroProfile Configuration Property Binding hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.java, hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.java |
HieroNetworkConfiguration now binds a nested mirror-node object, and MicroProfile HieroConfigImpl stores separate gRPC and REST values while deriving them from either NetworkSettings or configuration. |
Spring Configuration Property Binding hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroNetworkProperties.java, hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroConfigImpl.java |
HieroNetworkProperties binds nested mirror-node properties, and Spring HieroConfigImpl exposes separate gRPC and REST mirror-node values. |
Client Providers Updated for Mirror Node Configuration hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.java, hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroAutoConfiguration.java |
ClientProvider reads the mirror-node REST URL directly from configuration, and HieroAutoConfiguration validates the REST endpoint before building the mirror-node client. |
MicroProfile Integration Subscription Tests hiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.java |
TopicClientTest adds MicroProfile integration coverage for subscription callbacks, limits, time windows, and validation failures. |
Spring Integration Subscription Tests and Cleanup hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.java, hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.java |
TopicClientTest adds Spring integration coverage for the same subscription flows, and TopicRepositoryTest removes a debug print. |
Test Utilities Updated for Mirror Node API hiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java |
SoloActionNetworkSettings switches to the split mirror-node accessors. |
Estimated code review effort: 4 (Complex) | ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 27.91% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | The code implements #68 by adding TopicClient subscribeTopic APIs, implementations, and tests. |
| Out of Scope Changes check | ✅ Passed | The mirror-node config updates support the new subscription flow and are not clearly unrelated. |
| Title check | ✅ Passed | The title accurately names the main change: adding subscribeTopic to TopicClient. |
| Description check | ✅ Passed | The description is on-topic and summarizes the subscribeTopic and mirror-node config changes. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d27faa52-fe3f-4ef4-8a69-21b8ade3604e
📒 Files selected for processing (25)
hiero-enterprise-base/src/main/java/org/hiero/base/TopicClient.javahiero-enterprise-base/src/main/java/org/hiero/base/config/HieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/NetworkSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaMainnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaTestnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/NetworkSettingsBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/ProtocolLayerClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageResult.javahiero-enterprise-base/src/test/java/org/hiero/base/test/TopicClientImplTest.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/HieroConfigImplTest.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroAutoConfiguration.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroConfigImpl.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroNetworkProperties.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.javahiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java
| mirrorNodeGrpcAddress = getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").orElse(null); | ||
| mirrorNodeRestUrl = getEnv("HEDERA_MIRROR_NODE_REST_URL").orElse(null); |
There was a problem hiding this comment.
Normalize blank mirror-node env vars to “unset”.
If either env var is present but empty, this stores "" as a real endpoint. That later becomes Set.of("") / Optional.of("") and propagates an invalid mirror-node configuration instead of falling back to absent values.
Proposed fix
- mirrorNodeGrpcAddress = getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").orElse(null);
- mirrorNodeRestUrl = getEnv("HEDERA_MIRROR_NODE_REST_URL").orElse(null);
+ mirrorNodeGrpcAddress =
+ getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").filter(value -> !value.isBlank()).orElse(null);
+ mirrorNodeRestUrl =
+ getEnv("HEDERA_MIRROR_NODE_REST_URL").filter(value -> !value.isBlank()).orElse(null);| if (limit < -1) { | ||
| throw new IllegalArgumentException("limit must be -1 (infinite) or greater than 0"); | ||
| } |
There was a problem hiding this comment.
Align the limit check with the documented boundary.
Line 270 accepts limit == 0, but Line 271 says the valid values are -1 or numbers greater than 0. The new tests also codify that message, so callers currently get contradictory behavior depending on whether they trust the predicate or the error text. Either reject 0 here or update the message/tests/Javadoc to >= 0.
| if (!networkSettings.getMirrorNodeGrpcAddresses().isEmpty()) { | ||
| try { | ||
| client.setMirrorNetwork(networkSettings.getMirrorNodeAddresses().stream().toList()); | ||
| client.setMirrorNetwork(networkSettings.getMirrorNodeGrpcAddresses().stream().toList()); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException("Error in configuring Mirror Node", e); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'InterruptedException|currentThread\(\)\.interrupt' \
hiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.javaRepository: hiero-ledger/hiero-enterprise-java
Length of output: 326
Restore the interrupt flag before rethrowing (InterruptedException catch in HieroTestContext).
The catch (InterruptedException e) block rethrows RuntimeException without restoring the thread’s interrupted status.
Proposed fix
try {
client.setMirrorNetwork(networkSettings.getMirrorNodeGrpcAddresses().stream().toList());
} catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
throw new RuntimeException("Error in configuring Mirror Node", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!networkSettings.getMirrorNodeGrpcAddresses().isEmpty()) { | |
| try { | |
| client.setMirrorNetwork(networkSettings.getMirrorNodeAddresses().stream().toList()); | |
| client.setMirrorNetwork(networkSettings.getMirrorNodeGrpcAddresses().stream().toList()); | |
| } catch (InterruptedException e) { | |
| throw new RuntimeException("Error in configuring Mirror Node", e); | |
| if (!networkSettings.getMirrorNodeGrpcAddresses().isEmpty()) { | |
| try { | |
| client.setMirrorNetwork(networkSettings.getMirrorNodeGrpcAddresses().stream().toList()); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new RuntimeException("Error in configuring Mirror Node", e); | |
| } | |
| } |
| @Test | ||
| void shouldThrowExceptionOnSubscribeTopicWithLimitLessThanNegOne() { | ||
| final String msg = "limit must be -1 (infinite) or greater than 0"; | ||
| // given | ||
| final TopicId topicId = TopicId.fromString("1.2.3"); | ||
| final Consumer<TopicMessage> subscription = (message) -> {}; | ||
| final Instant startTime = Instant.now().plusSeconds(120); | ||
| final Instant endTime = startTime.plusSeconds(120); | ||
| final int limit = -2; | ||
|
|
||
| final IllegalArgumentException e1 = | ||
| Assertions.assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> topicClient.subscribeTopic(topicId, subscription, limit)); | ||
| final IllegalArgumentException e2 = | ||
| Assertions.assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> topicClient.subscribeTopic(topicId, subscription, startTime, endTime, limit)); | ||
|
|
||
| Assertions.assertEquals(msg, e1.getMessage()); | ||
| Assertions.assertEquals(msg, e2.getMessage()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a regression test for limit == 0.
The new suite only covers -2, so it doesn't lock down the exact boundary where the implementation and the published error message currently diverge. A dedicated 0 case would prevent this contract from drifting again.
| final List<String> messages = new ArrayList<>(); | ||
| final TopicId topicId = topicClient.createTopic(); | ||
| hieroTestUtils.waitForMirrorNodeRecords(); | ||
|
|
||
| final SubscriptionHandle handler = | ||
| topicClient.subscribeTopic( | ||
| topicId, | ||
| (message) -> { | ||
| messages.add(new String(message.contents)); | ||
| }); | ||
|
|
||
| topicClient.submitMessage(topicId, msg); | ||
| hieroTestUtils.waitForMirrorNodeRecords(); | ||
| Thread.sleep(5000); // Make sure to wait after message get recorded in mirrornode | ||
|
|
||
| Assertions.assertNotNull(handler); | ||
| Assertions.assertEquals(1, messages.size()); | ||
| Assertions.assertEquals(msg, messages.getFirst()); |
There was a problem hiding this comment.
Synchronize the async callback before asserting.
These tests write messages from the subscription callback thread and read it from the test thread, but ArrayList plus Thread.sleep(5000) provides neither thread safety nor deterministic timing. That makes the assertions flaky and can mask missed deliveries. Use a thread-safe collection and a bounded await/latch instead.
Proposed pattern
-import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
- final List<String> messages = new ArrayList<>();
+ final List<String> messages = new CopyOnWriteArrayList<>();
+ final CountDownLatch delivered = new CountDownLatch(1);
final TopicId topicId = topicClient.createTopic();
hieroTestUtils.waitForMirrorNodeRecords();
final SubscriptionHandle handler =
topicClient.subscribeTopic(
topicId,
(message) -> {
messages.add(new String(message.contents));
+ delivered.countDown();
});
- topicClient.submitMessage(topicId, msg);
- hieroTestUtils.waitForMirrorNodeRecords();
- Thread.sleep(5000); // Make sure to wait after message get recorded in mirrornode
-
- Assertions.assertNotNull(handler);
- Assertions.assertEquals(1, messages.size());
- Assertions.assertEquals(msg, messages.getFirst());
- handler.unsubscribe();
+ try {
+ topicClient.submitMessage(topicId, msg);
+ hieroTestUtils.waitForMirrorNodeRecords();
+
+ Assertions.assertTrue(delivered.await(10, TimeUnit.SECONDS));
+ Assertions.assertEquals(1, messages.size());
+ Assertions.assertEquals(msg, messages.get(0));
+ } finally {
+ handler.unsubscribe();
+ }Also applies to: 239-260
manishdait
force-pushed
the
feat/subscribe-topic-impl
branch
from
933afda to
dc63b48
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 207adb6b-fcd5-4b2f-8de4-46e0d6a3737d
📒 Files selected for processing (25)
hiero-enterprise-base/src/main/java/org/hiero/base/TopicClient.javahiero-enterprise-base/src/main/java/org/hiero/base/config/HieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/NetworkSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaMainnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaTestnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/NetworkSettingsBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/ProtocolLayerClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageResult.javahiero-enterprise-base/src/test/java/org/hiero/base/test/TopicClientImplTest.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/HieroConfigImplTest.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroAutoConfiguration.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroConfigImpl.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroNetworkProperties.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.javahiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java
| public static TopicMessageRequest of( | ||
| @NonNull TopicId topicId, | ||
| @NonNull Consumer<TopicMessage> subscription, | ||
| @NonNull Instant startTime, | ||
| @NonNull Instant endTime, | ||
| long limit) { | ||
| return new TopicMessageRequest(topicId, subscription, startTime, endTime, limit, null, null); |
There was a problem hiding this comment.
Fix nullability contract on the 5-argument factory.
Line 54 and Line 55 are marked @NonNull, but callers pass nullable bounds for open-ended subscriptions. This breaks the cross-file contract.
Suggested fix
public static TopicMessageRequest of(
`@NonNull` TopicId topicId,
`@NonNull` Consumer<TopicMessage> subscription,
- `@NonNull` Instant startTime,
- `@NonNull` Instant endTime,
+ `@Nullable` Instant startTime,
+ `@Nullable` Instant endTime,
long limit) {
return new TopicMessageRequest(topicId, subscription, startTime, endTime, limit, null, null);
}| final String target = | ||
| hieroConfig.getMirrorNodeAddresses().stream() | ||
| .findFirst() | ||
| hieroConfig | ||
| .getMirrorNodeRestUrl() | ||
| .orElseThrow(() -> new IllegalStateException("No mirror node addresses configured")); | ||
| final MirrorNodeRestClientImpl restClient = new MirrorNodeRestClientImpl(target); |
There was a problem hiding this comment.
Reject blank REST URLs before creating the mirror-node client.
This only rejects missing values, not blank ones. A blank configured REST URL can pass and produce an invalid client target with late runtime failures.
Proposed fix
- final String target =
- hieroConfig
- .getMirrorNodeRestUrl()
- .orElseThrow(() -> new IllegalStateException("No mirror node addresses configured"));
+ final String target =
+ hieroConfig
+ .getMirrorNodeRestUrl()
+ .map(String::trim)
+ .filter(url -> !url.isBlank())
+ .orElseThrow(() -> new IllegalStateException("No mirror node REST URL configured"));| public static class MirrorNode { | ||
| @ConfigProperty(name = "hiero.network.mirrornode.rest-url") | ||
| Optional<String> restUrl = Optional.empty(); | ||
|
|
||
| @ConfigProperty(name = "hiero.network.mirrornode.grpc-addresses") | ||
| public Optional<String> grpcAddresses = Optional.empty(); | ||
|
|
||
| public Optional<String> getRestUrl() { | ||
| return restUrl; | ||
| } | ||
|
|
||
| public Set<String> getGrpcAddresses() { | ||
| // eg: mirrornode.mirrornode:443,testnet.mirrornode:5600 | ||
| return grpcAddresses | ||
| .map(n -> n.split(",")) | ||
| .map(n -> Stream.of(n)) | ||
| .orElse(Stream.empty()) | ||
| .collect(Collectors.toUnmodifiableSet()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are integration tests that verify the mirrornode config is actually read
rg -n "mirrornode" --type=java -g '**/*Test*.java' -C3Repository: hiero-ledger/hiero-enterprise-java
Length of output: 20992
🌐 Web query:
MicroProfile Config @ConfigProperty nested object injection
💡 Result:
MicroProfile Config does not support direct injection of nested objects using the @ConfigProperty annotation [1][2]. The @ConfigProperty annotation is designed to inject scalar values, arrays, or collections (like List or Set) into individual injection points, provided a suitable Converter exists for the target type [1][2]. To group related configuration properties into a structured object or "nested" configuration, you should use the @ConfigProperties annotation instead of @ConfigProperty [3][4]. When you annotate a POJO with @ConfigProperties(prefix = "my.prefix"), MicroProfile Config automatically maps fields within that class to configuration keys starting with that prefix [4][5]. If a field in your POJO is itself a POJO, some implementations (such as Quarkus) provide extended support for nesting these objects using the same @ConfigProperties mechanism [6]. Key differences for configuration injection: 1. @ConfigProperty: Used for injecting individual, flat properties [1][2]. It supports types with registered Converters, Optional, Provider, and collections [1][2]. 2. @ConfigProperties: Used to aggregate multiple related properties into a single CDI bean or POJO [3][4]. This is the standard way to handle structured configuration [3][4]. If you are working in an environment like Quarkus, you can define nested structures by nesting classes and using @ConfigProperties, as demonstrated in common implementation guides [6]. For pure MicroProfile implementations, you may need to ensure your configuration structure is flattened or use @ConfigProperties to bind a group of properties to a bean [3][4].
Citations:
- 1: https://download.eclipse.org/microprofile/microprofile-config-3.1/microprofile-config-spec-3.1.html
- 2: https://download.eclipse.org/microprofile/microprofile-config-3.1/apidocs/org/eclipse/microprofile/config/inject/ConfigProperty.html
- 3: https://github.com/OpenLiberty/draft-guide-microprofile-config-apis
- 4: https://download.eclipse.org/microprofile/microprofile-config-3.0.2/microprofile-config-spec-3.0.2.html
- 5: https://download.eclipse.org/microprofile/microprofile-config-3.1-RC2/microprofile-config-spec-3.1-RC2.html
- 6: https://docs.redhat.com/en/documentation/red_hat_build_of_quarkus/1.11/html/configuring_your_quarkus_applications/proc-injecting-configuration-value-in-your-quarkus-application_quarkus-configuration-guide
🏁 Script executed:
cat -n hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.java | head -100Repository: hiero-ledger/hiero-enterprise-java
Length of output: 2976
🏁 Script executed:
# Search for property files containing mirrornode config
find . -type f \( -name "*.properties" -o -name "*.yaml" -o -name "*.yml" \) | xargs grep -l "mirrornode" 2>/dev/null | head -10Repository: hiero-ledger/hiero-enterprise-java
Length of output: 60
🏁 Script executed:
# Search for actual usage of getMirrornode() or getRestUrl/getGrpcAddresses
rg "getMirrornode|getRestUrl|getGrpcAddresses" --type=java -A2 -B2Repository: hiero-ledger/hiero-enterprise-java
Length of output: 6869
🏁 Script executed:
cat -n hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.java | grep -A10 -B5 "getMirrornode"Repository: hiero-ledger/hiero-enterprise-java
Length of output: 965
🏁 Script executed:
# Check for any microprofile-config.properties or application.properties in the microprofile module
find hiero-enterprise-microprofile -type f \( -name "*.properties" -o -name "*.yaml" \) | xargs cat 2>/dev/nullRepository: hiero-ledger/hiero-enterprise-java
Length of output: 120
🏁 Script executed:
# Check the HieroConfigImplTest to understand how the microprofile config is tested
cat -n hiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/HieroConfigImplTest.java | head -150Repository: hiero-ledger/hiero-enterprise-java
Length of output: 3787
🏁 Script executed:
# Search for any test configuration or property files in test resources
find hiero-enterprise-microprofile/src/test -type f \( -name "*.properties" -o -name "*.yaml" -o -name "*.xml" \) | xargs ls -laRepository: hiero-ledger/hiero-enterprise-java
Length of output: 315
🏁 Script executed:
# Check what MicroProfile Config implementation is being used
rg "microprofile-config" hiero-enterprise-microprofile/pom.xml -A2 -B2Repository: hiero-ledger/hiero-enterprise-java
Length of output: 255
🏁 Script executed:
# Look for how the HieroNetworkConfiguration is actually injected/used in the microprofile module
rg "HieroNetworkConfiguration" hiero-enterprise-microprofile/src/main --type=java -B3 -A3Repository: hiero-ledger/hiero-enterprise-java
Length of output: 3811
🏁 Script executed:
# Check the ClientProvider to see the full injection setup
cat -n hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.javaRepository: hiero-ledger/hiero-enterprise-java
Length of output: 8820
🏁 Script executed:
# Look for any actual configuration examples or test setup that sets mirrornode properties
rg "hiero.network.mirrornode" --type=java --type=properties --type=xml -B2 -A2Repository: hiero-ledger/hiero-enterprise-java
Length of output: 113
🏁 Script executed:
# Search for property definitions with the full property names
rg "rest-url|grpc-addresses" --type-list | head -20Repository: hiero-ledger/hiero-enterprise-java
Length of output: 468
🏁 Script executed:
# Try a broader search for property files and configuration
find . -path ./target -prune -o -type f \( -name "*.properties" -o -name "*.yaml" -o -name "*.yml" \) -print | xargs grep -l "hiero.network" 2>/dev/nullRepository: hiero-ledger/hiero-enterprise-java
Length of output: 375
🏁 Script executed:
# Check the sample and test property files for mirrornode configuration
for file in ./hiero-enterprise-microprofile-sample/src/main/resources/application.properties ./hiero-enterprise-spring-sample/src/main/resources/application.properties ./hiero-enterprise-spring/src/test/resources/application-solo.properties ./hiero-enterprise-spring/src/test/resources/application.properties; do
echo "=== $file ==="
cat "$file"
echo ""
doneRepository: hiero-ledger/hiero-enterprise-java
Length of output: 1461
🏁 Script executed:
# Check if there's a test that actually sets mirrornode properties and expects them to be read
rg "hiero.network.mirrornode" -i --type=java --type=xml --type=yamlRepository: hiero-ledger/hiero-enterprise-java
Length of output: 408
🏁 Script executed:
# Check the actual Quarkus/MicroProfile Config behavior in Quarkus documentation comments or test setup
find . -name "*Test*.java" -o -name "*Producer*.java" | xargs grep -l "ConfigProperties" | head -5 | xargs cat | head -200Repository: hiero-ledger/hiero-enterprise-java
Length of output: 60
@ConfigProperty annotations on MirrorNode fields will not be injected.
The MirrorNode class is instantiated manually via new MirrorNode() on line 27. While the parent HieroNetworkConfiguration is a CDI bean with @ConfigProperties, MicroProfile Config does not automatically apply configuration binding to manually instantiated nested objects. The restUrl and grpcAddresses fields will remain their default Optional.empty() values.
To enable configuration injection for nested properties, consider one of these approaches:
- Add
@ConfigProperties(prefix = "hiero.network.mirrornode")to theMirrorNodeclass and make it a CDI bean - Inject
MirrorNodeas a CDI dependency instead of instantiating it manually - Read properties manually via
ConfigProvider.getConfig().getOptionalValue(...)
| mirrorNodeGrpcAddresses = | ||
| networkConfiguration.getMirrornode().getGrpcAddresses() == null | ||
| ? Set.of() | ||
| : Set.copyOf(networkConfiguration.getMirrornode().getGrpcAddresses()); | ||
| mirrorNodeRestUrl = networkConfiguration.getMirrornode().getRestUrl(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant null check, but safe.
getGrpcAddresses() returns an empty Set when grpcAddresses is empty (see line 77 in HieroNetworkConfiguration.MirrorNode), so the == null check on line 66 will never be true. However, this is defensive coding and not harmful.
Note: This code path depends on the MirrorNode configuration injection working correctly (see related comment on HieroNetworkConfiguration).
| @Override | ||
| public Set<String> getMirrorNodeAddresses() { | ||
| return mirrorNodeAddresses; | ||
| public Set<String> getMirrorNodeGrpcAddresses() { | ||
| return mirrorNodeGrpcAddresses; | ||
| } |
There was a problem hiding this comment.
Missing @NonNull annotation on getMirrorNodeGrpcAddresses().
The HieroConfig interface declares @NonNull Set<String> getMirrorNodeGrpcAddresses(). This implementation should include the annotation for consistency and to satisfy static analysis tools.
Suggested fix
`@Override`
- public Set<String> getMirrorNodeGrpcAddresses() {
+ public `@NonNull` Set<String> getMirrorNodeGrpcAddresses() {
return mirrorNodeGrpcAddresses;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public Set<String> getMirrorNodeAddresses() { | |
| return mirrorNodeAddresses; | |
| public Set<String> getMirrorNodeGrpcAddresses() { | |
| return mirrorNodeGrpcAddresses; | |
| } | |
| `@Override` | |
| public `@NonNull` Set<String> getMirrorNodeGrpcAddresses() { | |
| return mirrorNodeGrpcAddresses; | |
| } |
Signed-off-by: Manish Dait <daitmanish88@gmail.com> # Conflicts: # hiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.java # hiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com> # Conflicts: # hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.java
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
manishdait
force-pushed
the
feat/subscribe-topic-impl
branch
from
dc63b48 to
e36529f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (8)
hiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.java (1)
271-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
limit == 0boundary contradicts the documented/thrown error message.
limit < -1letslimit == 0pass validation, but the error message (andTopicClientJavadoc/tests) says only-1(infinite) or values> 0are valid. Downstream,ProtocolLayerClientImpl.executeTopicMessageQuerycallsquery.setLimit(request.limit())wheneverlimit() >= 0, so a0limit is forwarded straight to the Hedera SDK'sTopicMessageQuery.setLimit, whose behavior for0isn't verified here — it may silently deliver zero messages, contradicting caller expectations.🔧 Proposed fix
- if (limit < -1) { + if (limit < -1 || limit == 0) { throw new IllegalArgumentException("limit must be -1 (infinite) or greater than 0"); }What does Hedera Java SDK TopicMessageQuery.setLimit(0) do — unlimited messages or zero messages?hiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.java (1)
39-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFlaky async assertions: unsynchronized
ArrayList+ fixedThread.sleep.
messagesis written from the subscription callback thread and read from the test thread with no synchronization, and correctness is gated on a fixedThread.sleep(5000)rather than a deterministic wait. This is the same pattern already flagged (by a human reviewer here, and by an automated review on the equivalent Spring test) as flaky/non-deterministic.🔧 Suggested pattern
-import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; ... - final List<String> messages = new ArrayList<>(); + final List<String> messages = new CopyOnWriteArrayList<>(); + final CountDownLatch delivered = new CountDownLatch(1); ... final SubscriptionHandle handler = topicClient.subscribeTopic( topicId, (message) -> { messages.add(new String(message.contents)); + delivered.countDown(); }); - topicClient.submitMessage(topicId, msg); - hieroTestUtils.waitForMirrorNodeRecords(); - Thread.sleep(5000); - - Assertions.assertNotNull(handler); - Assertions.assertEquals(1, messages.size()); - Assertions.assertEquals(msg, messages.getFirst()); - handler.unsubscribe(); + try { + topicClient.submitMessage(topicId, msg); + hieroTestUtils.waitForMirrorNodeRecords(); + Assertions.assertTrue(delivered.await(10, TimeUnit.SECONDS)); + Assertions.assertEquals(1, messages.size()); + Assertions.assertEquals(msg, messages.get(0)); + } finally { + handler.unsubscribe(); + }hiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.java (1)
209-262: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnresolved: async callback race + flaky fixed-delay wait.
This repeats a previously raised concern:
messages(ArrayList) is mutated from the subscription callback thread and read from the test thread without synchronization, and the test relies on a fixedThread.sleep(5000)instead of a deterministic wait/latch. This remains unaddressed intestSubscribeTopic(Lines 213-232) andtestSubscribeTopicWithLimit(Lines 239-262).🔧 Suggested pattern
-import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; ... - final List<String> messages = new ArrayList<>(); + final List<String> messages = new CopyOnWriteArrayList<>(); + final CountDownLatch delivered = new CountDownLatch(1); ... (message) -> { messages.add(new String(message.contents)); + delivered.countDown(); }); - topicClient.submitMessage(topicId, msg); - hieroTestUtils.waitForMirrorNodeRecords(); - Thread.sleep(5000); - - Assertions.assertNotNull(handler); - Assertions.assertEquals(1, messages.size()); - Assertions.assertEquals(msg, messages.getFirst()); - handler.unsubscribe(); + try { + topicClient.submitMessage(topicId, msg); + hieroTestUtils.waitForMirrorNodeRecords(); + Assertions.assertTrue(delivered.await(10, TimeUnit.SECONDS)); + Assertions.assertEquals(1, messages.size()); + Assertions.assertEquals(msg, messages.get(0)); + } finally { + handler.unsubscribe(); + }hiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.java (1)
50-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNullability contract broken again on the 5-arg factory.
startTime/endTimeare annotated@NonNullhere, butTopicClientImpl.subscribeTopic(topicId, handler, limit)delegates tosubscribeTopic(topicId, handler, null, null, limit), which calls exactly this overload withnullfor both. This is the same issue flagged on a previous commit (then at lines 51-57); it has resurfaced at the new line numbers.🐛 Proposed fix
`@NonNull` public static TopicMessageRequest of( `@NonNull` TopicId topicId, `@NonNull` Consumer<TopicMessage> subscription, - `@NonNull` Instant startTime, - `@NonNull` Instant endTime, + `@Nullable` Instant startTime, + `@Nullable` Instant endTime, long limit) { return new TopicMessageRequest(topicId, subscription, startTime, endTime, limit, null, null); }hiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.java (1)
42-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStill unaddressed: blank env vars propagate invalid endpoints.
getEnv()returnsOptional.ofNullable(System.getenv(key)). When an env var is set but blank (""),System.getenvreturns""(notnull), so.orElse(null)stores"". This becomesSet.of("")ingetMirrorNodeGrpcAddresses()andOptional.of("")ingetMirrorNodeRestUrl(), propagating an invalid mirror-node configuration instead of falling back to absent values.This was previously flagged and remains unaddressed.
Proposed fix
- mirrorNodeGrpcAddress = getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").orElse(null); - mirrorNodeRestUrl = getEnv("HEDERA_MIRROR_NODE_REST_URL").orElse(null); + mirrorNodeGrpcAddress = + getEnv("HEDERA_MIRROR_NODE_GRPC_ADDRESS").filter(v -> !v.isBlank()).orElse(null); + mirrorNodeRestUrl = + getEnv("HEDERA_MIRROR_NODE_REST_URL").filter(v -> !v.isBlank()).orElse(null);hiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.java (1)
74-75: 🩺 Stability & Availability | 🟡 Minor | 💤 Low valueStill unaddressed: interrupt flag not restored before rethrowing.
The
catch (InterruptedException e)block rethrowsRuntimeExceptionwithout callingThread.currentThread().interrupt(), losing the thread's interrupted status. This was previously flagged and remains unaddressed.Proposed fix
try { client.setMirrorNetwork(networkSettings.getMirrorNodeGrpcAddresses().stream().toList()); } catch (InterruptedException e) { + Thread.currentThread().interrupt(); throw new RuntimeException("Error in configuring Mirror Node", e); }hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.java (1)
24-28: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftStill unaddressed (critical):
@ConfigPropertyon manually instantiatedMirrorNodewill not be injected.
MirrorNodeis created vianew MirrorNode()on line 27. MicroProfile Config only processes@ConfigPropertyon CDI-managed beans, not on manually instantiated objects. TherestUrlandgrpcAddressesfields will remain at their defaultOptional.empty()values.Downstream impact:
HieroConfigImpl(lines 65-69) receives an emptySetandOptional.empty()for custom network configurations, silently disabling mirror-node gRPC and REST connectivity.This was previously flagged as critical and remains unaddressed.
Proposed fix — inject MirrorNode as a CDI bean
+ `@Dependent` public static class MirrorNode { - `@ConfigProperty`(name = "hiero.network.mirrornode.rest-url") + `@ConfigProperty`(name = "mirrornode.rest-url") Optional<String> restUrl = Optional.empty(); - `@ConfigProperty`(name = "hiero.network.mirrornode.grpc-addresses") + `@ConfigProperty`(name = "mirrornode.grpc-addresses") public Optional<String> grpcAddresses = Optional.empty();And in
HieroNetworkConfiguration:- public MirrorNode mirrorNode = new MirrorNode(); + `@Inject` MirrorNode mirrorNode;Also applies to: 60-80
hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.java (1)
65-69: 📐 Maintainability & Code Quality | 🟠 Major | 💤 Low valueStill unaddressed: redundant null check on
getGrpcAddresses().
getGrpcAddresses()returnsCollectors.toUnmodifiableSet()which is nevernull— it returns an emptySetwhengrpcAddressesisOptional.empty(). The== nullcheck on line 66 will never be true.Set.copyOfon line 68 also creates a redundant copy of an already-unmodifiable set.This was previously flagged and remains unaddressed.
Proposed fix
networkName = networkConfiguration.getName().orElse(null); mirrorNodeGrpcAddresses = - networkConfiguration.getMirrornode().getGrpcAddresses() == null - ? Set.of() - : Set.copyOf(networkConfiguration.getMirrornode().getGrpcAddresses()); + Collections.unmodifiableSet(networkConfiguration.getMirrornode().getGrpcAddresses()); mirrorNodeRestUrl = networkConfiguration.getMirrornode().getRestUrl();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 196085ee-a2dc-4adc-acf9-9b4e71ddc58c
📒 Files selected for processing (25)
hiero-enterprise-base/src/main/java/org/hiero/base/TopicClient.javahiero-enterprise-base/src/main/java/org/hiero/base/config/HieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/NetworkSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaMainnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/hedera/HederaTestnetSettings.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/EnvBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/config/implementation/NetworkSettingsBasedHieroConfig.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/ProtocolLayerClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/implementation/TopicClientImpl.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageRequest.javahiero-enterprise-base/src/main/java/org/hiero/base/protocol/data/TopicMessageResult.javahiero-enterprise-base/src/test/java/org/hiero/base/test/TopicClientImplTest.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/HieroTestContext.javahiero-enterprise-base/src/test/java/org/hiero/base/test/config/SoloActionNetworkSettings.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/ClientProvider.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/HieroNetworkConfiguration.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/HieroConfigImpl.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/HieroConfigImplTest.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/TopicClientTest.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroAutoConfiguration.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroConfigImpl.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/HieroNetworkProperties.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicClientTest.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/TopicRepositoryTest.javahiero-enterprise-test/src/main/java/org/hiero/test/SoloActionNetworkSettings.java
Description:
This PR introduces
subscribeTopic()method to the TopicClient interface, which help to subscribe and listen to the messages received by the specific Hedera Topic.Key Changes:
getMirrornodeRestUrl()andgetMirrornodeGrpcs()to NetworkSettings to store the HCS gRPC endpoint to set the mirrorNodeAddress in Client becauseTopicMessageQueryuses the gRPC protocol.Method Added:
Related issue(s):
Fixes #68
Notes for reviewer:
The current implemetation will break the backward compatiblity with as the env names has been changed.
Checklist