Move Stanag to dedicated repo#2123
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Add a ServiceLoader-based state adapter SPI and move protocol-specific STANAG handling out to mapsmessaging-stanag-state-adapter. State adapter configuration now lives under an explicit stateAdapters section, keyed by adapter name. The server keeps that configuration opaque and adapters parse their own blocks. Expose generic audit primitives through StateAuditContext so adapters can construct protocol-specific auditors without server-owned STANAG types.
Add focused coverage for the explicit stateAdapters configuration map and generic audit context factory behavior.
b75e923 to
3f3b3ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/test/java/io/mapsmessaging/state/config/TwinManagerConfigTest.java (2)
98-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen
update_copiesAdapterConfigMapto validate map isolation explicitly.The test currently validates content but not aliasing. Add assertions that the map instance differs (
assertNotSame) and that mutatingnewConfig.getAdapterConfig()afterupdatedoes not mutateconfig.getAdapterConfig().Proposed test hardening
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; @@ void update_copiesAdapterConfigMap() { @@ boolean changed = config.update(newConfig); @@ assertTrue(changed); + assertNotSame(newConfig.getAdapterConfig(), config.getAdapterConfig()); assertEquals(1, config.getAdapterConfig().size()); assertSame(adapter, config.getAdapterConfig().get("alphaAdapter")); + + newConfig.getAdapterConfig().put("bravoAdapter", adapterProperties("bravo/topic", 22)); + assertEquals(1, config.getAdapterConfig().size()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/io/mapsmessaging/state/config/TwinManagerConfigTest.java` around lines 98 - 111, The update_copiesAdapterConfigMap test currently validates that adapter configuration content is copied but does not verify that the maps are truly isolated instances. Add an assertNotSame assertion to confirm that the adapter config map in config is a different instance than the map in newConfig, and then add a mutation to newConfig.getAdapterConfig() after the update call followed by assertions that verify the config.getAdapterConfig() remains unchanged, ensuring the maps are properly isolated and not sharing references.
33-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate test naming/base-class usage to match project standards.
Rename test methods to snake_case and have this test class extend
BaseTestorBaseTestConfigto satisfy the repository’s test contract.As per coding guidelines "Use snake_case for test method names with descriptive naming" and "Extend BaseTest or BaseTestConfig for test classes to inherit logging/timing hooks".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/io/mapsmessaging/state/config/TwinManagerConfigTest.java` around lines 33 - 111, Update the TwinManagerConfigTest class declaration to extend BaseTest or BaseTestConfig instead of no base class, which will ensure the test class inherits the required logging and timing hooks as per the project's test standards. The test method names are already following snake_case convention, so only the class inheritance needs to be modified.Source: Coding guidelines
src/test/java/io/mapsmessaging/state/auditor/AuditorFactoryTest.java (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign test class/method conventions with repository test rules.
This test should follow
*Test.javaconventions: use snake_case method names and extendBaseTestorBaseTestConfig.As per coding guidelines "Use snake_case for test method names with descriptive naming" and "Extend BaseTest or BaseTestConfig for test classes to inherit logging/timing hooks".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/io/mapsmessaging/state/auditor/AuditorFactoryTest.java` around lines 30 - 37, The AuditorFactoryTest class needs to align with repository test conventions. First, make the AuditorFactoryTest class extend either BaseTest or BaseTestConfig to inherit logging and timing hooks. Second, convert the test method name build_returnsGenericAuditContextThatTwinManagerCanExpose to proper snake_case format with all lowercase letters and underscores separating words (e.g., build_returns_generic_audit_context_that_twin_manager_can_expose) to follow the coding guideline for descriptive test method naming.Source: Coding guidelines
src/main/java/io/mapsmessaging/state/config/TwinManagerConfigDTO.java (1)
24-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign import block with repository Java import rules.
The import order and
java.utilimport style in this block do not match the repository’s Java conventions.♻️ Suggested import cleanup
import io.mapsmessaging.configuration.ConfigurationProperties; import io.mapsmessaging.state.config.n2k.N2KTwinConfig; -import lombok.Data; import io.swagger.v3.oas.annotations.media.Schema; +import lombok.Data; import lombok.EqualsAndHashCode; - -import java.util.ArrayList; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; +import java.util.*;As per coding guidelines,
**/*.javaimports must be organized as project, third-party, Lombok, then Java stdlib, andjava.util.*should use wildcard imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/io/mapsmessaging/state/config/TwinManagerConfigDTO.java` around lines 24 - 33, The import block in the TwinManagerConfigDTO class does not follow the repository's Java import conventions. Reorganize the imports in this order: first all project imports (io.mapsmessaging.*), then third-party imports (io.swagger.*), then Lombok imports (lombok.*), and finally Java stdlib imports. Additionally, replace the individual java.util imports (ArrayList, LinkedHashMap, List, Map) with a single wildcard import using java.util.*.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pom.xml`:
- Around line 684-688: The mapsmessaging-stanag-state-adapter dependency is
currently using ${project.version} which will cause dependency resolution issues
once it moves to a separate repository with independent versioning. Add a new
properties entry called maps.stanag.version in the properties section of the
pom.xml file and set it to the appropriate version for the Stanag adapter. Then
update the version element in the mapsmessaging-stanag-state-adapter dependency
declaration to reference this new ${maps.stanag.version} property instead of
${project.version}, following the same pattern used by other io.mapsmessaging
external modules like maps.canbus.version and maps.logging.version.
In `@src/main/java/io/mapsmessaging/state/config/TwinManagerConfig.java`:
- Around line 230-233: The comparison in the adapterConfig check uses reference
equality instead of value-based equality, which means maps with identical
content but different object references will incorrectly trigger the hasChanged
flag. Additionally, the code does not handle null-safe copying, which will throw
a NullPointerException if newConfig.getAdapterConfig() returns null. Replace the
reference comparison with equals() for proper value-based comparison, and add
null-safe handling before copying the adapterConfig map to prevent null pointer
exceptions when the incoming adapterConfig is null.
In `@src/main/java/io/mapsmessaging/state/StateManagerAgent.java`:
- Around line 72-79: The catch block for IOException in the AuditorFactory
initialization silently swallows the error using e.printStackTrace(), leaving
auditContext null and allowing TwinManager to be constructed without audit
context, which disables auditing without warning. Replace the
e.printStackTrace() call with proper logging using the project's enum-based
Logger from io.mapsmessaging.logging.Logger, log the failure with relevant
context about the factory.build() call, and either propagate the exception or
handle it explicitly so the failure is clearly visible rather than silent.
---
Nitpick comments:
In `@src/main/java/io/mapsmessaging/state/config/TwinManagerConfigDTO.java`:
- Around line 24-33: The import block in the TwinManagerConfigDTO class does not
follow the repository's Java import conventions. Reorganize the imports in this
order: first all project imports (io.mapsmessaging.*), then third-party imports
(io.swagger.*), then Lombok imports (lombok.*), and finally Java stdlib imports.
Additionally, replace the individual java.util imports (ArrayList,
LinkedHashMap, List, Map) with a single wildcard import using java.util.*.
In `@src/test/java/io/mapsmessaging/state/auditor/AuditorFactoryTest.java`:
- Around line 30-37: The AuditorFactoryTest class needs to align with repository
test conventions. First, make the AuditorFactoryTest class extend either
BaseTest or BaseTestConfig to inherit logging and timing hooks. Second, convert
the test method name build_returnsGenericAuditContextThatTwinManagerCanExpose to
proper snake_case format with all lowercase letters and underscores separating
words (e.g., build_returns_generic_audit_context_that_twin_manager_can_expose)
to follow the coding guideline for descriptive test method naming.
In `@src/test/java/io/mapsmessaging/state/config/TwinManagerConfigTest.java`:
- Around line 98-111: The update_copiesAdapterConfigMap test currently validates
that adapter configuration content is copied but does not verify that the maps
are truly isolated instances. Add an assertNotSame assertion to confirm that the
adapter config map in config is a different instance than the map in newConfig,
and then add a mutation to newConfig.getAdapterConfig() after the update call
followed by assertions that verify the config.getAdapterConfig() remains
unchanged, ensuring the maps are properly isolated and not sharing references.
- Around line 33-111: Update the TwinManagerConfigTest class declaration to
extend BaseTest or BaseTestConfig instead of no base class, which will ensure
the test class inherits the required logging and timing hooks as per the
project's test standards. The test method names are already following snake_case
convention, so only the class inheritance needs to be modified.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50e1a5c2-2a68-4c96-952e-b4ed8277ba79
📒 Files selected for processing (76)
pom.xmlsrc/main/java/io/mapsmessaging/state/StateJsonHelper.javasrc/main/java/io/mapsmessaging/state/StateManagerAgent.javasrc/main/java/io/mapsmessaging/state/adapter/StateMessageAdapter.javasrc/main/java/io/mapsmessaging/state/adapter/StateMessageAdapterContext.javasrc/main/java/io/mapsmessaging/state/adapter/StateMessageAdapterFactory.javasrc/main/java/io/mapsmessaging/state/auditor/AuditorFactory.javasrc/main/java/io/mapsmessaging/state/auditor/StateAuditContext.javasrc/main/java/io/mapsmessaging/state/config/StanagConfig.javasrc/main/java/io/mapsmessaging/state/config/TwinManagerConfig.javasrc/main/java/io/mapsmessaging/state/config/TwinManagerConfigDTO.javasrc/main/java/io/mapsmessaging/state/drone/core/TwinManager.javasrc/main/java/io/mapsmessaging/state/drone/publisher/TwinJsonPublisher.javasrc/main/java/io/mapsmessaging/state/stanag/ChatListener.javasrc/main/java/io/mapsmessaging/state/stanag/NodeUpdate.javasrc/main/java/io/mapsmessaging/state/stanag/StanagSession.javasrc/main/java/io/mapsmessaging/state/stanag/StanagUuidGenerator.javasrc/main/java/io/mapsmessaging/state/stanag/TaskAdminCommand.javasrc/main/java/io/mapsmessaging/state/stanag/TaskAdminCommandException.javasrc/main/java/io/mapsmessaging/state/stanag/TaskListener.javasrc/main/java/io/mapsmessaging/state/stanag/audit/AuditEvent.javasrc/main/java/io/mapsmessaging/state/stanag/audit/AuditMessages.javasrc/main/java/io/mapsmessaging/state/stanag/audit/AuditPayload.javasrc/main/java/io/mapsmessaging/state/stanag/audit/Auditor.javasrc/main/java/io/mapsmessaging/state/stanag/messages/TaskEventPublisher.javasrc/main/java/io/mapsmessaging/state/stanag/messages/TaskSchemaValidator.javasrc/main/java/io/mapsmessaging/state/stanag/messages/TaskState.javasrc/main/java/io/mapsmessaging/state/stanag/messages/TaskStatusContext.javasrc/main/java/io/mapsmessaging/state/stanag/messages/core/MessageHeader.javasrc/main/java/io/mapsmessaging/state/stanag/messages/core/MessageHeaderBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/core/MessageType.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/EntityDescription.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/EulerAngles.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/LatitudeLongitudeAltitude.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/NodeMessageSupport.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/Orientation.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/Pose.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/Position.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/SpeedCourseClimbRate.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/common/Velocity.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescription.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescriptionBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescriptionBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdate.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateOperation.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/PutValue.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/Track.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatus.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatusBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatusBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminActionEnum.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminMessage.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminMessageBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackMessage.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackMessageBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/result/ResultReason.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/result/ResultReasonBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultBody.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultMessage.javasrc/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultMessageBuilder.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/RepositionTaskHandler.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/TaskDispatchResult.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/TaskDispatcher.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/TaskHandler.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/monitor/CompletedTaskMonitor.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/monitor/RepositionTaskMonitor.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskMonitor.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskMonitorManager.javasrc/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskStatusPublisher.javasrc/main/resources/META-INF/services/io.mapsmessaging.state.stanag.tasks.TaskHandlersrc/test/java/io/mapsmessaging/state/auditor/AuditorFactoryTest.javasrc/test/java/io/mapsmessaging/state/config/TwinManagerConfigTest.java
💤 Files with no reviewable changes (62)
- src/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultMessage.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/Velocity.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/SpeedCourseClimbRate.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/EulerAngles.java
- src/main/java/io/mapsmessaging/state/stanag/audit/AuditEvent.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateBody.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/Orientation.java
- src/main/java/io/mapsmessaging/state/stanag/audit/AuditMessages.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/EntityDescription.java
- src/main/java/io/mapsmessaging/state/stanag/messages/core/MessageHeaderBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/messages/TaskEventPublisher.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultMessageBuilder.java
- src/main/java/io/mapsmessaging/state/config/StanagConfig.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskMonitorManager.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/Position.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/TaskDispatcher.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackBody.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/TaskDispatchResult.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminBody.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateOperation.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescriptionBody.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/Track.java
- src/main/java/io/mapsmessaging/state/stanag/messages/TaskSchemaValidator.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/RepositionTaskHandler.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminMessage.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/NodeMessageSupport.java
- src/main/java/io/mapsmessaging/state/stanag/messages/core/MessageType.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskMonitor.java
- src/main/java/io/mapsmessaging/state/stanag/messages/core/MessageHeader.java
- src/main/java/io/mapsmessaging/state/stanag/TaskListener.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescription.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdate.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/Pose.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/monitor/RepositionTaskMonitor.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatus.java
- src/main/java/io/mapsmessaging/state/stanag/messages/TaskStatusContext.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/common/LatitudeLongitudeAltitude.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/result/TaskResultBody.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/result/ResultReasonBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminMessageBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/PutValue.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/monitor/TaskStatusPublisher.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackMessage.java
- src/main/java/io/mapsmessaging/state/stanag/StanagSession.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/description/NodeDescriptionBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/audit/Auditor.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatusBody.java
- src/main/java/io/mapsmessaging/state/stanag/NodeUpdate.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/TaskHandler.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/feedback/TaskFeedbackMessageBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/audit/AuditPayload.java
- src/main/java/io/mapsmessaging/state/stanag/TaskAdminCommandException.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/status/NodeStatusBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/messages/TaskState.java
- src/main/java/io/mapsmessaging/state/stanag/StanagUuidGenerator.java
- src/main/resources/META-INF/services/io.mapsmessaging.state.stanag.tasks.TaskHandler
- src/main/java/io/mapsmessaging/state/stanag/ChatListener.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/admin/TaskAdminActionEnum.java
- src/main/java/io/mapsmessaging/state/stanag/TaskAdminCommand.java
- src/main/java/io/mapsmessaging/state/stanag/tasks/monitor/CompletedTaskMonitor.java
- src/main/java/io/mapsmessaging/state/stanag/messages/node/dynamic/DynamicUpdateBuilder.java
- src/main/java/io/mapsmessaging/state/stanag/messages/task/result/ResultReason.java
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Refactored Stanag to a seperate repo/module
Summary by CodeRabbit
New Features
Refactor