Introduce application service implementations and document service contracts#9
Merged
koder95 merged 10 commits intoMay 7, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a comprehensive application service layer with eight new service implementations (AdminPasswordService, DebugService, DeviceInfoService, NetworkConfigurationService, PixelProgramService, RebootService, RuntimeDataService, UserManagementService), introduces an exception hierarchy, adds domain DTOs (AdminPasswordResetRequest, UserCreateRequest), updates port contracts, and expands test coverage across all new services while updating the README documentation. ChangesApplication Service Layer Implementation
Sequence Diagram(s)Service implementations follow a consistent request-response pattern through domain ports. Here is a representative flow for a typical service operation (e.g., AdminPasswordService): sequenceDiagram
participant Client
participant AdminPasswordServiceImpl
participant InputPort
participant Infrastructure
Client->>AdminPasswordServiceImpl: resetPassword(secureKey, passwordDto)
activate AdminPasswordServiceImpl
AdminPasswordServiceImpl->>AdminPasswordServiceImpl: validateInputs()
alt Validation fails
AdminPasswordServiceImpl-->>Client: AdminPasswordResetException
else Validation succeeds
AdminPasswordServiceImpl->>AdminPasswordServiceImpl: buildRequest(secureKey, passwordDto)
AdminPasswordServiceImpl->>InputPort: send(AdminPasswordResetRequest)
activate InputPort
InputPort->>Infrastructure: create & send HTTP request
activate Infrastructure
Infrastructure-->>InputPort: response/exception
deactivate Infrastructure
InputPort-->>AdminPasswordServiceImpl: success or InputPortException
deactivate InputPort
alt Port throws InputPortException
AdminPasswordServiceImpl-->>Client: AdminPasswordResetException(cause=InputPortException)
else Port succeeds
AdminPasswordServiceImpl-->>Client: (void)
end
end
deactivate AdminPasswordServiceImpl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR introduces eight new service implementations with heterogeneous error-handling patterns, synchronized operations (ReentrantLock in PixelProgramServiceImpl and UserManagementServiceImpl), complex validation chains (RuntimeDataServiceImpl pixel-program existence checks), and cross-cutting exception-translation logic. Each service exhibits distinct orchestration semantics (PixelProgramServiceImpl defensive-copy returns and list-based mutations; UserManagementServiceImpl concurrent user checks; RuntimeDataServiceImpl multi-port validation). Comprehensive test coverage across all implementations and port changes (UserCreateInputPort signature shift) demands careful reasoning about backward compatibility. Nine new exception classes with similar structure reduce per-class cognitive load but necessitate systematic exception-contract validation across service interfaces. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 764ee3cc26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 (2)
src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java (2)
62-67: Minor DRY: consolidate the "pixel program not found" lookup.
get(int)(lines 62–67) andfindListIndex(...)(lines 127–135) duplicate the same search and identical exception message. You could haveget(int)delegate throughfindListIndex(e.g.,return programs.get(findListIndex(programs, index));) to keep the not-found semantics in one place.Also applies to: 127-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 62 - 67, get(int) duplicates the search and exception logic already implemented in findListIndex(List<PixelProgram>, int); change get(int) to call loadPrograms(), store the list in a variable (e.g., programs) and return programs.get(findListIndex(programs, index)), reusing findListIndex to throw PixelProgramNotFoundException with the existing message; ensure findListIndex is accessible from get (adjust visibility if needed) and remove the duplicated stream/filter code from get(int).
41-45: Prefer a more specific unchecked exception over rawRuntimeException.Wrapping port failures in bare
new RuntimeException(...)at lines 44, 88, 103, and 123 is a code smell — callers can't distinguish infrastructure failures from programmer errors, and it's harder to catch selectively. ConsiderIllegalStateException(standard, unchecked) or a dedicated application-layer unchecked exception consistent with the other new service implementations in this package.Also applies to: 85-89, 100-104, 117-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 41 - 45, Replace the raw RuntimeException wrappers around port failures in PixelProgramServiceImpl (the catch blocks that wrap calls such as set(current), find(...), update(...), delete(...)) with a more specific unchecked exception; either throw IllegalStateException with the original exception as the cause or throw the same application-layer unchecked exception used by other services (e.g., ApplicationServiceException or PixelProgramServiceException if one exists) so callers can distinguish infrastructure errors; update each catch that currently does `throw new RuntimeException("...", e)` to `throw new IllegalStateException("Cannot <operation> pixel programs", e)` or the project-specific unchecked type and keep the original message and cause.
🤖 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 53-69: The fenced code block containing the directory tree
(starting with "pl.vtt.wpi.core") triggers MD040; update its opening fence from
``` to a language-hint fence like ```text or ```plaintext so the block is
treated as plain text; locate the block in README.md (the one with the directory
tree and comments such as "AuthorizationHolder (thread-local auth state)") and
replace the fence accordingly.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java`:
- Around line 22-27: Update the null-check block in AdminPasswordServiceImpl
that validates the incoming PasswordDto to also reject empty or whitespace-only
passwords and confirmations (use passwordDto.password().isBlank() and
passwordDto.passwordConfirmation().isBlank() or equivalent) and throw an
IllegalArgumentException with a clear message; keep the subsequent equality
check (password().equals(passwordConfirmation())) but ensure both fields are
validated for blank values first so blank admin passwords are blocked just like
user password validation.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.java`:
- Around line 27-36: pollLogs() currently calls peekLogs() then
logsDeleteInputPort.send(null), which can lose entries appended between the read
(LOGS_READ via peekLogs()) and the delete (LOGS_DELETE via logsDeleteInputPort);
add an atomic read-and-clear input port (e.g., logsReadAndClearInputPort /
method name readAndClearLogs()) and use it from pollLogs(), or if an atomic
endpoint cannot be added, update pollLogs() and its JavaDoc to explicitly mark
it as best-effort (non-atomic) and document the race window; also update any
callers/tests to expect non-guaranteed drain if you choose the documentation
approach.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`:
- Around line 35-46: In PixelProgramServiceImpl.insert(List<PixelProgram>
pixelPrograms) move the null/empty guard to the top so you return immediately
before calling getAll(); currently getAll() is invoked even for null/empty
inputs (causing unnecessary I/O and possible exceptions), so check "if
(pixelPrograms == null || pixelPrograms.isEmpty()) return;" first, then call
getAll(), addAll(pixelPrograms), and call set(current) catching
DataInconsistencyException as before.
- Around line 35-125: The class PixelProgramServiceImpl has unguarded
read-modify-write races in methods insert, save, update, and remove (and any
other methods that call getAll()/loadPrograms() followed by set()), so either
document a single-threaded/external-synchronization contract in the class
Javadoc or make the operations atomic by adding a per-instance lock and using it
to guard all mutation sequences; specifically add a private final Lock (or
synchronized block) used around insert(List<PixelProgram>),
save(List<RgbColor>), update(int,List<RgbColor>), remove(int), and any other
code paths that call getAll()/loadPrograms() then set(), and ensure
set(List<PixelProgram>) and loadPrograms() are called inside that lock; also
update the class-level Javadoc to state the threading model and requirements (or
mention that InputPort/OutputPort must provide atomicity) so callers know
whether external synchronization is required.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.java`:
- Around line 18-48: The createUser flow currently discards the initial password
because createUser(User user, PasswordDto passwordDto) validates passwordDto but
calls userCreateInputPort.send(user); change the port payload to a composite
request: introduce a record UserCreateRequest(User user, PasswordDto
passwordDto) and update the field and constructor signature from InputPort<User>
userCreateInputPort to InputPort<UserCreateRequest> userCreateInputPort; then in
createUser(...) send new UserCreateRequest(user, passwordDto) instead of user
and adjust any callers/implementations of the port to accept the new
UserCreateRequest type (also update the Objects.requireNonNull message to
reflect the new parameter name).
---
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`:
- Around line 62-67: get(int) duplicates the search and exception logic already
implemented in findListIndex(List<PixelProgram>, int); change get(int) to call
loadPrograms(), store the list in a variable (e.g., programs) and return
programs.get(findListIndex(programs, index)), reusing findListIndex to throw
PixelProgramNotFoundException with the existing message; ensure findListIndex is
accessible from get (adjust visibility if needed) and remove the duplicated
stream/filter code from get(int).
- Around line 41-45: Replace the raw RuntimeException wrappers around port
failures in PixelProgramServiceImpl (the catch blocks that wrap calls such as
set(current), find(...), update(...), delete(...)) with a more specific
unchecked exception; either throw IllegalStateException with the original
exception as the cause or throw the same application-layer unchecked exception
used by other services (e.g., ApplicationServiceException or
PixelProgramServiceException if one exists) so callers can distinguish
infrastructure errors; update each catch that currently does `throw new
RuntimeException("...", e)` to `throw new IllegalStateException("Cannot
<operation> pixel programs", e)` or the project-specific unchecked type and keep
the original message and cause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98b1fa4e-7bc9-4cca-a268-eb20df402d09
📒 Files selected for processing (9)
README.mdsrc/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.java
There was a problem hiding this comment.
🧹 Nitpick comments (5)
README.md (1)
154-154: Minor: the unit-test description is now narrower than the actual coverage.The updated sentence mentions only "core port behaviors and login service logic", but this PR in fact broadens coverage to include
UserCreateInputPort(with the newUserCreateRequestDTO) and other application services. Consider rewording to cover port + application-service coverage more generally, so the doc doesn't under-sell what's there.✏️ Proposed tweak
-Unit tests are written with JUnit Jupiter 5 and currently include coverage for core port behaviors and login service logic. +Unit tests are written with JUnit Jupiter 5 and currently include coverage for core port behaviors and application service logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 154, Update the README sentence about unit-test coverage to reflect the broader scope: replace the narrow phrase "core port behaviors and login service logic" with wording that mentions both port-level and application-service tests (e.g., include UserCreateInputPort and the new UserCreateRequest DTO plus other application services); locate the sentence in README.md that currently describes JUnit Jupiter 5 coverage and rephrase it to indicate port + application-service coverage so the documentation accurately reflects tests for UserCreateInputPort, UserCreateRequest, login service, and other service-level behaviors.src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java (2)
90-110: Minor: the returnedsavedreference may mislead onIllegalStateException.When
set(programs)on line 103 fails (and is wrapped intoIllegalStateExceptionwith theDataInconsistencyExceptioncause), the method exits via the catch block without ever returning thesavedvalue — good — but a caller reading only the Javadoc/interface contract might still reasonably assume "ifsavereturned normally, the program is persisted; if it threw, nothing was persisted". That assumption holds here, so no action is strictly required. One small readability tweak: usegetAll()'s defensive copy plus a local mutation rather thanprograms.add(saved)directly, so the intent (mutation happens on a scratch list, not on state shared with anything) is obvious. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 90 - 110, The save method currently mutates the list returned by getAll() which can be confusing; change it to operate on a local mutable copy: call getAll(), wrap the result into a new ArrayList (e.g. List<PixelProgram> programs = new ArrayList<>(getAll())), add the new PixelProgram(saved) to that local list, then call set(programs) as before; keep the existing mutationLock, exception handling, and use of PixelProgram constructor and List.copyOf for pixelProgram unchanged so semantics remain identical but intent is clearer.
62-77: Minor:List.copyOfwill throw a raw NPE on null elements instead ofDataInconsistencyException.
setexplicitly converts a null list intoDataInconsistencyException("Pixel programs cannot be null"), butList.copyOf(pixelPrograms)on line 69 throws a plainNullPointerExceptionif any element is null — that NPE escapes thetry { ... } catch (InputPortException ...)and surfaces to the caller as an untypedNullPointerException. If the intent (per the PR summary "validates non-null input elements") is to treat null elements as a consistency issue, consider validating elements up-front and throwingDataInconsistencyExceptionfor symmetry:♻️ Proposed tweak
if (pixelPrograms == null) { throw new DataInconsistencyException("Pixel programs cannot be null"); } + for (PixelProgram program : pixelPrograms) { + if (program == null) { + throw new DataInconsistencyException("Pixel program element cannot be null"); + } + } try { pixelProgramsInputPort.send(List.copyOf(pixelPrograms));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 62 - 77, The set method uses List.copyOf(pixelPrograms) which will throw a raw NullPointerException if any element is null; before calling List.copyOf in PixelProgramServiceImpl.set, validate that pixelPrograms contains no null elements and if any are found throw a DataInconsistencyException("Pixel programs cannot contain null elements") so the caller gets a consistent exception; keep the existing mutationLock and existing InputPortException handling around pixelProgramsInputPort.send(List.copyOf(pixelPrograms)).src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java (1)
23-33: Optional: consider narrowing thecatch (Exception e)to avoid double-wrapping.Because
requestFactory.create(...)/requestSender.send(...)can themselves throwInputPortException(the declared sender exception type in other ports),catch (Exception e)will re-wrap an already-typedInputPortExceptioninside anotherInputPortExceptionwith cause = original. That's functionally harmless here but loses the original message/type on the outer layer. Either catchRuntimeException(sinceRequestSender.senddoesn't actually declare a checked throw in this codebase) or catch specific exceptions explicitly. Low priority — existing behavior matches other input ports, so feel free to leave as-is for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java` around lines 23 - 33, The catch-all in UserCreateInputPort.send currently catches Exception and wraps it in a new InputPortException, which double-wraps if the cause is already an InputPortException; modify the error handling in send(UserCreateRequest) so you either catch RuntimeException (or specific checked exceptions thrown by requestFactory.create/requestSender.send) or detect and rethrow an existing InputPortException (i.e., if (e instanceof InputPortException) throw (InputPortException)e;) before wrapping, referencing the send method in UserCreateInputPort and the calls to requestFactory.create and requestSender.send to locate the change.src/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.java (1)
55-62: Minor: test name and scope don't fully match the new validation surface.
send_nullUser_throwsInputPortExceptiononly exercises the fully-nullUserCreateRequestpath. The production guard atUserCreateInputPort#sendadditionally rejectsnullforobj.user()andobj.passwordDto()independently, but neither partial-null case is covered here. Consider either renaming this test (e.g.send_nullRequest_throwsInputPortException) and adding two more cases fornew UserCreateRequest(null, passwordDto)andnew UserCreateRequest(user, null), to lock in the expanded validation behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.java` around lines 55 - 62, The test name and coverage should match the expanded validation in UserCreateInputPort#send: either rename send_nullUser_throwsInputPortException to send_nullRequest_throwsInputPortException, or (preferred) add two new tests that assert InputPortException is thrown for partial-null cases by calling port.send(new UserCreateRequest(null, validPasswordDto)) and port.send(new UserCreateRequest(validUser, null)); keep the existing null-request test intact, use the same assertion pattern (assertThrows(InputPortException.class, ...)) and assert the expected message text to ensure the validations for obj.user() and obj.passwordDto() are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Line 154: Update the README sentence about unit-test coverage to reflect the
broader scope: replace the narrow phrase "core port behaviors and login service
logic" with wording that mentions both port-level and application-service tests
(e.g., include UserCreateInputPort and the new UserCreateRequest DTO plus other
application services); locate the sentence in README.md that currently describes
JUnit Jupiter 5 coverage and rephrase it to indicate port + application-service
coverage so the documentation accurately reflects tests for UserCreateInputPort,
UserCreateRequest, login service, and other service-level behaviors.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`:
- Around line 90-110: The save method currently mutates the list returned by
getAll() which can be confusing; change it to operate on a local mutable copy:
call getAll(), wrap the result into a new ArrayList (e.g. List<PixelProgram>
programs = new ArrayList<>(getAll())), add the new PixelProgram(saved) to that
local list, then call set(programs) as before; keep the existing mutationLock,
exception handling, and use of PixelProgram constructor and List.copyOf for
pixelProgram unchanged so semantics remain identical but intent is clearer.
- Around line 62-77: The set method uses List.copyOf(pixelPrograms) which will
throw a raw NullPointerException if any element is null; before calling
List.copyOf in PixelProgramServiceImpl.set, validate that pixelPrograms contains
no null elements and if any are found throw a DataInconsistencyException("Pixel
programs cannot contain null elements") so the caller gets a consistent
exception; keep the existing mutationLock and existing InputPortException
handling around pixelProgramsInputPort.send(List.copyOf(pixelPrograms)).
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java`:
- Around line 23-33: The catch-all in UserCreateInputPort.send currently catches
Exception and wraps it in a new InputPortException, which double-wraps if the
cause is already an InputPortException; modify the error handling in
send(UserCreateRequest) so you either catch RuntimeException (or specific
checked exceptions thrown by requestFactory.create/requestSender.send) or detect
and rethrow an existing InputPortException (i.e., if (e instanceof
InputPortException) throw (InputPortException)e;) before wrapping, referencing
the send method in UserCreateInputPort and the calls to requestFactory.create
and requestSender.send to locate the change.
In `@src/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.java`:
- Around line 55-62: The test name and coverage should match the expanded
validation in UserCreateInputPort#send: either rename
send_nullUser_throwsInputPortException to
send_nullRequest_throwsInputPortException, or (preferred) add two new tests that
assert InputPortException is thrown for partial-null cases by calling
port.send(new UserCreateRequest(null, validPasswordDto)) and port.send(new
UserCreateRequest(validUser, null)); keep the existing null-request test intact,
use the same assertion pattern (assertThrows(InputPortException.class, ...)) and
assert the expected message text to ensure the validations for obj.user() and
obj.passwordDto() are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54b3dc4d-32d3-419d-8a03-31d070c1f955
📒 Files selected for processing (9)
README.mdsrc/main/java/pl/vtt/wpi/core/application/service/DebugService.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/dto/UserCreateRequest.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.javasrc/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/pl/vtt/wpi/core/application/service/DebugService.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.java (1)
10-21: Consider expanding coverage for the other branches ofNetworkConfigurationServiceImpl.The mapping assertion is good, but the test only exercises a single happy path. The class exposes a
config(WifiConfig)overload and theconfig(AccessPointConfig)method throwsIllegalArgumentExceptionwhen passednull— neither is covered. Adding small tests for these cases would lock in the contract and guard against regressions.🧪 Suggested additional tests
`@Test` void config_accessPointConfig_isMappedToWifiConfig() { AtomicReference<WifiConfig> sent = new AtomicReference<>(); NetworkConfigurationServiceImpl service = new NetworkConfigurationServiceImpl(sent::set); service.config(new AccessPointConfig("ap", "pass")); assertEquals(new WifiConfig("ap", "pass"), sent.get()); } + + `@Test` + void config_nullAccessPointConfig_throws() { + NetworkConfigurationServiceImpl service = new NetworkConfigurationServiceImpl(c -> {}); + assertThrows(IllegalArgumentException.class, + () -> service.config((AccessPointConfig) null)); + } + + `@Test` + void config_wifiConfig_isForwardedAsIs() { + AtomicReference<WifiConfig> sent = new AtomicReference<>(); + NetworkConfigurationServiceImpl service = new NetworkConfigurationServiceImpl(sent::set); + WifiConfig cfg = new WifiConfig("ap", "pass"); + + service.config(cfg); + + assertEquals(cfg, sent.get()); + }(You'd also need
import static org.junit.jupiter.api.Assertions.assertThrows;.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.java` around lines 10 - 21, Add unit tests for the uncovered branches in NetworkConfigurationServiceImpl: (1) a test that calls config(WifiConfig) and verifies the same WifiConfig is forwarded to the provided Consumer (similar to the existing AccessPointConfig test but passing a WifiConfig directly), and (2) a test that asserts config((AccessPointConfig) null) throws IllegalArgumentException using assertThrows; reference the class/method names NetworkConfigurationServiceImpl, config(WifiConfig), config(AccessPointConfig) and the exception IllegalArgumentException, and add the static import for assertThrows from org.junit.jupiter.api.Assertions.src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java (1)
27-35: Prefer a dedicated catch over aninstanceofpattern insidecatch (Exception).The
instanceofre-throw pattern works but is unusual; a dedicatedcatch (InputPortException e)ahead of the generic catch is more idiomatic and avoids the broad-catch-then-rethrow dance. It also makes the intent (propagate domain exceptions, wrap everything else) obvious to readers.♻️ Suggested cleanup
try { Request<UserCreateRequest> request = requestFactory.create(obj, Method.POST, RequestTarget.USERS_CREATE); requestSender.send(request); - } catch (Exception e) { - if (e instanceof InputPortException inputPortException) { - throw inputPortException; - } + } catch (InputPortException e) { + throw e; + } catch (Exception e) { throw new InputPortException("Cannot create user", e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java` around lines 27 - 35, In the create flow in UserCreateInputPort (around the requestFactory.create(...) and requestSender.send(...) calls) replace the broad catch(Exception) with two dedicated catches: first catch InputPortException and rethrow it unchanged, then catch Exception and wrap it in a new InputPortException("Cannot create user", e); remove the instanceof pattern inside the catch and keep the original behavior of propagating InputPortException while wrapping all other exceptions.src/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.java (1)
17-64: LGTM — consider broadening coverage in a follow-up.Tests correctly exercise the three main branches (success forwarding, existing-user conflict, blank password). The
assertEquals(new UserCreateRequest(user, passwordDto), sent.get())assumes record-based value equality onUserCreateRequest, which holds for the DTO as written.Given that
UserManagementServiceImplalso handles null user, password/confirmation mismatch,InputPortException→RuntimeExceptionwrapping,changePasswordsuccess path, andremoveUser, it would be worth adding tests for those paths in a follow-up to prevent regressions as the port contracts evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.java` around lines 17 - 64, Add unit tests to broaden coverage for UserManagementServiceImpl: add tests for createUser(null) to assert the expected null-user handling, createUser when password/confirmation mismatch to assert the appropriate exception, simulate the input port throwing InputPortException and verify it is wrapped as a RuntimeException by createUser, add a success-path test for changePassword verifying the forwarded PasswordDto and no exception, and add tests for removeUser verifying the forwarded removal request/behavior; reference the methods and DTOs UserManagementServiceImpl#createUser, UserManagementServiceImpl#changePassword, UserManagementServiceImpl#removeUser, PasswordDto, UserCreateRequest and exceptions UserAlreadyExistsException, InvalidPasswordException, InputPortException/RuntimeException when implementing these tests.src/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.java (1)
11-32: LGTM — optional: also assert the cause is preserved.The failure test could additionally verify
exception.getCause() instanceof IllegalStateExceptionto lock in the cause-preservation contract implemented byRebootServiceImpl(which unwrapsInputPortException.getCause()). Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.java` around lines 11 - 32, Update the failing-path test reboot_portFailure_wrapsException in RebootServiceImplTest to also assert that the wrapped runtime exception preserves the original cause: after capturing the RuntimeException from assertThrows(service::reboot), add an assertion that exception.getCause() is an instance of IllegalStateException (or equals the original cause), referencing the test method reboot_portFailure_wrapsException and the captured exception variable to lock in the cause-preservation contract of RebootServiceImpl.src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java (2)
62-80: Validate inputs before acquiring the lock.Null/null-element validation here does not touch shared state, so taking
mutationLockprior to the checks needlessly serializes invalid calls. Consider moving the two validation blocks abovemutationLock.lock().♻️ Proposed fix
`@Override` public void set(List<PixelProgram> pixelPrograms) throws DataInconsistencyException { + if (pixelPrograms == null) { + throw new DataInconsistencyException("Pixel programs cannot be null"); + } + if (pixelPrograms.stream().anyMatch(Objects::isNull)) { + throw new DataInconsistencyException("Pixel programs cannot contain null elements"); + } mutationLock.lock(); try { - if (pixelPrograms == null) { - throw new DataInconsistencyException("Pixel programs cannot be null"); - } - if (pixelPrograms.stream().anyMatch(Objects::isNull)) { - throw new DataInconsistencyException("Pixel programs cannot contain null elements"); - } try { pixelProgramsInputPort.send(List.copyOf(pixelPrograms));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 62 - 80, The null and null-element validations in PixelProgramServiceImpl.set are performed while holding mutationLock, but they don't access shared state—move the two checks (pixelPrograms == null and pixelPrograms.stream().anyMatch(Objects::isNull)) to before calling mutationLock.lock() so invalid inputs are rejected without serializing callers; keep the mutationLock around the pixelProgramsInputPort.send(List.copyOf(pixelPrograms)) call and the InputPortException handling so the mutation remains properly synchronized.
94-102: Redundant defensive copy.
getAll()already returnsnew ArrayList<>(loadPrograms()), so wrapping it again innew ArrayList<>(...)creates two copies. Minor.♻️ Proposed fix
- List<PixelProgram> programs = new ArrayList<>(getAll()); + List<PixelProgram> programs = getAll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java` around lines 94 - 102, The save method creates an unnecessary defensive copy by doing new ArrayList<>(getAll()) even though getAll() already returns a new ArrayList from loadPrograms(); inside PixelProgramServiceImpl.save replace the double-copy with a single reference to the list returned by getAll() (e.g., List<PixelProgram> programs = getAll()) so you avoid creating two ArrayList instances while preserving the ability to mutate the local programs list under the existing mutationLock.src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java (1)
11-33: Consider expanding branch coverage.Current tests cover the happy path and blank-password branch. The production
resetPasswordadditionally throws on: nullsecureKey, blanksecureKey, nullpasswordDto(or null fields), and password/confirmation mismatch. A parameterized test (or a few extra cases) would lock in those branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java` around lines 11 - 33, Tests only exercise the happy path and blank-password case for AdminPasswordServiceImpl.resetPassword; add tests that cover the remaining validation branches: null secureKey, blank secureKey, null passwordDto, passwordDto with null fields, and password/confirmation mismatch so IllegalArgumentException is thrown as in production. Create additional unit tests (or a `@ParameterizedTest`) that call AdminPasswordServiceImpl.resetPassword with these inputs and assert assertThrows(IllegalArgumentException.class), and keep one positive test that verifies the AdminPasswordResetRequest is sent (using the AtomicReference/InputPort approach already present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`:
- Around line 133-145: The remove method currently propagates
DataInconsistencyException while insert/save/update wrap it; change remove(int
index) in PixelProgramServiceImpl to catch DataInconsistencyException thrown by
set(programs) and rethrow as an IllegalStateException (matching
insert/save/update), and update the service interface signature to drop
DataInconsistencyException from remove's throws clause so callers no longer see
the checked exception; keep the same locking logic (mutationLock) and still
return the removed PixelProgram.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImplTest.java`:
- Around line 16-23: The test set_nullElement_throwsDataInconsistencyException
never reaches PixelProgramServiceImpl.set(...) because List.of(null) throws NPE;
change the test to use a list factory that allows nulls (e.g.,
Collections.singletonList(null) or Arrays.asList((PixelProgram) null)) so the
null element is passed into PixelProgramServiceImpl.set(...) and
assertThrows(DataInconsistencyException.class, () -> service.set(...)) can
observe the DataInconsistencyException thrown by the null-element validation in
PixelProgramServiceImpl.
---
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`:
- Around line 62-80: The null and null-element validations in
PixelProgramServiceImpl.set are performed while holding mutationLock, but they
don't access shared state—move the two checks (pixelPrograms == null and
pixelPrograms.stream().anyMatch(Objects::isNull)) to before calling
mutationLock.lock() so invalid inputs are rejected without serializing callers;
keep the mutationLock around the
pixelProgramsInputPort.send(List.copyOf(pixelPrograms)) call and the
InputPortException handling so the mutation remains properly synchronized.
- Around line 94-102: The save method creates an unnecessary defensive copy by
doing new ArrayList<>(getAll()) even though getAll() already returns a new
ArrayList from loadPrograms(); inside PixelProgramServiceImpl.save replace the
double-copy with a single reference to the list returned by getAll() (e.g.,
List<PixelProgram> programs = getAll()) so you avoid creating two ArrayList
instances while preserving the ability to mutate the local programs list under
the existing mutationLock.
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.java`:
- Around line 27-35: In the create flow in UserCreateInputPort (around the
requestFactory.create(...) and requestSender.send(...) calls) replace the broad
catch(Exception) with two dedicated catches: first catch InputPortException and
rethrow it unchanged, then catch Exception and wrap it in a new
InputPortException("Cannot create user", e); remove the instanceof pattern
inside the catch and keep the original behavior of propagating
InputPortException while wrapping all other exceptions.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java`:
- Around line 11-33: Tests only exercise the happy path and blank-password case
for AdminPasswordServiceImpl.resetPassword; add tests that cover the remaining
validation branches: null secureKey, blank secureKey, null passwordDto,
passwordDto with null fields, and password/confirmation mismatch so
IllegalArgumentException is thrown as in production. Create additional unit
tests (or a `@ParameterizedTest`) that call AdminPasswordServiceImpl.resetPassword
with these inputs and assert assertThrows(IllegalArgumentException.class), and
keep one positive test that verifies the AdminPasswordResetRequest is sent
(using the AtomicReference/InputPort approach already present).
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.java`:
- Around line 10-21: Add unit tests for the uncovered branches in
NetworkConfigurationServiceImpl: (1) a test that calls config(WifiConfig) and
verifies the same WifiConfig is forwarded to the provided Consumer (similar to
the existing AccessPointConfig test but passing a WifiConfig directly), and (2)
a test that asserts config((AccessPointConfig) null) throws
IllegalArgumentException using assertThrows; reference the class/method names
NetworkConfigurationServiceImpl, config(WifiConfig), config(AccessPointConfig)
and the exception IllegalArgumentException, and add the static import for
assertThrows from org.junit.jupiter.api.Assertions.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.java`:
- Around line 11-32: Update the failing-path test
reboot_portFailure_wrapsException in RebootServiceImplTest to also assert that
the wrapped runtime exception preserves the original cause: after capturing the
RuntimeException from assertThrows(service::reboot), add an assertion that
exception.getCause() is an instance of IllegalStateException (or equals the
original cause), referencing the test method reboot_portFailure_wrapsException
and the captured exception variable to lock in the cause-preservation contract
of RebootServiceImpl.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.java`:
- Around line 17-64: Add unit tests to broaden coverage for
UserManagementServiceImpl: add tests for createUser(null) to assert the expected
null-user handling, createUser when password/confirmation mismatch to assert the
appropriate exception, simulate the input port throwing InputPortException and
verify it is wrapped as a RuntimeException by createUser, add a success-path
test for changePassword verifying the forwarded PasswordDto and no exception,
and add tests for removeUser verifying the forwarded removal request/behavior;
reference the methods and DTOs UserManagementServiceImpl#createUser,
UserManagementServiceImpl#changePassword, UserManagementServiceImpl#removeUser,
PasswordDto, UserCreateRequest and exceptions UserAlreadyExistsException,
InvalidPasswordException, InputPortException/RuntimeException when implementing
these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57661681-3e2a-4afe-90ba-ce34c5b61e62
📒 Files selected for processing (14)
README.mdsrc/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.java
💤 Files with no reviewable changes (1)
- src/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java
- README.md
Comment on lines
+133
to
+145
| @Override | ||
| public PixelProgram remove(int index) throws PixelProgramNotFoundException, DataInconsistencyException { | ||
| mutationLock.lock(); | ||
| try { | ||
| List<PixelProgram> programs = getAll(); | ||
| int removeIndex = findListIndex(programs, index); | ||
| PixelProgram removed = programs.remove(removeIndex); | ||
| set(programs); | ||
| return removed; | ||
| } finally { | ||
| mutationLock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the PixelProgramService interface signature for remove() to know which direction to align.
fd -a 'PixelProgramService.java' --type f | xargs -I{} sh -c 'echo "=== {} ==="; cat -n "{}"'Repository: VerbaTechTeam/wpi-core
Length of output: 1303
🏁 Script executed:
cat -n /home/jailuser/git/src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java | head -150Repository: VerbaTechTeam/wpi-core
Length of output: 7038
Inconsistent exception handling: remove propagates DataInconsistencyException, peers wrap it.
insert, save, and update all catch DataInconsistencyException from set(...) and rewrap it as IllegalStateException, while remove propagates it uncaught. This inconsistency breaks the symmetry of the mutation API and leaks an extra checked exception to callers of remove only.
To align with the other mutation methods, wrap the exception in remove and update the interface signature to remove DataInconsistencyException from its throws clause.
♻️ Proposed fix (wrap for consistency)
`@Override`
- public PixelProgram remove(int index) throws PixelProgramNotFoundException, DataInconsistencyException {
+ public PixelProgram remove(int index) throws PixelProgramNotFoundException {
mutationLock.lock();
try {
List<PixelProgram> programs = getAll();
int removeIndex = findListIndex(programs, index);
PixelProgram removed = programs.remove(removeIndex);
set(programs);
return removed;
+ } catch (DataInconsistencyException e) {
+ throw new IllegalStateException("Cannot remove pixel program", e);
} finally {
mutationLock.unlock();
}
}📝 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
| @Override | |
| public PixelProgram remove(int index) throws PixelProgramNotFoundException, DataInconsistencyException { | |
| mutationLock.lock(); | |
| try { | |
| List<PixelProgram> programs = getAll(); | |
| int removeIndex = findListIndex(programs, index); | |
| PixelProgram removed = programs.remove(removeIndex); | |
| set(programs); | |
| return removed; | |
| } finally { | |
| mutationLock.unlock(); | |
| } | |
| } | |
| `@Override` | |
| public PixelProgram remove(int index) throws PixelProgramNotFoundException { | |
| mutationLock.lock(); | |
| try { | |
| List<PixelProgram> programs = getAll(); | |
| int removeIndex = findListIndex(programs, index); | |
| PixelProgram removed = programs.remove(removeIndex); | |
| set(programs); | |
| return removed; | |
| } catch (DataInconsistencyException e) { | |
| throw new IllegalStateException("Cannot remove pixel program", e); | |
| } finally { | |
| mutationLock.unlock(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java`
around lines 133 - 145, The remove method currently propagates
DataInconsistencyException while insert/save/update wrap it; change remove(int
index) in PixelProgramServiceImpl to catch DataInconsistencyException thrown by
set(programs) and rethrow as an IllegalStateException (matching
insert/save/update), and update the service interface signature to drop
DataInconsistencyException from remove's throws clause so callers no longer see
the checked exception; keep the same locking logic (mutationLock) and still
return the removed PixelProgram.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
src/main/java/pl/vtt/wpi/core/application/exception/DebugServiceException.java (1)
3-11: Consider addingserialVersionUIDand brief Javadoc.
ExceptionimplementsSerializable, so declaring an explicitserialVersionUIDavoids compiler warnings and prevents accidental serialization incompatibilities if the class evolves. A short Javadoc explaining when this exception is thrown (e.g., wrappingInputPortException/OutputPortExceptionfrom debug operations) would also help consumers handle it appropriately.♻️ Proposed refinement
-public class DebugServiceException extends Exception { - public DebugServiceException(String message) { - super(message); - } - - public DebugServiceException(String message, Throwable cause) { - super(message, cause); - } -} +/** + * Thrown by {`@code` DebugService} operations when an underlying input/output port + * fails (e.g., reading logs or current state, clearing logs). + */ +public class DebugServiceException extends Exception { + private static final long serialVersionUID = 1L; + + public DebugServiceException(String message) { + super(message); + } + + public DebugServiceException(String message, Throwable cause) { + super(message, cause); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/exception/DebugServiceException.java` around lines 3 - 11, Add a brief Javadoc to the DebugServiceException class describing when it is thrown (e.g., to wrap InputPortException/OutputPortException from debug operations) and add an explicit serialVersionUID field to avoid serialization warnings and compatibility issues; specifically, update the DebugServiceException class to include a short Javadoc above the class declaration and a private static final long serialVersionUID = 1L (or a generated UID) while leaving the existing constructors (DebugServiceException(String) and DebugServiceException(String, Throwable)) intact.src/main/java/pl/vtt/wpi/core/application/exception/RebootServiceException.java (1)
3-11: Optional: add aserialVersionUID.
ExceptionisSerializable, so without an explicitserialVersionUIDthe JVM-generated default will silently change if the class shape ever evolves, breaking deserialization across versions. Most lint/IDE configurations warn about this for new exception types.♻️ Suggested addition
public class RebootServiceException extends Exception { + private static final long serialVersionUID = 1L; + public RebootServiceException(String message) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/exception/RebootServiceException.java` around lines 3 - 11, The RebootServiceException class lacks an explicit serialVersionUID which can break serialization compatibility; add a private static final long serialVersionUID field to the RebootServiceException class (e.g., in class RebootServiceException) with a chosen constant value to satisfy the Serializable contract and silence IDE/lint warnings while preserving deserialization stability across versions.src/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.java (1)
21-24: Consider preserving the full exception chain instead of unwrappinggetCause().
Throwable cause = e.getCause() == null ? e : e.getCause();drops theInputPortExceptionframe from the chain whenever a cause is present, which makes diagnostics in logs lose the port-layer context. SinceRebootServiceExceptionalready carries its own message, simply passingeas the cause preserves the full chain (RebootServiceException→InputPortException→ underlying cause) without harm.♻️ Suggested change
- } catch (InputPortException e) { - Throwable cause = e.getCause() == null ? e : e.getCause(); - throw new RebootServiceException("Cannot reboot device", cause); - } + } catch (InputPortException e) { + throw new RebootServiceException("Cannot reboot device", e); + }Note: this would also require adjusting the test that asserts on the wrapped cause (if any) accordingly. Per the snippet,
RebootServiceImplTest#reboot_portFailure_wrapsExceptiononly asserts the message, so this change should be test-compatible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.java` around lines 21 - 24, Replace the current unwrapping logic in RebootServiceImpl that computes Throwable cause = e.getCause() == null ? e : e.getCause(); and then throws new RebootServiceException("Cannot reboot device", cause) with simply passing the original InputPortException as the cause (throw new RebootServiceException("Cannot reboot device", e)); this preserves the full exception chain (RebootServiceException → InputPortException → underlying cause) while keeping the existing message; update any tests that assert on the exact wrapped cause if necessary (e.g., RebootServiceImplTest#reboot_portFailure_wrapsException).src/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoServiceException.java (1)
3-11: Optional: consider a shared service-exception base and aserialVersionUID.A couple of nice-to-haves for the new checked-exception family being introduced across the service layer:
- Since several near-identical exception classes are being added in this PR (
DeviceInfoServiceException,PixelProgramServiceException,UserManagementServiceException, …), a common abstract base (e.g.,ApplicationServiceException extends Exception) would let callers catch the family generically and remove constructor boilerplate from each subclass.ExceptionisSerializable, so omittingserialVersionUIDproduces a compiler/IDE warning and makes serial form depend on the auto-generated UID. Adding an explicit constant is cheap insurance.♻️ Suggested tweak
package pl.vtt.wpi.core.application.exception; -public class DeviceInfoServiceException extends Exception { +public class DeviceInfoServiceException extends ApplicationServiceException { + private static final long serialVersionUID = 1L; + public DeviceInfoServiceException(String message) { super(message); } public DeviceInfoServiceException(String message, Throwable cause) { super(message, cause); } }With a shared base such as:
package pl.vtt.wpi.core.application.exception; public abstract class ApplicationServiceException extends Exception { private static final long serialVersionUID = 1L; protected ApplicationServiceException(String message) { super(message); } protected ApplicationServiceException(String message, Throwable cause) { super(message, cause); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoServiceException.java` around lines 3 - 11, Introduce a shared abstract base exception class (e.g., ApplicationServiceException) that extends Exception, declares a private static final long serialVersionUID, and provides protected constructors (message and message+cause); change DeviceInfoServiceException to extend this new ApplicationServiceException and add its own serialVersionUID constant as well (do similarly for other service exceptions like PixelProgramServiceException and UserManagementServiceException) to remove constructor duplication and silence serialization warnings.src/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationServiceException.java (1)
3-11: LGTM!Standard checked-exception pattern with the two most useful constructors (
messageandmessage + cause). Naming and package placement (application.exception) are consistent with the new application-service exception family introduced in this PR.Optional nit: since
ExceptionimplementsSerializable, you may want to declare aserialVersionUIDto silence serialization-lint warnings and stabilize the serialized form across JVMs. Not a blocker.♻️ Optional: add serialVersionUID
public class NetworkConfigurationServiceException extends Exception { + private static final long serialVersionUID = 1L; + public NetworkConfigurationServiceException(String message) { super(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationServiceException.java` around lines 3 - 11, Add a serialVersionUID field to NetworkConfigurationServiceException to satisfy serialization linting and stabilize its serialized form; specifically, inside the NetworkConfigurationServiceException class (alongside the existing constructors) declare a private static final long serialVersionUID with any fixed long literal (e.g., 1L) so that the class explicitly defines its serialization identifier.src/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.java (1)
5-5: Nit: import ordering.
pl.vtt.wpi.core.application.exception.NetworkConfigurationServiceExceptionis grouped afterpl.vtt.wpi.core.domain.model.*imports, which breaks lexical ordering within the same root package. If your project enforces sorted imports (Checkstyle / IDE formatter), this will likely be flagged. Minor — feel free to ignore if no formatter rule is configured.♻️ Suggested ordering
-import pl.vtt.wpi.core.domain.model.device.AccessPointConfig; -import pl.vtt.wpi.core.domain.model.device.WifiConfig; -import pl.vtt.wpi.core.application.exception.NetworkConfigurationServiceException; +import pl.vtt.wpi.core.application.exception.NetworkConfigurationServiceException; +import pl.vtt.wpi.core.domain.model.device.AccessPointConfig; +import pl.vtt.wpi.core.domain.model.device.WifiConfig;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.java` at line 5, Import ordering is out of lexical order: move the import of NetworkConfigurationServiceException so imports under the same root are sorted; specifically, place pl.vtt.wpi.core.application.exception.NetworkConfigurationServiceException before the pl.vtt.wpi.core.domain.model.* imports in NetworkConfigurationService.java (adjust other nearby imports if necessary) to satisfy the project's sorted-import convention.src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java (1)
59-75: Test name and@DisplayNameno longer reflect what is being tested.The method name
runtime_exception_clears_authorizationand the display name"Tests cleanup when auth port throws runtime exception"were accurate when the production code wrapped non-domain failures in aRuntimeException. After this PR, the port throws anOutputPortException(checked) wrapping anIllegalStateException, and the assertion is now on the checkedLoginServiceException. Renaming would make the test intent clearer for future readers.♻️ Proposed rename
- `@DisplayName`("Tests cleanup when auth port throws runtime exception") - void runtime_exception_clears_authorization() { + `@DisplayName`("Tests cleanup when auth port wraps a non-domain failure in OutputPortException") + void output_port_failure_clears_authorization() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java` around lines 59 - 75, Rename the test method runtime_exception_clears_authorization and its `@DisplayName` to reflect that the port now throws a checked OutputPortException and the service translates it into a LoginServiceException; update the method name (e.g., output_port_exception_clears_authorization or login_service_throws_loginserviceexception_and_clears_authorization) and change the `@DisplayName` to something like "Tests cleanup when OutputPortException causes LoginServiceException", keeping assertions on LoginServiceException, OutputPortException wrapping IllegalStateException, and the AuthorizationHolder.get() null check unchanged; locate these in LoginServiceImplTest where OutputPort, OutputPortException, LoginServiceException, and AuthorizationHolder are used.src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java (1)
11-11: Consider extractingAdminPasswordResetRequestto a top-level DTO.Per the AI summary,
UserCreateRequestwas introduced as a top-level DTO underpl.vtt.wpi.core.domain.dtoto parameterizeUserCreateInputPort. KeepingAdminPasswordResetRequestas a public nested record insideAdminPasswordServiceImplcouples the input-port generic type to the implementation class symbol (e.g., adapters wiring this port must importAdminPasswordServiceImpl.AdminPasswordResetRequest). Extracting it for parity would make the port wiring cleaner and decouple adapters from the concrete service class.♻️ Sketch of the refactor
-public class AdminPasswordServiceImpl implements AdminPasswordService { - private final InputPort<AdminPasswordResetRequest> adminPasswordResetInputPort; +public class AdminPasswordServiceImpl implements AdminPasswordService { + private final InputPort<AdminPasswordResetRequest> adminPasswordResetInputPort; @@ - public record AdminPasswordResetRequest(String secureKey, PasswordDto passwordDto) { - } }Then add a new top-level file e.g.
src/main/java/pl/vtt/wpi/core/domain/dto/AdminPasswordResetRequest.java:package pl.vtt.wpi.core.domain.dto; public record AdminPasswordResetRequest(String secureKey, PasswordDto passwordDto) { }…and update the import in this file accordingly.
Also applies to: 40-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java` at line 11, AdminPasswordResetRequest is defined as a public nested record inside AdminPasswordServiceImpl which forces consumers to reference AdminPasswordServiceImpl.AdminPasswordResetRequest; extract it to a top-level DTO (e.g., package pl.vtt.wpi.core.domain.dto) as public record AdminPasswordResetRequest(String secureKey, PasswordDto passwordDto) and replace all usages/imports of AdminPasswordServiceImpl.AdminPasswordResetRequest with the new top-level AdminPasswordResetRequest, including the InputPort<AdminPasswordResetRequest> field in AdminPasswordServiceImpl and any adapter wiring that imports the nested type.src/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.java (1)
16-58: Solid coverage ofset(); consider addingread()tests too.The three scenarios for
set()(null input, missing pixel program withNoSuchElementExceptioncause, happy path) align well with the implementation. The PR description and AI summary indicateread()also now declaresthrows RuntimeDataServiceExceptionand wrapsOutputPortException. Adding a couple of tests forread()(success path andOutputPortException → RuntimeDataServiceExceptiontranslation) would round out coverage of the new contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.java` around lines 16 - 58, Add two unit tests for RuntimeDataServiceImpl.read(): one success test that configures the service (RuntimeDataServiceImpl) so its read() call returns a known RuntimeData and asserts the returned value equals that instance, and one error-translation test that makes the underlying output port throw an OutputPortException (or configure the read supplier to throw) and asserts read() throws RuntimeDataServiceException with the OutputPortException as the cause; reference RuntimeDataServiceImpl.read(), RuntimeDataServiceException, and OutputPortException when locating where to hook the mocks/suppliers and use a concrete RuntimeData instance for the success case.src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java (1)
12-34: Consider also covering theInputPortException → AdminPasswordServiceExceptionpath.Current tests cover the happy path and a blank-password validation failure. The exception-translation branch in
AdminPasswordServiceImpl(lines 32-37) — including cause unwrapping whene.getCause()is non-null — isn't exercised. A small additional test using anInputPortlambda that throwsInputPortExceptionwould lock in this contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java` around lines 12 - 34, Add a unit test that verifies AdminPasswordServiceImpl.resetPassword translates an InputPortException into an AdminPasswordServiceException (including the case where InputPortException.getCause() is non-null); create an AdminPasswordServiceImpl with an InputPort implementation that throws new InputPortException("msg") and another variant that throws new InputPortException("msg", new RuntimeException("cause")), call service.resetPassword("secure-key", valid PasswordDto) and assert that an AdminPasswordServiceException is thrown and that the original cause/message is preserved/accessible via getCause()/getMessage() as appropriate; reference AdminPasswordServiceImpl, InputPort, InputPortException, and AdminPasswordServiceException when locating where to add the test.src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java (1)
72-74: Optional: preferObjects.equalsfor the index comparison.
program.index() == runtimeData.pixelProgram()is currently safe (the null branch is short-circuited at L61), but it relies on auto-unboxing theIntegerreturned bypixelProgram(). A future refactor that removes the early null guard would silently introduce an NPE here. UsingObjects.equals(program.index(), runtimeData.pixelProgram())(or explicitruntimeData.pixelProgram().intValue()) makes the intent and null-safety explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java` around lines 72 - 74, Replace the direct == comparison between boxed integers with a null-safe equality check: in the stream anyMatch where you currently test program != null && program.index() == runtimeData.pixelProgram(), change the comparison to use Objects.equals(program.index(), runtimeData.pixelProgram()) (or call runtimeData.pixelProgram().intValue() if you prefer explicit unboxing) so the check is explicit about null-safety; update the import if needed and keep the surrounding null guard for pixelPrograms and program.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 29-32: The current catch only handles
IncorrectUsernameOrPasswordException and LoginServiceException, so unchecked
exceptions from authPort.load() can bypass AuthorizationHolder.clear(); modify
LoginServiceImpl to guarantee cleanup by wrapping the authPort.load()/post-load
logic in a try/finally that calls AuthorizationHolder.clear() on failure (or add
an additional catch for RuntimeException that calls AuthorizationHolder.clear()
and rethrows); ensure the finally/catch covers all exceptions so
AuthorizationHolder.clear() is always invoked before propagating errors from
authPort.load() or other unexpected failures.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java`:
- Around line 64-70: The catch block around pixelProgramsOutputPort.load()
should not convert an infrastructure OutputPortException into a
DataInconsistencyException; change it to rethrow a RuntimeDataServiceException
with the caught cause (keep the original message/context), and update any method
signatures that declare throwing DataInconsistencyException (e.g., the set
method and validatePixelProgramExists) so they only throw
DataInconsistencyException for the genuine "pixel program does not exist" path
while port failures propagate as RuntimeDataServiceException; locate and modify
the load call handling in RuntimeDataServiceImpl (and adjust
validatePixelProgramExists and set declarations/usages) accordingly.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.java`:
- Around line 38-47: In createUser, move the null-check for the User before
calling validatePassword so that a null user triggers
UserManagementServiceException("User cannot be null") immediately; specifically,
in UserManagementServiceImpl.update createUser(...) perform the if (user ==
null) guard first, then call validatePassword(passwordDto), then check existence
and throw UserAlreadyExistsException as before to ensure argument shape is
validated before content.
- Around line 44-49: The read-then-write sequence around loadUsers() and
userCreateRequestInputPort.send() has a race; guard the entire uniqueness
check+send with a ReentrantLock (similar to PixelProgramServiceImpl) by adding a
private final ReentrantLock (e.g., userLock) and wrapping the
loadUsers().stream().anyMatch(...) check and subsequent
userCreateRequestInputPort.send(new UserCreateRequest(...)) call inside
userLock.lock()/unlock() (use try/finally), and apply the same locking pattern
to removeUser() around its load/delete sequence; alternatively, if you choose
not to add a lock, add a clear javadoc on createUser/removeUser stating that
username uniqueness is enforced at the storage/port layer and must be atomic.
---
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/exception/DebugServiceException.java`:
- Around line 3-11: Add a brief Javadoc to the DebugServiceException class
describing when it is thrown (e.g., to wrap
InputPortException/OutputPortException from debug operations) and add an
explicit serialVersionUID field to avoid serialization warnings and
compatibility issues; specifically, update the DebugServiceException class to
include a short Javadoc above the class declaration and a private static final
long serialVersionUID = 1L (or a generated UID) while leaving the existing
constructors (DebugServiceException(String) and DebugServiceException(String,
Throwable)) intact.
In
`@src/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoServiceException.java`:
- Around line 3-11: Introduce a shared abstract base exception class (e.g.,
ApplicationServiceException) that extends Exception, declares a private static
final long serialVersionUID, and provides protected constructors (message and
message+cause); change DeviceInfoServiceException to extend this new
ApplicationServiceException and add its own serialVersionUID constant as well
(do similarly for other service exceptions like PixelProgramServiceException and
UserManagementServiceException) to remove constructor duplication and silence
serialization warnings.
In
`@src/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationServiceException.java`:
- Around line 3-11: Add a serialVersionUID field to
NetworkConfigurationServiceException to satisfy serialization linting and
stabilize its serialized form; specifically, inside the
NetworkConfigurationServiceException class (alongside the existing constructors)
declare a private static final long serialVersionUID with any fixed long literal
(e.g., 1L) so that the class explicitly defines its serialization identifier.
In
`@src/main/java/pl/vtt/wpi/core/application/exception/RebootServiceException.java`:
- Around line 3-11: The RebootServiceException class lacks an explicit
serialVersionUID which can break serialization compatibility; add a private
static final long serialVersionUID field to the RebootServiceException class
(e.g., in class RebootServiceException) with a chosen constant value to satisfy
the Serializable contract and silence IDE/lint warnings while preserving
deserialization stability across versions.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java`:
- Line 11: AdminPasswordResetRequest is defined as a public nested record inside
AdminPasswordServiceImpl which forces consumers to reference
AdminPasswordServiceImpl.AdminPasswordResetRequest; extract it to a top-level
DTO (e.g., package pl.vtt.wpi.core.domain.dto) as public record
AdminPasswordResetRequest(String secureKey, PasswordDto passwordDto) and replace
all usages/imports of AdminPasswordServiceImpl.AdminPasswordResetRequest with
the new top-level AdminPasswordResetRequest, including the
InputPort<AdminPasswordResetRequest> field in AdminPasswordServiceImpl and any
adapter wiring that imports the nested type.
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.java`:
- Around line 21-24: Replace the current unwrapping logic in RebootServiceImpl
that computes Throwable cause = e.getCause() == null ? e : e.getCause(); and
then throws new RebootServiceException("Cannot reboot device", cause) with
simply passing the original InputPortException as the cause (throw new
RebootServiceException("Cannot reboot device", e)); this preserves the full
exception chain (RebootServiceException → InputPortException → underlying cause)
while keeping the existing message; update any tests that assert on the exact
wrapped cause if necessary (e.g.,
RebootServiceImplTest#reboot_portFailure_wrapsException).
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java`:
- Around line 72-74: Replace the direct == comparison between boxed integers
with a null-safe equality check: in the stream anyMatch where you currently test
program != null && program.index() == runtimeData.pixelProgram(), change the
comparison to use Objects.equals(program.index(), runtimeData.pixelProgram())
(or call runtimeData.pixelProgram().intValue() if you prefer explicit unboxing)
so the check is explicit about null-safety; update the import if needed and keep
the surrounding null guard for pixelPrograms and program.
In
`@src/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.java`:
- Line 5: Import ordering is out of lexical order: move the import of
NetworkConfigurationServiceException so imports under the same root are sorted;
specifically, place
pl.vtt.wpi.core.application.exception.NetworkConfigurationServiceException
before the pl.vtt.wpi.core.domain.model.* imports in
NetworkConfigurationService.java (adjust other nearby imports if necessary) to
satisfy the project's sorted-import convention.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java`:
- Around line 12-34: Add a unit test that verifies
AdminPasswordServiceImpl.resetPassword translates an InputPortException into an
AdminPasswordServiceException (including the case where
InputPortException.getCause() is non-null); create an AdminPasswordServiceImpl
with an InputPort implementation that throws new InputPortException("msg") and
another variant that throws new InputPortException("msg", new
RuntimeException("cause")), call service.resetPassword("secure-key", valid
PasswordDto) and assert that an AdminPasswordServiceException is thrown and that
the original cause/message is preserved/accessible via getCause()/getMessage()
as appropriate; reference AdminPasswordServiceImpl, InputPort,
InputPortException, and AdminPasswordServiceException when locating where to add
the test.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java`:
- Around line 59-75: Rename the test method
runtime_exception_clears_authorization and its `@DisplayName` to reflect that the
port now throws a checked OutputPortException and the service translates it into
a LoginServiceException; update the method name (e.g.,
output_port_exception_clears_authorization or
login_service_throws_loginserviceexception_and_clears_authorization) and change
the `@DisplayName` to something like "Tests cleanup when OutputPortException
causes LoginServiceException", keeping assertions on LoginServiceException,
OutputPortException wrapping IllegalStateException, and the
AuthorizationHolder.get() null check unchanged; locate these in
LoginServiceImplTest where OutputPort, OutputPortException,
LoginServiceException, and AuthorizationHolder are used.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.java`:
- Around line 16-58: Add two unit tests for RuntimeDataServiceImpl.read(): one
success test that configures the service (RuntimeDataServiceImpl) so its read()
call returns a known RuntimeData and asserts the returned value equals that
instance, and one error-translation test that makes the underlying output port
throw an OutputPortException (or configure the read supplier to throw) and
asserts read() throws RuntimeDataServiceException with the OutputPortException
as the cause; reference RuntimeDataServiceImpl.read(),
RuntimeDataServiceException, and OutputPortException when locating where to hook
the mocks/suppliers and use a concrete RuntimeData instance for the success
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58ae5834-6e5c-47dd-ad3b-13926486fc7f
📒 Files selected for processing (36)
src/main/java/pl/vtt/wpi/core/application/exception/AdminPasswordServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/DebugServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/LoginServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/PixelProgramServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/RebootServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/RuntimeDataServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/UserManagementServiceException.javasrc/main/java/pl/vtt/wpi/core/application/service/AdminPasswordService.javasrc/main/java/pl/vtt/wpi/core/application/service/DebugService.javasrc/main/java/pl/vtt/wpi/core/application/service/DeviceInfoService.javasrc/main/java/pl/vtt/wpi/core/application/service/LoginService.javasrc/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.javasrc/main/java/pl/vtt/wpi/core/application/service/PixelProgramService.javasrc/main/java/pl/vtt/wpi/core/application/service/RebootService.javasrc/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.javasrc/main/java/pl/vtt/wpi/core/application/service/UserManagementService.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.java
✅ Files skipped from review due to trivial changes (4)
- src/main/java/pl/vtt/wpi/core/application/exception/LoginServiceException.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImplTest.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImplTest.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImplTest.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/DebugService.java
- src/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java
Contributor
Author
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad17cba7c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| List<PixelProgram> programs = new ArrayList<>(getAll()); | ||
| int nextIndex = programs.stream() | ||
| .map(PixelProgram::index) |
There was a problem hiding this comment.
In save, programs.stream().map(PixelProgram::index) will throw a NullPointerException when the loaded list contains a null element. This class already accounts for null entries elsewhere (findListIndex), so a null coming from pixelProgramsOutputPort.load() is a realistic corrupted-data path; in that case save fails with an unchecked exception instead of the declared PixelProgramOperationException, breaking the service contract for callers.
Useful? React with 👍 / 👎.
| validatePassword(passwordDto); | ||
| userLock.lock(); | ||
| try { | ||
| boolean exists = loadUsers().stream().anyMatch(existingUser -> existingUser.username().equals(user.username())); |
There was a problem hiding this comment.
Both createUser and removeUser dereference existingUser.username() directly while scanning loadUsers(). If the output port returns a list containing a null entry, this path throws NullPointerException (not wrapped), so callers receive an unexpected runtime failure instead of UserManagementOperationException/domain exceptions. Adding a null filter (or validating loadUsers output) would preserve the checked-exception contract.
Useful? React with 👍 / 👎.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.java (1)
23-24: ⚡ Quick win
OutputPortExceptioncontext is silently dropped when a nested cause exists.When
e.getCause() != null,causeis set toe.getCause()directly, bypassing theOutputPortExceptionitself. Any message or port-specific context carried by theOutputPortExceptionis discarded, andDeviceInfoReadException.getCause()points straight to the root cause. If a caller (or log line) chains throughgetCause()it will never see theOutputPortExceptionlayer.The more conventional approach is to always chain the caught exception, letting the full stack be traversable:
♻️ Proposed fix
- } catch (OutputPortException e) { - Throwable cause = e.getCause() == null ? e : e.getCause(); - throw new DeviceInfoReadException("Cannot read device info", cause); + } catch (OutputPortException e) { + throw new DeviceInfoReadException("Cannot read device info", e); }🤖 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/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.java` around lines 23 - 24, The current catch in DeviceInfoServiceImpl replaces the caught OutputPortException context by using e.getCause() when present, losing the OutputPortException layer; change the throw to always chain the caught exception e (the OutputPortException) as the cause when constructing DeviceInfoReadException so the full exception chain and port-specific context remain accessible (i.e., stop substituting e.getCause() and pass e as the cause to the DeviceInfoReadException).
🤖 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.
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.java`:
- Around line 23-24: The current catch in DeviceInfoServiceImpl replaces the
caught OutputPortException context by using e.getCause() when present, losing
the OutputPortException layer; change the throw to always chain the caught
exception e (the OutputPortException) as the cause when constructing
DeviceInfoReadException so the full exception chain and port-specific context
remain accessible (i.e., stop substituting e.getCause() and pass e as the cause
to the DeviceInfoReadException).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3de476ef-f913-426f-8fc9-a165a76992db
📒 Files selected for processing (35)
src/main/java/pl/vtt/wpi/core/application/exception/AdminPasswordResetException.javasrc/main/java/pl/vtt/wpi/core/application/exception/ApplicationServiceException.javasrc/main/java/pl/vtt/wpi/core/application/exception/AuthenticationServiceUnavailableException.javasrc/main/java/pl/vtt/wpi/core/application/exception/DebugDataAccessException.javasrc/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoReadException.javasrc/main/java/pl/vtt/wpi/core/application/exception/DeviceRebootException.javasrc/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationException.javasrc/main/java/pl/vtt/wpi/core/application/exception/PixelProgramOperationException.javasrc/main/java/pl/vtt/wpi/core/application/exception/RuntimeDataOperationException.javasrc/main/java/pl/vtt/wpi/core/application/exception/UserManagementOperationException.javasrc/main/java/pl/vtt/wpi/core/application/service/AdminPasswordService.javasrc/main/java/pl/vtt/wpi/core/application/service/DebugService.javasrc/main/java/pl/vtt/wpi/core/application/service/DeviceInfoService.javasrc/main/java/pl/vtt/wpi/core/application/service/LoginService.javasrc/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.javasrc/main/java/pl/vtt/wpi/core/application/service/PixelProgramService.javasrc/main/java/pl/vtt/wpi/core/application/service/RebootService.javasrc/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.javasrc/main/java/pl/vtt/wpi/core/application/service/UserManagementService.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/dto/AdminPasswordResetRequest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.java
✅ Files skipped from review due to trivial changes (12)
- src/main/java/pl/vtt/wpi/core/application/exception/ApplicationServiceException.java
- src/main/java/pl/vtt/wpi/core/domain/dto/AdminPasswordResetRequest.java
- src/main/java/pl/vtt/wpi/core/application/exception/UserManagementOperationException.java
- src/main/java/pl/vtt/wpi/core/application/exception/AuthenticationServiceUnavailableException.java
- src/main/java/pl/vtt/wpi/core/application/exception/NetworkConfigurationException.java
- src/main/java/pl/vtt/wpi/core/application/exception/DeviceRebootException.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/exception/DebugDataAccessException.java
- src/main/java/pl/vtt/wpi/core/application/exception/DeviceInfoReadException.java
- src/main/java/pl/vtt/wpi/core/application/exception/PixelProgramOperationException.java
- src/main/java/pl/vtt/wpi/core/application/exception/RuntimeDataOperationException.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/NetworkConfigurationServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (20)
- src/main/java/pl/vtt/wpi/core/application/service/DeviceInfoService.java
- src/main/java/pl/vtt/wpi/core/application/service/RebootService.java
- src/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.java
- src/main/java/pl/vtt/wpi/core/application/service/LoginService.java
- src/main/java/pl/vtt/wpi/core/application/service/DebugService.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImplTest.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/DeviceInfoServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/UserManagementService.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImpl.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/RebootServiceImpl.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/AdminPasswordServiceImplTest.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/UserManagementServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/DebugServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/PixelProgramServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/PixelProgramService.java
- src/main/java/pl/vtt/wpi/core/application/service/impl/RuntimeDataServiceImpl.java
- src/main/java/pl/vtt/wpi/core/application/service/RuntimeDataService.java
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.
Motivation
Description
README.mdto document service contracts, package layout changes, request model timestamp, and to list available application services and ports.pl.vtt.wpi.core.application.service.implimplementations:AdminPasswordServiceImpl,DebugServiceImpl,DeviceInfoServiceImpl,NetworkConfigurationServiceImpl,PixelProgramServiceImpl,RebootServiceImpl,RuntimeDataServiceImpl, andUserManagementServiceImpl.InputPortException/OutputPortExceptioninto appropriate runtime or domain exceptions while delegating I/O toInputPort/OutputPortinterfaces.PixelProgramServiceImplprovides full list-management operations (getAll/insert/set/get/save/update/remove) andUserManagementServiceImplimplements create/changePassword/remove with existence checks and password validation.Testing
mvn testwhich executes the JUnit Jupiter 5 unit tests.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests