Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request refactors terminology throughout the codebase, systematically replacing "robot" account references with "client" account references. Changes span configuration files, Kubernetes manifests, Java source code, tests, documentation, and deployment scripts. Changes
Poem
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
cert-showcase/docker-compose.yml (1)
65-67: ParameterizeHUB_AUTH_CLIENT_IDfor safer environment reuse.This is probably not a secret, but hardcoding environment-specific IDs in tracked compose files makes reuse/rotation harder.
♻️ Suggested change
- HUB_AUTH_CLIENT_ID: "ce84b6a8-df3d-451b-8411-4fb57570c47a" + HUB_AUTH_CLIENT_ID: "${HUB_AUTH_CLIENT_ID:?set HUB_AUTH_CLIENT_ID}" HUB_AUTH_CLIENT_SECRET_FILE: /run/secrets/client-secret-node.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cert-showcase/docker-compose.yml` around lines 65 - 67, Replace the hardcoded HUB_AUTH_CLIENT_ID value with a parameterized reference (e.g., use an environment variable interpolation like ${HUB_AUTH_CLIENT_ID}) and ensure the compose runtime reads it from a .env or an external secret management mechanism; update the docker-compose service definition that currently sets HUB_AUTH_CLIENT_ID to the literal "ce84b6a8-df3d-451b-8411-4fb57570c47a" so it instead references the external variable, and add a note to repository docs or example .env showing the expected variable name and how to provide it (or switch to a docker secret if preferred) so deployments can rotate/change the ID without editing tracked files.src/main/java/de/privateaim/node_message_broker/common/CommonSpringConfig.java (2)
197-204: Parameter namehubAuthRobotSecretshould behubAuthClientSecretfor consistency.The qualifier correctly references
HUB_AUTH_CLIENT_SECRET, but the parameter name still uses "Robot" terminology.♻️ Proposed fix
`@Qualifier`("HUB_AUTHENTICATOR") `@Bean` OIDCAuthenticator hubAuthenticator( `@Qualifier`("HUB_AUTH_WEB_CLIENT") WebClient webClient, `@Qualifier`("HUB_EXCHANGE_RETRY_CONFIG") HttpRetryConfig retryConfig, `@Qualifier`("HUB_AUTH_CLIENT_ID") String hubAuthClientId, - `@Qualifier`("HUB_AUTH_CLIENT_SECRET") String hubAuthRobotSecret, + `@Qualifier`("HUB_AUTH_CLIENT_SECRET") String hubAuthClientSecret, `@Qualifier`("HUB_JSON_MAPPER") ObjectMapper jsonMapper ) { return HubOIDCAuthenticator.builder() .usingWebClient(webClient) .withRetryConfig(retryConfig) - .withAuthCredentials(hubAuthClientId, hubAuthRobotSecret) + .withAuthCredentials(hubAuthClientId, hubAuthClientSecret) .withJsonDecoder(jsonMapper) .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/de/privateaim/node_message_broker/common/CommonSpringConfig.java` around lines 197 - 204, Rename the method parameter currently declared as hubAuthRobotSecret to hubAuthClientSecret to match the `@Qualifier`("HUB_AUTH_CLIENT_SECRET") and maintain consistent naming; update all references in the CommonSpringConfig method signature and its usage (the builder call that passes this value to HubOIDCAuthenticator.withAuthCredentials) so the new parameter name is used everywhere.
65-69: Inconsistent method name:hubAuthRobotSecret()should behubAuthClientSecret().The bean qualifier was updated to
HUB_AUTH_CLIENT_SECRET, but the method name still references "Robot". For consistency with the refactoring, consider renaming the method.♻️ Proposed fix
`@Qualifier`("HUB_AUTH_CLIENT_SECRET") `@Bean` - public String hubAuthRobotSecret() throws IOException { + public String hubAuthClientSecret() throws IOException { return new String(ConfigurationUtil.readExternalFileContent(hubAuthClientSecretFile)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/de/privateaim/node_message_broker/common/CommonSpringConfig.java` around lines 65 - 69, Rename the bean factory method hubAuthRobotSecret() in CommonSpringConfig to hubAuthClientSecret() so its name matches the `@Qualifier`("HUB_AUTH_CLIENT_SECRET") and the intended refactor; update all internal references/usages to the new method name and keep the signature (throws IOException) and implementation (return new String(ConfigurationUtil.readExternalFileContent(hubAuthClientSecretFile))) unchanged to preserve the bean behavior.src/main/java/de/privateaim/node_message_broker/message/MessageService.java (1)
121-122: Align error log wording withclientIdterminology.Line 121 still says “to node” while the logged identifier is a client id.
Suggested tweak
- .doOnError(err -> log.error("emitting message `{}` to node `{}` failed", + .doOnError(err -> log.error("emitting message `{}` to client `{}` failed", msg.context().messageId(), msg.recipient().nodeClientId(), err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/de/privateaim/node_message_broker/message/MessageService.java` around lines 121 - 122, Update the log message in MessageService where the reactive chain uses .doOnError(...) to emit failures: change the wording from "emitting message `{}` to node `{}` failed" to use the clientId terminology, e.g. "emitting message `{}` to client `{}` failed", while keeping the same parameters msg.context().messageId() and msg.recipient().nodeClientId() so the logged identifier aligns with clientId naming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.MD`:
- Around line 34-35: Update the description for the environment variable
HUB_AUTH_CLIENT_ID in the README: replace the current text "Robot ID associated
with the node." with "Client ID associated with the node." so the variable name
and description are consistent; ensure you change only the description text for
HUB_AUTH_CLIENT_ID and keep the surrounding table formatting intact.
In
`@src/main/java/de/privateaim/node_message_broker/discovery/DiscoveryService.java`:
- Around line 80-82: The filter that builds selfParticipants in DiscoveryService
is not null-safe and can NPE when a Participant has a null clientId; update the
predicate used in participants.stream().filter(...) (the selfParticipants
construction) to a null-safe check such as using Objects.equals(selfClientId,
p.clientId()) or checking p.clientId() != null &&
p.clientId().equals(selfClientId) so comparisons never call equals on a null
value.
In `@src/main/java/de/privateaim/node_message_broker/message/MessageService.java`:
- Around line 129-132: The stream in MessageService that builds
AnalysisParticipant objects and filters by selfClientId assumes
participant.clientId is non-null; update the pipeline to defensively exclude
participants with null or blank clientId before comparing to selfClientId (i.e.,
filter out entries where AnalysisParticipant.clientId is null or empty/blank),
so the comparison participant.clientId.equals(selfClientId) cannot NPE and
invalid recipients are prevented; modify the lambda around new
AnalysisParticipant(...) and the subsequent .filter(...) to perform the
null/blank check (or normalize clientId) prior to equality comparison and
collecting.
In `@system-tests/environment/hub/hub-docker-compose.yml`:
- Line 84: The environment variable CLIENT_ADMIN_ENABLED is invalid for Authup;
replace it with the correct variable (either ROBOT_ADMIN_ENABLED to restore the
previous robot-based admin behavior or USER_ADMIN_ENABLED for user-based admin
accounts). Locate the docker-compose service env block that sets
CLIENT_ADMIN_ENABLED and change that key to the chosen valid symbol
(ROBOT_ADMIN_ENABLED or USER_ADMIN_ENABLED) while preserving the boolean value
and formatting.
In `@system-tests/environment/resources/hub/robot-account-setup-template.json`:
- Around line 1-6: Rename the JSON template file from
robot-account-setup-template.json to client-account-setup-template.json so the
setup script setup-hub-resources.sh (which expects
client-account-setup-template.json at line 34) can find it; ensure the file
content remains unchanged (it already uses <CLIENT_SECRET> and <CLIENT_NAME>)
and update any other references if present to the new filename to keep naming
consistent with the script.
In `@system-tests/environment/setup.sh`:
- Around line 48-52: The current pipeline writes the processed template into
"$BASE_DIR"/node/node-docker-compose.yml but ends the line with a redirection
followed by a pipe to the docker compose command, breaking the pipe and hiding
write errors; update the sequence so the sed pipeline writes the file first and
only then runs docker compose (use a conditional operator like &&) so that the
sed commands targeting node-docker-compose.tpl.yml and producing
node-docker-compose.yml must succeed before invoking docker compose -f
"$BASE_DIR"/node/node-docker-compose.yml up --build -d.
---
Nitpick comments:
In `@cert-showcase/docker-compose.yml`:
- Around line 65-67: Replace the hardcoded HUB_AUTH_CLIENT_ID value with a
parameterized reference (e.g., use an environment variable interpolation like
${HUB_AUTH_CLIENT_ID}) and ensure the compose runtime reads it from a .env or an
external secret management mechanism; update the docker-compose service
definition that currently sets HUB_AUTH_CLIENT_ID to the literal
"ce84b6a8-df3d-451b-8411-4fb57570c47a" so it instead references the external
variable, and add a note to repository docs or example .env showing the expected
variable name and how to provide it (or switch to a docker secret if preferred)
so deployments can rotate/change the ID without editing tracked files.
In
`@src/main/java/de/privateaim/node_message_broker/common/CommonSpringConfig.java`:
- Around line 197-204: Rename the method parameter currently declared as
hubAuthRobotSecret to hubAuthClientSecret to match the
`@Qualifier`("HUB_AUTH_CLIENT_SECRET") and maintain consistent naming; update all
references in the CommonSpringConfig method signature and its usage (the builder
call that passes this value to HubOIDCAuthenticator.withAuthCredentials) so the
new parameter name is used everywhere.
- Around line 65-69: Rename the bean factory method hubAuthRobotSecret() in
CommonSpringConfig to hubAuthClientSecret() so its name matches the
`@Qualifier`("HUB_AUTH_CLIENT_SECRET") and the intended refactor; update all
internal references/usages to the new method name and keep the signature (throws
IOException) and implementation (return new
String(ConfigurationUtil.readExternalFileContent(hubAuthClientSecretFile)))
unchanged to preserve the bean behavior.
In `@src/main/java/de/privateaim/node_message_broker/message/MessageService.java`:
- Around line 121-122: Update the log message in MessageService where the
reactive chain uses .doOnError(...) to emit failures: change the wording from
"emitting message `{}` to node `{}` failed" to use the clientId terminology,
e.g. "emitting message `{}` to client `{}` failed", while keeping the same
parameters msg.context().messageId() and msg.recipient().nodeClientId() so the
logged identifier aligns with clientId naming.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
README.MDcert-showcase/README.mdcert-showcase/docker-compose.ymlk8s/README.mdk8s/deploy-to-minikube.shk8s/manifests/broker-deployment.ymlk8s/manifests/hub-auth-secret.ymlsrc/main/java/de/privateaim/node_message_broker/common/CommonSpringConfig.javasrc/main/java/de/privateaim/node_message_broker/common/hub/HttpHubClient.javasrc/main/java/de/privateaim/node_message_broker/common/hub/HubClient.javasrc/main/java/de/privateaim/node_message_broker/common/hub/api/Node.javasrc/main/java/de/privateaim/node_message_broker/common/hub/auth/HubOIDCAuthenticator.javasrc/main/java/de/privateaim/node_message_broker/discovery/DiscoveryService.javasrc/main/java/de/privateaim/node_message_broker/discovery/DiscoverySpringConfig.javasrc/main/java/de/privateaim/node_message_broker/discovery/Participant.javasrc/main/java/de/privateaim/node_message_broker/message/MessageService.javasrc/main/java/de/privateaim/node_message_broker/message/MessageSpringConfig.javasrc/main/java/de/privateaim/node_message_broker/message/api/hub/HubMessageRecipient.javasrc/main/java/de/privateaim/node_message_broker/message/api/hub/HubMessageSender.javasrc/main/java/de/privateaim/node_message_broker/message/emit/EmitMessageRecipient.javasrc/main/java/de/privateaim/node_message_broker/message/emit/HubMessageEmitter.javasrc/main/java/de/privateaim/node_message_broker/message/emit/HubMessageEncryptionMiddleware.javasrc/main/java/de/privateaim/node_message_broker/message/receive/HubMessageDecryptionMiddleware.javasrc/main/java/de/privateaim/node_message_broker/message/receive/HubMessageReceiver.javasrc/main/java/de/privateaim/node_message_broker/message/receive/ReceiveMessageSender.javasrc/main/resources/application.ymlsrc/test/java/de/privateaim/node_message_broker/common/hub/HttpHubClientIT.javasrc/test/java/de/privateaim/node_message_broker/discovery/DiscoveryControllerIT.javasrc/test/java/de/privateaim/node_message_broker/discovery/DiscoveryServiceIT.javasrc/test/java/de/privateaim/node_message_broker/discovery/DiscoveryServiceTest.javasrc/test/java/de/privateaim/node_message_broker/message/MessageServiceIT.javasrc/test/java/de/privateaim/node_message_broker/message/MessageServiceTest.javasrc/test/java/de/privateaim/node_message_broker/message/emit/HubMessageEmitterTest.javasrc/test/java/de/privateaim/node_message_broker/message/emit/HubMessageEncryptionMiddlewareTest.javasrc/test/java/de/privateaim/node_message_broker/message/receive/HubMessageDecryptionMiddlewareTest.javasrc/test/java/de/privateaim/node_message_broker/message/receive/HubMessageReceiverTest.javasystem-tests/README.mdsystem-tests/environment/hub/hub-docker-compose.ymlsystem-tests/environment/node/node-docker-compose.tpl.ymlsystem-tests/environment/resources/hub/node-setup-template.jsonsystem-tests/environment/resources/hub/robot-account-setup-template.jsonsystem-tests/environment/resources/hub/setup-hub-resources.shsystem-tests/environment/resources/secrets/setup-secrets.shsystem-tests/environment/setup.sh
Comment on lines
80
to
82
| var selfParticipants = participants.stream() | ||
| .filter(p -> p.robotId().equals(selfRobotId)) | ||
| .filter(p -> p.clientId().equals(selfClientId)) | ||
| .toList(); |
There was a problem hiding this comment.
Make self-discovery filter null-safe.
On Line 81, p.clientId().equals(selfClientId) can throw if hub data contains a participant with a null clientId. This turns discovery into an unexpected 500 path.
Suggested fix
- var selfParticipants = participants.stream()
- .filter(p -> p.clientId().equals(selfClientId))
- .toList();
+ var selfParticipants = participants.stream()
+ .filter(p -> p.clientId() != null)
+ .filter(p -> selfClientId.equals(p.clientId()))
+ .toList();📝 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.
Suggested change
| var selfParticipants = participants.stream() | |
| .filter(p -> p.robotId().equals(selfRobotId)) | |
| .filter(p -> p.clientId().equals(selfClientId)) | |
| .toList(); | |
| var selfParticipants = participants.stream() | |
| .filter(p -> p.clientId() != null) | |
| .filter(p -> selfClientId.equals(p.clientId())) | |
| .toList(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/de/privateaim/node_message_broker/discovery/DiscoveryService.java`
around lines 80 - 82, The filter that builds selfParticipants in
DiscoveryService is not null-safe and can NPE when a Participant has a null
clientId; update the predicate used in participants.stream().filter(...) (the
selfParticipants construction) to a null-safe check such as using
Objects.equals(selfClientId, p.clientId()) or checking p.clientId() != null &&
p.clientId().equals(selfClientId) so comparisons never call equals on a null
value.
Comment on lines
129
to
132
| .map(nodes -> nodes.stream() | ||
| .map(n -> new AnalysisParticipant(n.node.id, n.node.robotId)) | ||
| .filter(participant -> !participant.robotId.equals(selfRobotId)) | ||
| .map(n -> new AnalysisParticipant(n.node.id, n.node.clientId)) | ||
| .filter(participant -> !participant.clientId.equals(selfClientId)) | ||
| .collect(Collectors.toSet())); |
There was a problem hiding this comment.
Defensively handle missing clientId from hub participant data.
On Line 131, dereferencing participant.clientId assumes hub data is always complete. A null/blank clientId can cause runtime failure or invalid emit recipients.
Suggested fix
return hubClient.fetchAnalysisNodes(analysisId)
.map(nodes -> nodes.stream()
.map(n -> new AnalysisParticipant(n.node.id, n.node.clientId))
- .filter(participant -> !participant.clientId.equals(selfClientId))
+ .filter(participant -> participant.clientId() != null && !participant.clientId().isBlank())
+ .filter(participant -> !selfClientId.equals(participant.clientId()))
.collect(Collectors.toSet()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/de/privateaim/node_message_broker/message/MessageService.java`
around lines 129 - 132, The stream in MessageService that builds
AnalysisParticipant objects and filters by selfClientId assumes
participant.clientId is non-null; update the pipeline to defensively exclude
participants with null or blank clientId before comparing to selfClientId (i.e.,
filter out entries where AnalysisParticipant.clientId is null or empty/blank),
so the comparison participant.clientId.equals(selfClientId) cannot NPE and
invalid recipients are prevented; modify the lambda around new
AnalysisParticipant(...) and the subsequent .filter(...) to perform the
null/blank check (or normalize clientId) prior to equality comparison and
collecting.
system-tests/environment/resources/hub/client-account-setup-template.json
Show resolved
Hide resolved
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
1 participant
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.
Refactor robot references to client
Summary by CodeRabbit
Release Notes
Configuration Changes
Documentation