Introduce core domain models, services, and request handling#7
Open
Introduce core domain models, services, and request handling#7
Conversation
Introduce Java record types for core domain models: Color (with 0–255 validation), User (with groups), AccessPointConfig and WiFiConfig, DeviceInfo, PixelProgram, RuntimeData, and CurrentState. CurrentState includes defensive copies for lists and a nested Builder to construct/modify immutable instances. These types model device configuration, runtime data and pixel/program state for the WPI core.
Introduce new service interfaces under pl.vtt.wpi.core.application.service to define contracts for core operations: AdminPasswordService (resetPassword), DebugService (pollLogs, peekLogs, getCurrentState), DeviceInfoService (read), NetworkConfigurationService (config for WiFi and AccessPoint), and RebootService (reboot). These are interface-only additions; implementations will be provided separately. Create RuntimeDataService and PixelProgramService Introduce core user management contracts and supporting types: add UserManagementService interface (createUser, changePassword, removeUser), PasswordDto record, and three exceptions (UserAlreadyExistsException, InvalidPasswordException, UserNotExistsException). Also export pl.vtt.wpi.core.domain.dto in module-info so the DTO is visible to consumers. These changes scaffold the core user lifecycle and password handling; implementations will be provided separately.
Add new domain types and refactor core models and services: - Add multiple color model records (RgbColor renamed from Color, plus CmykColor, HsbColor, HslColor, LabColor) with conversion helpers and validation. - Replace usages of generic Color with color.RgbColor across device models and services (PixelProgramService, CurrentState, PixelProgram, RuntimeData adjustments). - Introduce InputPort and OutputPort interfaces and specific exceptions (InputPortException, OutputPortException) with a sealed PortException. - Refactor request/response handling: Request now includes a timestamp and stricter validation; RequestFactory simplified to a single create(...) signature; add infrastructure Response record. - Refactor authorization/login: Authorization.basic(...) added (Base64 encoding), AuthorizationHolder API changed to accept username/password and Credentials; LoginServiceImpl updated to use AuthorizationHolder and new Request timestamp. - Add builders and defensive copy/null-handling to RuntimeData and CurrentState builders. - Update module exports and add maven.compiler.parameters property in pom.xml. These changes centralize auth encoding, add richer color domain models, improve request/response metadata, and introduce port abstractions for IO errors.
Introduce RequestSender interface and two InputPort implementations. - Add pl.vtt.wpi.core.domain.RequestSender for sending Request objects. - Add RuntimeDataInputPort: validates RuntimeData, picks Method.PUT when all fields present or Method.PATCH otherwise, builds a DATA_UPDATE request via RequestFactory and sends it, wrapping failures in InputPortException. - Add WifiConfigInputPort: validates WiFiConfig (ssid/password must be non-null), builds a PATCH WIFI_UPDATE request and sends it, wrapping failures in InputPortException. These changes wire request creation (RequestFactory) to request dispatch (RequestSender) for runtime data and WiFi configuration updates.
* test: expand login tests and update README * test: correct URL in LoginServiceImpl tests
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive port-adapter pattern for the wpi-core module, adding input and output port abstractions, service interfaces, rich domain models (colors, devices, users), and a complete set of concrete port implementations with supporting tests. It also refactors Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 291d98cd74
ℹ️ 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: 15
🧹 Nitpick comments (2)
src/main/java/pl/vtt/wpi/core/application/service/RebootService.java (1)
3-4: Consider aligningRebootvsRESTARTterminology.Line 3 and Line 4 use
Reboot, while the domain endpoint model usesRESTART(src/main/java/pl/vtt/wpi/core/domain/model/endpoint/RequestTarget.java:1-50andsrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java:1-50). Aligning vocabulary (or documenting the mapping) will reduce cognitive overhead across layers.🤖 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/RebootService.java` around lines 3 - 4, The service API uses "Reboot" (RebootService.reboot()) while the domain model uses "RESTART" (RequestTarget, Resource); unify terminology by renaming the service and method to use RESTART (e.g., RestartService.restart()) or add a clear mapping layer/documentation linking RebootService.reboot() to the domain's RESTART constants (update service interface name, method name, and any callers to match or add a translator class/method that maps Reboot -> RESTART and reference RequestTarget/Resource constants).src/main/java/pl/vtt/wpi/core/domain/model/device/DeviceInfo.java (1)
5-13: Consider enforcing a minimal invariant foruuid.Without a compact constructor, Line 6 currently allows
uuid == null, which can defer failures to later layers.Proposed refactor
package pl.vtt.wpi.core.domain.model.device; +import java.util.Objects; import java.util.UUID; @@ public record DeviceInfo( UUID uuid, String manufacturer, String productName, String applicationName, String applicationVersion, String applicationAuthor, String applicationAuthorEmail ) { + public DeviceInfo { + uuid = Objects.requireNonNull(uuid, "uuid must not be null"); + } }🤖 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/model/device/DeviceInfo.java` around lines 5 - 13, The DeviceInfo record allows uuid to be null; add a compact constructor for DeviceInfo that validates uuid (e.g., Objects.requireNonNull(uuid, "uuid must not be null")) to enforce the invariant early; implement the check inside the record's compact constructor so any creation of DeviceInfo (constructor, factory, or deserialization) will fail fast if uuid is null.
🤖 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/config/AuthorizationHolder.java`:
- Around line 20-24: In AuthorizationHolder.authorize(Credentials) ensure you
reject null credential fields before creating/storing the Basic header: after
the existing null check for credentials, validate credentials.username() and
credentials.token() are non-null (and optionally non-empty) and throw
IllegalArgumentException with a clear message if either is null; only then call
Authorization.basic(...) and proceed to cache/store the header so you never
encode a malformed "null:..." value.
In `@src/main/java/pl/vtt/wpi/core/domain/dto/PasswordDto.java`:
- Line 3: The PasswordDto record's auto-generated toString() exposes plaintext
passwords; override toString() in PasswordDto to return a redacted
representation (e.g., mask both password and passwordConfirmation with fixed
placeholder like "[REDACTED]" or show only length/asterisks) so logging or
debugging won't reveal secrets, ensuring the override is implemented on the
PasswordDto record declaration.
In `@src/main/java/pl/vtt/wpi/core/domain/model/Authorization.java`:
- Around line 18-20: The basic(...) factory method in Authorization silently
accepts null username/password (Java concatenates "null"), so modify
Authorization.basic(String username, String password) to validate inputs
up-front (e.g., Objects.requireNonNull or explicit checks) and throw a clear
exception (NullPointerException or IllegalArgumentException) when username or
password is null or empty; keep the rest of the logic (building credentials and
encoding) unchanged so only the null/empty guard around the Authorization.basic
method is added.
In `@src/main/java/pl/vtt/wpi/core/domain/model/color/CmykColor.java`:
- Around line 19-23: The toRgb method in CmykColor currently truncates
fractional channel values via (int) casts causing darker results; change the
conversion in toRgb to round each computed channel (use Math.round on the
floating result before casting to int) for r, g, b and ensure the final values
fed to the RgbColor constructor are proper ints (optionally clamp to 0–255) to
avoid off-by-one issues when creating the new RgbColor(r, g, b).
In `@src/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.java`:
- Around line 27-31: The XYZ→RGB step in LabColor is using incorrect
coefficients; update the conversion inside the Lab→RGB flow (in class LabColor,
the method that computes r,g,b from x,y,z) to use the standard sRGB linear
transform (apply the correct 3x3 matrix converting XYZ to linear RGB), then
clamp linear RGB to [0,1], apply the sRGB gamma correction (implement
gammaCorrect as described: clamp to [0,1], return 12.92*c for c<=0.0031308 else
1.055*pow(c,1/2.4)-0.055), convert to 0–255 ints and pass through the existing
clamp(...) before constructing new RgbColor; ensure you replace the current
float coefficients with the standard matrix and insert the gammaCorrect step
before integer conversion.
In `@src/main/java/pl/vtt/wpi/core/domain/model/device/AccessPointConfig.java`:
- Line 3: The AccessPointConfig record's default toString() exposes the
password; override toString() in the AccessPointConfig record to return only
non-sensitive fields (e.g., include ssid but mask or omit password), e.g.
implement AccessPointConfig.toString() that prints the ssid and a fixed masked
value like "<redacted>" or "***" for the password; ensure any existing usages
that rely on the default string format still work or are updated to use explicit
getters (ssid(), password()) when the raw password is required.
In `@src/main/java/pl/vtt/wpi/core/domain/model/device/CurrentState.java`:
- Around line 71-135: The Builder.from(CurrentState) method currently checks
this.<field> != null before copying, so calling new Builder().from(currentState)
copies nothing; change each condition to check the source (currentState.<field>
!= null) or use Objects.nonNull(currentState.<field>) and then assign
this.<field> = currentState.<field> for all fields (alive, time, waterflow,
sensorPin, offPin, action, programs, data, pixels, nextPixel, nol,
restartCountdown, pixelProgram, brightness, stepTime, on, sensorDependency,
timeDependency, overflow, onTime, offTime) so the builder properly copies
non-null values from the provided CurrentState.
In `@src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java`:
- Around line 49-50: The Builder.from method currently dereferences the incoming
RuntimeData instance (calling data.nol()) without a null check; add a null guard
at the start of Builder.from(RuntimeData data) that returns the Builder
unchanged (or throws a clear IllegalArgumentException if preferred) when data is
null, then proceed to copy fields (e.g., nol()) only when data is non-null so
new Builder().from(null) no longer NPEs.
- Around line 6-19: RuntimeData currently stores and exposes a mutable
java.util.TimeZone by reference; to prevent external mutation, defensively clone
the TimeZone whenever it's accepted or returned: in the RuntimeData canonical
constructor replace direct assignment with timeZone == null ? null : (TimeZone)
timeZone.clone(); override the record accessor timeZone() to return timeZone ==
null ? null : (TimeZone) timeZone.clone(); and update Builder.from(), the
builder's timeZone(...) setter/accessor, and build() to copy incoming TimeZone
using (TimeZone) timeZone.clone() (or null if absent) so all stored and returned
TimeZone instances are independent copies.
In `@src/main/java/pl/vtt/wpi/core/domain/model/device/WiFiConfig.java`:
- Line 3: The WiFiConfig record currently exposes the plaintext password via its
auto-generated toString; override the toString() in WiFiConfig to prevent
credential leakage by returning a string that includes the ssid but masks or
omits the password (e.g., "WiFiConfig{ssid='...', password=****}"); implement a
public String toString() method inside the record (keeping the record components
unchanged) modeled after the safe toString used in Authorization.java.
In `@src/main/java/pl/vtt/wpi/core/domain/model/User.java`:
- Line 7: The User record stores the mutable Set reference directly; add a
compact constructor for User that defensively copies the incoming groups with
Set.copyOf(groups) (and validate non-null if desired) so the record holds an
immutable copy; mirror the pattern used in EndpointDescriptor to locate the
intended behavior and replace the direct assignment with Set.copyOf in the
compact constructor for User.
In `@src/main/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPort.java`:
- Around line 37-43: The request construction via requestFactory.create in
RuntimeDataInputPort is not covered by the existing try/catch, so failures from
create() escape the port boundary; modify the sendRuntimeData (or the method
containing Request<RuntimeData> request = requestFactory.create(...)) to include
requestFactory.create(...) inside the try block (or wrap it in its own try) and
catch Exception, then rethrow an InputPortException with a descriptive message
and the caught exception as the cause, ensuring both the creation and
requestSender.send(...) errors are translated to InputPortException.
In `@src/main/java/pl/vtt/wpi/core/domain/port/WifiConfigInputPort.java`:
- Around line 31-37: The Request creation with requestFactory.create(...) is
outside the try block so its failures escape as raw exceptions; move the
Request<WiFiConfig> request = requestFactory.create(Method.PATCH,
RequestTarget.WIFI_UPDATE, obj) into the same try that calls
requestSender.send(request) (or expand the try to include the create call) and
on any exception throw the same InputPortException("Cannot send the WiFi
config", e) so both creation and send failures are wrapped consistently; update
the method in WifiConfigInputPort accordingly around the requestFactory.create
and requestSender.send calls.
In `@src/main/java/pl/vtt/wpi/core/domain/RequestSender.java`:
- Line 6: Change the overly-broad signature of RequestSender.send(Request<?>)
from "throws Exception" to a sealed-domain exception (e.g., throws
PortException) so the interface respects the sealed PortException hierarchy;
update callers like WifiConfigInputPort and RuntimeDataInputPort to either
propagate the PortException directly or to convert lower-level errors into the
permitted InputPortException/OutputPortException as appropriate, or if a new
domain exception is truly required, add it to PortException's permits clause and
use it in the send signature.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java`:
- Line 30: The lambdas using "_" as a parameter name (e.g., the lambda paired
with AuthorizationHolder::get like "URL, AuthorizationHolder::get, _ -> new
Credentials(username, token)") will not compile on Java 9+; rename that
parameter to a valid identifier such as "ignored" or "unused" in each occurrence
(lines referenced around the lambdas at positions 30, 53, 68, 81, 94, 107, 119,
132) so the form becomes e.g. "URL, AuthorizationHolder::get, ignored -> new
Credentials(username, token)"; update all similar lambda expressions in
LoginServiceImplTest to use the new name.
---
Nitpick comments:
In `@src/main/java/pl/vtt/wpi/core/application/service/RebootService.java`:
- Around line 3-4: The service API uses "Reboot" (RebootService.reboot()) while
the domain model uses "RESTART" (RequestTarget, Resource); unify terminology by
renaming the service and method to use RESTART (e.g., RestartService.restart())
or add a clear mapping layer/documentation linking RebootService.reboot() to the
domain's RESTART constants (update service interface name, method name, and any
callers to match or add a translator class/method that maps Reboot -> RESTART
and reference RequestTarget/Resource constants).
In `@src/main/java/pl/vtt/wpi/core/domain/model/device/DeviceInfo.java`:
- Around line 5-13: The DeviceInfo record allows uuid to be null; add a compact
constructor for DeviceInfo that validates uuid (e.g.,
Objects.requireNonNull(uuid, "uuid must not be null")) to enforce the invariant
early; implement the check inside the record's compact constructor so any
creation of DeviceInfo (constructor, factory, or deserialization) will fail fast
if uuid is null.
🪄 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: 4ca11525-59c0-47b8-b310-2b1a271d18d6
📒 Files selected for processing (44)
README.mdpom.xmlsrc/main/java/module-info.javasrc/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.javasrc/main/java/pl/vtt/wpi/core/application/exception/DataInconsistencyException.javasrc/main/java/pl/vtt/wpi/core/application/exception/InvalidPasswordException.javasrc/main/java/pl/vtt/wpi/core/application/exception/PixelProgramNotFoundException.javasrc/main/java/pl/vtt/wpi/core/application/exception/UserAlreadyExistsException.javasrc/main/java/pl/vtt/wpi/core/application/exception/UserNotExistsException.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/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/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/util/RequestFactory.javasrc/main/java/pl/vtt/wpi/core/domain/InputPort.javasrc/main/java/pl/vtt/wpi/core/domain/OutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/RequestSender.javasrc/main/java/pl/vtt/wpi/core/domain/dto/PasswordDto.javasrc/main/java/pl/vtt/wpi/core/domain/exception/InputPortException.javasrc/main/java/pl/vtt/wpi/core/domain/exception/OutputPortException.javasrc/main/java/pl/vtt/wpi/core/domain/exception/PortException.javasrc/main/java/pl/vtt/wpi/core/domain/model/Authorization.javasrc/main/java/pl/vtt/wpi/core/domain/model/Request.javasrc/main/java/pl/vtt/wpi/core/domain/model/User.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/CmykColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/HsbColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/HslColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/RgbColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/AccessPointConfig.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/CurrentState.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/DeviceInfo.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/PixelProgram.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/WiFiConfig.javasrc/main/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/WifiConfigInputPort.javasrc/main/java/pl/vtt/wpi/core/infrastructure/Response.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
Add null-checks and tighter validation across domain and config (AuthorizationHolder, User, RuntimeData.Builder, CurrentState.Builder). Mask sensitive fields in toString implementations for PasswordDto, AccessPointConfig and new WifiConfig. Rename WiFiConfig -> WifiConfig and update all usages (NetworkConfigurationService, WifiConfigInputPort). Introduce SendingException and update RequestSender signature to use it. Replace TimeZone with ZoneId in RuntimeData and related builder/accessors. Improve color conversion math: CMYK rounding and Lab->RGB conversion with proper linear transform, gamma correction and clamping. Fix minor request creation/sending ordering in RuntimeDataInputPort.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPort.java (1)
29-34: Consider extracting PATCH-detection predicate for maintainability.The null-check chain is long; moving it to a private helper will simplify
send(...)and make this rule easier to unit test.♻️ Suggested refactor
public class RuntimeDataInputPort implements InputPort<RuntimeData> { @@ `@Override` public void send(RuntimeData obj) throws InputPortException { if (obj == null) { throw new InputPortException("Runtime data cannot be null"); } - Method method = Method.PUT; - if (obj.nol() == null || obj.brightness() == null || obj.on() == null - || obj.overflow() == null || obj.sensorDependency() == null - || obj.timeDependency() == null || obj.defaultRestartCountdown() == null - || obj.offTime() == null || obj.onTime() == null - || obj.zoneId() == null || obj.pixelProgram() == null - || obj.stepTime() == null) { - method = Method.PATCH; - } + Method method = isPartialUpdate(obj) ? Method.PATCH : Method.PUT; try { Request<RuntimeData> request = requestFactory.create( method, RequestTarget.DATA_UPDATE, obj ); requestSender.send(request); } catch (Exception e) { throw new InputPortException("Cannot send the runtime data", e); } } + + private boolean isPartialUpdate(RuntimeData obj) { + return obj.nol() == null || obj.brightness() == null || obj.on() == null + || obj.overflow() == null || obj.sensorDependency() == null + || obj.timeDependency() == null || obj.defaultRestartCountdown() == null + || obj.offTime() == null || obj.onTime() == null + || obj.zoneId() == null || obj.pixelProgram() == null + || obj.stepTime() == null; + } }🤖 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/RuntimeDataInputPort.java` around lines 29 - 34, Extract the long null-check chain used to detect PATCH payloads from the send(...) method into a private helper on the RuntimeDataInputPort class (e.g., private boolean isPatchPayload(RuntimeData obj)); move the existing condition that checks obj.nol(), obj.brightness(), obj.on(), obj.overflow(), obj.sensorDependency(), obj.timeDependency(), obj.defaultRestartCountdown(), obj.offTime(), obj.onTime(), obj.zoneId(), obj.pixelProgram(), and obj.stepTime() into that helper and replace the inlined condition in send(...) with a call to isPatchPayload(obj); keep semantics identical and add unit tests targeting isPatchPayload to verify the predicate behavior.
🤖 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/domain/model/color/LabColor.java`:
- Around line 22-24: The Lab→XYZ conversion currently compares x/y/z to 0.008856
(the cubic threshold) but those variables hold the linear t values; change the
pivot check to test the linear t against 0.206896 (≈6/29) — i.e., compute
tX/tY/tZ (the pre-cube t values used for the ternary) and use (t > 0.206896) to
decide between Math.pow(t,3) and the linear formula, then multiply by the
reference white (95.047f, 100.000f, 108.883f). Update the code in LabColor.java
(the Lab→XYZ conversion block that sets x, y, z) to use these t-based
comparisons and operations.
---
Nitpick comments:
In `@src/main/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPort.java`:
- Around line 29-34: Extract the long null-check chain used to detect PATCH
payloads from the send(...) method into a private helper on the
RuntimeDataInputPort class (e.g., private boolean isPatchPayload(RuntimeData
obj)); move the existing condition that checks obj.nol(), obj.brightness(),
obj.on(), obj.overflow(), obj.sensorDependency(), obj.timeDependency(),
obj.defaultRestartCountdown(), obj.offTime(), obj.onTime(), obj.zoneId(),
obj.pixelProgram(), and obj.stepTime() into that helper and replace the inlined
condition in send(...) with a call to isPatchPayload(obj); keep semantics
identical and add unit tests targeting isPatchPayload to verify the predicate
behavior.
🪄 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: 9f59915d-850e-435e-b3cf-ce9b0b410d05
📒 Files selected for processing (14)
src/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.javasrc/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.javasrc/main/java/pl/vtt/wpi/core/domain/RequestSender.javasrc/main/java/pl/vtt/wpi/core/domain/dto/PasswordDto.javasrc/main/java/pl/vtt/wpi/core/domain/exception/SendingException.javasrc/main/java/pl/vtt/wpi/core/domain/model/User.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/CmykColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/AccessPointConfig.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/CurrentState.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.javasrc/main/java/pl/vtt/wpi/core/domain/model/device/WifiConfig.javasrc/main/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/WifiConfigInputPort.java
✅ Files skipped from review due to trivial changes (4)
- src/main/java/pl/vtt/wpi/core/domain/RequestSender.java
- src/main/java/pl/vtt/wpi/core/domain/exception/SendingException.java
- src/main/java/pl/vtt/wpi/core/domain/model/device/WifiConfig.java
- src/main/java/pl/vtt/wpi/core/application/service/NetworkConfigurationService.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/pl/vtt/wpi/core/domain/dto/PasswordDto.java
- src/main/java/pl/vtt/wpi/core/domain/model/device/AccessPointConfig.java
- src/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.java
- src/main/java/pl/vtt/wpi/core/domain/model/User.java
- src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java
- src/main/java/pl/vtt/wpi/core/domain/model/device/CurrentState.java
Introduce a new Color utility class with factory methods for creating Rgb/Cmyk/Hsl/Hsb/Lab colors and conversions from integer RGB (including CMYK derivation). Refactor LabColor: rename record component `l` to `lightness`, update validation messages, extract labPivot() helper and use it to clean up Lab->XYZ conversion math in toRgb(). Note: renaming the LabColor component is a public API change and may break source/binary compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/domain/model/color/Color.java`:
- Around line 23-30: The cmyk(int rgb) method computes C/M/Y as simple inverses
of RGB channels and ignores K, which yields incorrect CMYK values; update
cmyk(int rgb) in Color so that you compute k from max(R,G,B) as k = 1 - max/255,
then if (1 - k) > 0 compute c = (1 - r/255 - k) / (1 - k), m = (1 - g/255 - k) /
(1 - k), y = (1 - b/255 - k) / (1 - k), convert those to percentages/integers
consistently, and return new CmykColor(c,m,y,k); use existing rgb(int) →
RgbColor to get red/green/blue and ensure safe handling when (1 - k) == 0
(black) to avoid division by zero.
In `@src/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.java`:
- Around line 5-13: The LabColor constructor currently only checks numeric
ranges but allows NaN/Infinity because float comparisons fail; update the
constructor in class LabColor to first validate Float.isFinite(lightness),
Float.isFinite(a) and Float.isFinite(b) and throw IllegalArgumentException (or
same exception style) if any are not finite, then proceed with the existing
range checks for lightness, a and b to reject out-of-range values.
🪄 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: a99c1773-5fb7-4ae1-aaa0-cf929323cd44
📒 Files selected for processing (2)
src/main/java/pl/vtt/wpi/core/domain/model/color/Color.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.java
Refactor CMYK conversion to use normalized RGB (double) and compute K as 1 - max(r,g,b). Handle the edge case when K is 1.0 (avoid division-by-zero) by setting C/M/Y to 0, and otherwise compute C/M/Y with the standard formula, rounding to integer percentages. Also add validation in LabColor constructor to reject non-finite components (NaN/Infinity) and keep the existing lightness range check. These changes improve correctness and robustness of color conversions and prevent invalid Lab instances.
Move null validation for username/password from AuthorizationHolder.authorize into Authorization.basic so credentials are validated at construction of the Authorization record. Also remove trailing whitespace in Color.java. This centralizes validation and keeps the holder method simpler.
* Replace generic ports with model-specific port implementations * Add action/auth ports and refactor login to use ports * Fix user port tests to avoid Set.of null-check NPE * Expand UserCreateInputPortTest coverage and invocation checks * Strengthen AuthOutputPort cause propagation assertions * Align UserCreateInputPortTest with requested mock-style scenarios
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/main/java/pl/vtt/wpi/core/infrastructure/RequestHandler.java (1)
3-4: Replacethrows Exceptionwith a dedicated handler exception type.The broad
throws Exceptioncontract forces all consumers to catch withcatch (Exception e)and immediately wrap it into domain-specific exceptions (InputPortException,OutputPortException). This pattern appears across 13+ implementations across input and output port modules. A dedicated handler exception would eliminate the boilerplate wrapping and provide clearer failure semantics at the API boundary—either as a checked exception (e.g.,RequestHandledException) or an unchecked wrapper for consistency with your port exception hierarchy.🤖 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/infrastructure/RequestHandler.java` around lines 3 - 4, Replace the broad throws Exception on the RequestHandler.handle(Request<T>) signature with a dedicated handler exception (e.g., RequestHandlerException or RequestHandledException) and update all implementors to throw or wrap only that new exception instead of Exception; specifically change the interface method signature in RequestHandler to declare the new exception type, create the new checked or unchecked exception class to align with your port exception hierarchy, and refactor the 13+ implementations that currently catch/throw Exception to either propagate the new RequestHandlerException or convert internal errors to it so callers no longer need catch (Exception) and can handle InputPortException/OutputPortException and the new handler exception distinctly.src/main/java/pl/vtt/wpi/core/domain/port/input/LogsDeleteInputPort.java (1)
26-27: Narrow the catch scope to runtime failures.Catching
Exceptionis broader than this method’s collaborator contracts require;RuntimeExceptioncommunicates intent more clearly.♻️ Proposed change
- } catch (Exception e) { + } catch (RuntimeException e) { throw new InputPortException("Cannot delete logs", 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/LogsDeleteInputPort.java` around lines 26 - 27, The catch block in LogsDeleteInputPort currently catches Exception too broadly; change the catch(Exception e) to catch(RuntimeException e) in the method where InputPortException("Cannot delete logs", e) is thrown so only runtime failures are propagated as InputPortException; keep passing the caught exception as the cause to the InputPortException constructor (verify InputPortException supports a cause) and leave other checked exceptions to be handled by callers or declared.src/test/java/pl/vtt/wpi/core/domain/port/DeviceInfoOutputPortTest.java (2)
23-24: Assert HTTP method in the success-path test.The test validates endpoint URL but not that
GETis used, which is part of the port contract.Proposed test addition
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import pl.vtt.wpi.core.domain.model.endpoint.Method; @@ RequestHandler<Void, DeviceInfo> requestHandler = request -> { + assertEquals(Method.GET, request.method()); assertEquals(RequestTarget.INFO.descriptor().resource().url("http://localhost"), request.url()); return new DeviceInfo(UUID.randomUUID(), "VTT", "WP", "wpi", "1.0", "VTT", "test@vtt.pl"); };🤖 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/DeviceInfoOutputPortTest.java` around lines 23 - 24, In DeviceInfoOutputPortTest, add an assertion that the outgoing request uses the GET method to validate the port contract; specifically, after asserting request.url() in the success-path test, assert that request.method() (or request.getMethod() depending on the request type) equals "GET" (or HttpMethod.GET) so the test verifies both URL and HTTP method for RequestTarget.INFO.
43-45: Verify wrapped cause in exception-path test.The implementation wraps with original cause; asserting it here will prevent regression in error propagation.
Proposed test addition
OutputPortException exception = assertThrows(OutputPortException.class, port::load); assertEquals("Cannot load device info", exception.getMessage()); + assertEquals(IllegalStateException.class, exception.getCause().getClass()); + assertEquals("boom", exception.getCause().getMessage());🤖 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/DeviceInfoOutputPortTest.java` around lines 43 - 45, The test currently asserts only the OutputPortException message but must also verify the wrapped cause is preserved: when you set up the mock to throw the original cause (e.g., a RuntimeException originalCause = new RuntimeException("...") used in your stub for port.load), capture that same originalCause and add an assertion like assertSame(originalCause, exception.getCause()) (or assertEquals on cause type/message) in DeviceInfoOutputPortTest after the existing message assertion to ensure the implementation wraps and exposes the original cause from port::load.src/test/java/pl/vtt/wpi/core/domain/port/RestartInputPortTest.java (1)
16-37: Add coverage for endpoint target and sender-failure wrapping.Current tests verify method and factory-failure wrapping, but not RESTART target routing or
RequestSenderfailure wrapping.Proposed additions
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import pl.vtt.wpi.core.domain.model.endpoint.RequestTarget; @@ assertEquals(Method.POST, sent.get().method()); + assertEquals(RequestTarget.RESTART.descriptor().resource().url("http://localhost"), sent.get().url()); @@ void send_wrapsException() { @@ assertEquals("Cannot restart device", exception.getMessage()); } + + `@Test` + void send_wrapsSenderException() { + RequestFactory<Void> requestFactory = (method, target, payload) -> + new Request<>(null, method, target.descriptor().resource().url("http://localhost"), null, payload); + RestartInputPort port = new RestartInputPort(requestFactory, request -> { + throw new IllegalStateException("boom"); + }); + + InputPortException exception = assertThrows(InputPortException.class, () -> port.send(null)); + assertEquals("Cannot restart device", exception.getMessage()); + assertEquals(IllegalStateException.class, exception.getCause().getClass()); + }🤖 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/RestartInputPortTest.java` around lines 16 - 37, Add two tests to cover endpoint routing and sender-failure wrapping: (1) a test where RequestFactory captures the passed-in target argument when RestartInputPort.send(null) is called and asserts that the target corresponds to the RESTART endpoint (inspect target.descriptor().resource()/name() or compare to the concrete RESTART target constant used by RestartInputPort) to verify correct routing; (2) a test where RequestSender implementation throws a RuntimeException and assert that RestartInputPort.send(null) throws an InputPortException with message "Cannot restart device" and that the original exception is preserved as the cause. Ensure you use the existing RestartInputPort constructor signatures, RequestFactory and RequestSender lambdas, and assertions similar to the existing tests to integrate these cases.src/test/java/pl/vtt/wpi/core/domain/port/PixelProgramsInputPortTest.java (1)
40-43: Also assert the request target endpoint in the PUT-path test.Method-only assertions can miss regressions where the wrong endpoint is called with the correct verb.
Proposed test addition
import pl.vtt.wpi.core.domain.model.endpoint.Method; +import pl.vtt.wpi.core.domain.model.endpoint.RequestTarget; @@ Request<?> request = sent.get(); assertNotNull(request); assertEquals(Method.PUT, request.method()); + assertEquals(RequestTarget.PROGRAMS.descriptor().resource().url("http://localhost"), request.url());🤖 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/PixelProgramsInputPortTest.java` around lines 40 - 43, Add an assertion that verifies the request target endpoint in addition to the method: after obtaining Request<?> request = sent.get() and asserting Method.PUT, assert that the request's target/path equals the expected endpoint (e.g., assertEquals(expectedEndpoint, request.target()) or assertEquals(expectedEndpoint, request.uri().getPath()) depending on the Request API). Use the same endpoint constant from the production code (for example the PixelProgramsInputPort endpoint constant) or the exact expected string (e.g. "/v1/pixel-programs") to avoid hardcoding a mismatched value.
🤖 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 30-33: The constructor LoginServiceImpl currently accepts an
arbitrary Supplier<Authorization> which allows authPort.load() to send an
authorization different from the credentials seeded by login(); change the
constructor to use AuthorizationHolder::get (or accept an AuthorizationHolder
instance) so the auth supplier is always the AuthorizationHolder used by
login(), and update LoginRequestFactory.create to stop hardcoding POST — make it
respect the HTTP method chosen by AuthOutputPort (or accept the method as a
parameter) so AuthOutputPort and LoginRequestFactory produce requests
consistently; ensure login() seeds AuthorizationHolder before any call to
authPort.load().
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/PixelProgramsInputPort.java`:
- Around line 25-27: The current null-check only validates obj but not its
contents; update the PixelProgramsInputPort logic that handles the
List<PixelProgram> (the obj variable) to iterate the list and throw
InputPortException if any element is null (or use Objects.requireNonNull on each
entry) so null entries are rejected early; reference the
List<PixelProgram>/PixelProgram elements and the existing InputPortException for
consistency.
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/RestartInputPort.java`:
- Around line 26-27: The catch block in RestartInputPort currently wraps all
Exceptions into an InputPortException without preserving interruption semantics;
update the error handling so that InterruptedException is handled
specially—either add a separate catch (InterruptedException ie) that calls
Thread.currentThread().interrupt() and then throws new
InputPortException("Cannot restart device", ie), or inside the existing catch
check if (e instanceof InterruptedException) Thread.currentThread().interrupt();
before rethrowing new InputPortException("Cannot restart device", e); ensure
InputPortException is still thrown but the thread's interrupted status is
restored.
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/RuntimeDataInputPort.java`:
- Around line 29-35: The current check in RuntimeDataInputPort that sets method
= Method.PATCH treats any null in fields like onTime/offTime/zoneId as a partial
update even when timeDependency or sensorDependency explicitly allow those to be
null; update the conditional so that nulls for time-related fields are only
considered "partial" when Boolean.TRUE.equals(obj.timeDependency()) and likewise
only consider sensor-related fields partial when
Boolean.TRUE.equals(obj.sensorDependency()); leave other independent fields in
the original null-check. Locate the null-check block referencing obj.nol(),
obj.brightness(), obj.on(), obj.overflow(), obj.sensorDependency(),
obj.timeDependency(), obj.defaultRestartCountdown(), obj.offTime(),
obj.onTime(), obj.zoneId(), obj.pixelProgram(), obj.stepTime() and adjust the
logic to gate required presence of onTime/offTime/zoneId (and any
sensor-dependent fields) behind the corresponding dependency booleans before
setting Method.PATCH.
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/WifiConfigInputPort.java`:
- Around line 28-30: The current null-only check in the WifiConfigInputPort
validation allows empty or whitespace-only SSID/password; update the validation
that checks obj.ssid() and obj.password() to reject blank values as well (e.g.,
use String.isBlank() or trim().isEmpty()) and throw the existing
InputPortException with the same message; modify the guard around obj.ssid() ==
null || obj.password() == null in the method that performs validation
(references to obj.ssid() and obj.password()) so it also checks for blank/empty
strings before dispatching the update.
In `@src/main/java/pl/vtt/wpi/core/domain/port/output/DeviceInfoOutputPort.java`:
- Around line 25-27: The catch-all currently swallows InterruptedException;
change the exception handling in the DeviceInfoOutputPort code so
InterruptedException is caught separately, restore the interrupt status with
Thread.currentThread().interrupt(), and then wrap/throw it as an
OutputPortException (or rethrow) rather than letting it be absorbed — keep the
existing catch(Exception e) for other errors; reference OutputPortException and
the catch block in DeviceInfoOutputPort to locate the change.
In `@src/main/java/pl/vtt/wpi/core/domain/port/output/UsersOutputPort.java`:
- Around line 26-27: The catch in UsersOutputPort that currently wraps all
Exceptions into OutputPortException is losing the interrupt status; update the
catch logic so that if the caught exception is an InterruptedException (or has
an InterruptedException cause) you call Thread.currentThread().interrupt()
before throwing the new OutputPortException, then throw the OutputPortException
as before; locate the catch block that throws new OutputPortException("Cannot
load users", e) and add the interrupt re-signal handling there.
In `@src/test/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPortTest.java`:
- Around line 25-26: The RuntimeData construction in RuntimeDataInputPortTest
uses the wrong arity and types; update the RuntimeData(...) call to pass
arguments in the exact order and types defined by the RuntimeData record (use
the main RuntimeData record's field list/signatures from
src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java) so the
constructor call matches the record signature, and remove the now-unused
LocalTime imports from the test. Ensure you reference the RuntimeData record's
field names/types when reordering or changing values so compilation succeeds.
In `@src/test/java/pl/vtt/wpi/core/domain/port/WifiConfigInputPortTest.java`:
- Around line 30-35: Test currently only checks password == null; update the
test to also assert the SSID-missing case by calling WifiConfigInputPort.send
with a WifiConfig that has null SSID (e.g., new WifiConfig(null, "password"))
and expecting InputPortException, or rename the test to reflect it only covers
the password-null case; reference WifiConfigInputPort.send, WifiConfig, and
InputPortException when adding the additional assertion.
---
Nitpick comments:
In `@src/main/java/pl/vtt/wpi/core/domain/port/input/LogsDeleteInputPort.java`:
- Around line 26-27: The catch block in LogsDeleteInputPort currently catches
Exception too broadly; change the catch(Exception e) to catch(RuntimeException
e) in the method where InputPortException("Cannot delete logs", e) is thrown so
only runtime failures are propagated as InputPortException; keep passing the
caught exception as the cause to the InputPortException constructor (verify
InputPortException supports a cause) and leave other checked exceptions to be
handled by callers or declared.
In `@src/main/java/pl/vtt/wpi/core/infrastructure/RequestHandler.java`:
- Around line 3-4: Replace the broad throws Exception on the
RequestHandler.handle(Request<T>) signature with a dedicated handler exception
(e.g., RequestHandlerException or RequestHandledException) and update all
implementors to throw or wrap only that new exception instead of Exception;
specifically change the interface method signature in RequestHandler to declare
the new exception type, create the new checked or unchecked exception class to
align with your port exception hierarchy, and refactor the 13+ implementations
that currently catch/throw Exception to either propagate the new
RequestHandlerException or convert internal errors to it so callers no longer
need catch (Exception) and can handle InputPortException/OutputPortException and
the new handler exception distinctly.
In `@src/test/java/pl/vtt/wpi/core/domain/port/DeviceInfoOutputPortTest.java`:
- Around line 23-24: In DeviceInfoOutputPortTest, add an assertion that the
outgoing request uses the GET method to validate the port contract;
specifically, after asserting request.url() in the success-path test, assert
that request.method() (or request.getMethod() depending on the request type)
equals "GET" (or HttpMethod.GET) so the test verifies both URL and HTTP method
for RequestTarget.INFO.
- Around line 43-45: The test currently asserts only the OutputPortException
message but must also verify the wrapped cause is preserved: when you set up the
mock to throw the original cause (e.g., a RuntimeException originalCause = new
RuntimeException("...") used in your stub for port.load), capture that same
originalCause and add an assertion like assertSame(originalCause,
exception.getCause()) (or assertEquals on cause type/message) in
DeviceInfoOutputPortTest after the existing message assertion to ensure the
implementation wraps and exposes the original cause from port::load.
In `@src/test/java/pl/vtt/wpi/core/domain/port/PixelProgramsInputPortTest.java`:
- Around line 40-43: Add an assertion that verifies the request target endpoint
in addition to the method: after obtaining Request<?> request = sent.get() and
asserting Method.PUT, assert that the request's target/path equals the expected
endpoint (e.g., assertEquals(expectedEndpoint, request.target()) or
assertEquals(expectedEndpoint, request.uri().getPath()) depending on the Request
API). Use the same endpoint constant from the production code (for example the
PixelProgramsInputPort endpoint constant) or the exact expected string (e.g.
"/v1/pixel-programs") to avoid hardcoding a mismatched value.
In `@src/test/java/pl/vtt/wpi/core/domain/port/RestartInputPortTest.java`:
- Around line 16-37: Add two tests to cover endpoint routing and sender-failure
wrapping: (1) a test where RequestFactory captures the passed-in target argument
when RestartInputPort.send(null) is called and asserts that the target
corresponds to the RESTART endpoint (inspect
target.descriptor().resource()/name() or compare to the concrete RESTART target
constant used by RestartInputPort) to verify correct routing; (2) a test where
RequestSender implementation throws a RuntimeException and assert that
RestartInputPort.send(null) throws an InputPortException with message "Cannot
restart device" and that the original exception is preserved as the cause.
Ensure you use the existing RestartInputPort constructor signatures,
RequestFactory and RequestSender lambdas, and assertions similar to the existing
tests to integrate these cases.
🪄 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: 46ebd658-f271-4294-8b39-78bcb9cb40fd
📒 Files selected for processing (43)
README.mdsrc/main/java/module-info.javasrc/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/model/Authorization.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/Color.javasrc/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.javasrc/main/java/pl/vtt/wpi/core/domain/port/InputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/OutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/exception/InputPortException.javasrc/main/java/pl/vtt/wpi/core/domain/port/exception/OutputPortException.javasrc/main/java/pl/vtt/wpi/core/domain/port/exception/PortException.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/LogsDeleteInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/PixelProgramsInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/RestartInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/RuntimeDataInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/UserCreateInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/input/WifiConfigInputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/AuthOutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/CurrentStateOutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/DeviceInfoOutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/PixelProgramsOutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/RuntimeDataOutputPort.javasrc/main/java/pl/vtt/wpi/core/domain/port/output/UsersOutputPort.javasrc/main/java/pl/vtt/wpi/core/infrastructure/Request.javasrc/main/java/pl/vtt/wpi/core/infrastructure/RequestFactory.javasrc/main/java/pl/vtt/wpi/core/infrastructure/RequestHandler.javasrc/main/java/pl/vtt/wpi/core/infrastructure/RequestSender.javasrc/main/java/pl/vtt/wpi/core/infrastructure/Response.javasrc/main/java/pl/vtt/wpi/core/infrastructure/exception/SendingException.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/AuthOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/CurrentStateOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/DeviceInfoOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/LogsDeleteInputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/PixelProgramsInputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/PixelProgramsOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/RestartInputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/RuntimeDataInputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/RuntimeDataOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/UserCreateInputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/UsersOutputPortTest.javasrc/test/java/pl/vtt/wpi/core/domain/port/WifiConfigInputPortTest.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/pl/vtt/wpi/core/domain/port/exception/OutputPortException.java
- src/main/java/pl/vtt/wpi/core/infrastructure/Response.java
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/pl/vtt/wpi/core/domain/model/Authorization.java
- src/main/java/module-info.java
- src/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.java
- src/main/java/pl/vtt/wpi/core/domain/model/color/LabColor.java
- src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
Comment on lines
30
to
+33
| public LoginServiceImpl(String url, Supplier<Authorization> authorizationSupplier, | ||
| RequestHandler<Void, Credentials> requestHandler) { | ||
| this.requestFactory = new LoginRequestFactory(url, authorizationSupplier); | ||
| this.requestHandler = requestHandler; | ||
| this.authPort = new AuthOutputPort(new LoginRequestFactory(url, authorizationSupplier), requestHandler); | ||
| } |
There was a problem hiding this comment.
This overload can authenticate with credentials other than the login() arguments.
login() now seeds AuthorizationHolder with the username/password being validated, but this constructor still lets callers provide any Supplier<Authorization>. If that supplier is not AuthorizationHolder::get, authPort.load() can send a different authorization than the current login attempt. LoginRequestFactory.create(...) also hardcodes POST, so it bypasses the method selected by AuthOutputPort.
💡 Possible fix
- public LoginServiceImpl(String url, Supplier<Authorization> authorizationSupplier,
- RequestHandler<Void, Credentials> requestHandler) {
- this.authPort = new AuthOutputPort(new LoginRequestFactory(url, authorizationSupplier), requestHandler);
+ public LoginServiceImpl(String url,
+ RequestHandler<Void, Credentials> requestHandler) {
+ this.authPort = new AuthOutputPort(new LoginRequestFactory(url, AuthorizationHolder::get), requestHandler);
}
@@
- return new Request<>(LocalDateTime.now(), POST, url, supplier.get(), null);
+ return new Request<>(LocalDateTime.now(), method, url, supplier.get(), payload);Also applies to: 74-79
🤖 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/LoginServiceImpl.java`
around lines 30 - 33, The constructor LoginServiceImpl currently accepts an
arbitrary Supplier<Authorization> which allows authPort.load() to send an
authorization different from the credentials seeded by login(); change the
constructor to use AuthorizationHolder::get (or accept an AuthorizationHolder
instance) so the auth supplier is always the AuthorizationHolder used by
login(), and update LoginRequestFactory.create to stop hardcoding POST — make it
respect the HTTP method chosen by AuthOutputPort (or accept the method as a
parameter) so AuthOutputPort and LoginRequestFactory produce requests
consistently; ensure login() seeds AuthorizationHolder before any call to
authPort.load().
Comment on lines
+25
to
+27
| if (obj == null) { | ||
| throw new InputPortException("Pixel programs cannot be null"); | ||
| } |
There was a problem hiding this comment.
Guard against null elements inside the programs list.
obj is checked, but List<PixelProgram> may still contain null entries and fail later during request handling/serialization.
Suggested fix
if (obj == null) {
throw new InputPortException("Pixel programs cannot be null");
}
+ if (obj.contains(null)) {
+ throw new InputPortException("Pixel programs cannot contain null elements");
+ }📝 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
| if (obj == null) { | |
| throw new InputPortException("Pixel programs cannot be null"); | |
| } | |
| if (obj == null) { | |
| throw new InputPortException("Pixel programs cannot be null"); | |
| } | |
| if (obj.contains(null)) { | |
| throw new InputPortException("Pixel programs cannot contain null elements"); | |
| } |
🤖 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/PixelProgramsInputPort.java`
around lines 25 - 27, The current null-check only validates obj but not its
contents; update the PixelProgramsInputPort logic that handles the
List<PixelProgram> (the obj variable) to iterate the list and throw
InputPortException if any element is null (or use Objects.requireNonNull on each
entry) so null entries are rejected early; reference the
List<PixelProgram>/PixelProgram elements and the existing InputPortException for
consistency.
Comment on lines
+26
to
+27
| } catch (Exception e) { | ||
| throw new InputPortException("Cannot restart device", e); |
There was a problem hiding this comment.
Preserve interruption semantics in the catch block.
This wraps all Exceptions; if interruption is the cause, the current thread’s interrupt flag should be restored before rethrowing.
Proposed fix
- } catch (Exception e) {
+ } catch (Exception e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
throw new InputPortException("Cannot restart device", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (Exception e) { | |
| throw new InputPortException("Cannot restart device", e); | |
| } catch (Exception e) { | |
| if (e instanceof InterruptedException) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| throw new InputPortException("Cannot restart device", 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/RestartInputPort.java` around
lines 26 - 27, The catch block in RestartInputPort currently wraps all
Exceptions into an InputPortException without preserving interruption semantics;
update the error handling so that InterruptedException is handled
specially—either add a separate catch (InterruptedException ie) that calls
Thread.currentThread().interrupt() and then throws new
InputPortException("Cannot restart device", ie), or inside the existing catch
check if (e instanceof InterruptedException) Thread.currentThread().interrupt();
before rethrowing new InputPortException("Cannot restart device", e); ensure
InputPortException is still thrown but the thread's interrupted status is
restored.
Comment on lines
+29
to
+35
| if (obj.nol() == null || obj.brightness() == null || obj.on() == null | ||
| || obj.overflow() == null || obj.sensorDependency() == null | ||
| || obj.timeDependency() == null || obj.defaultRestartCountdown() == null | ||
| || obj.offTime() == null || obj.onTime() == null | ||
| || obj.zoneId() == null || obj.pixelProgram() == null | ||
| || obj.stepTime() == null) { | ||
| method = Method.PATCH; |
There was a problem hiding this comment.
Don't treat dependency-gated nulls as proof of a partial update.
src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java:7-19 makes all components nullable, so a payload with timeDependency=false can still be a complete runtime state while onTime/offTime/zoneId are intentionally null. This branch would still downgrade it to PATCH, which can leave stale schedule values on the device instead of replacing them.
💡 Possible fix
- Method method = Method.PUT;
- if (obj.nol() == null || obj.brightness() == null || obj.on() == null
- || obj.overflow() == null || obj.sensorDependency() == null
- || obj.timeDependency() == null || obj.defaultRestartCountdown() == null
- || obj.offTime() == null || obj.onTime() == null
- || obj.zoneId() == null || obj.pixelProgram() == null
- || obj.stepTime() == null) {
- method = Method.PATCH;
- }
+ boolean hasCoreFields =
+ obj.nol() != null
+ && obj.brightness() != null
+ && obj.on() != null
+ && obj.overflow() != null
+ && obj.sensorDependency() != null
+ && obj.timeDependency() != null
+ && obj.defaultRestartCountdown() != null
+ && obj.pixelProgram() != null
+ && obj.stepTime() != null;
+ boolean hasTimeFields =
+ Boolean.FALSE.equals(obj.timeDependency())
+ || (obj.onTime() != null && obj.offTime() != null && obj.zoneId() != null);
+ Method method = hasCoreFields && hasTimeFields ? Method.PUT : Method.PATCH;📝 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
| if (obj.nol() == null || obj.brightness() == null || obj.on() == null | |
| || obj.overflow() == null || obj.sensorDependency() == null | |
| || obj.timeDependency() == null || obj.defaultRestartCountdown() == null | |
| || obj.offTime() == null || obj.onTime() == null | |
| || obj.zoneId() == null || obj.pixelProgram() == null | |
| || obj.stepTime() == null) { | |
| method = Method.PATCH; | |
| boolean hasCoreFields = | |
| obj.nol() != null | |
| && obj.brightness() != null | |
| && obj.on() != null | |
| && obj.overflow() != null | |
| && obj.sensorDependency() != null | |
| && obj.timeDependency() != null | |
| && obj.defaultRestartCountdown() != null | |
| && obj.pixelProgram() != null | |
| && obj.stepTime() != null; | |
| boolean hasTimeFields = | |
| Boolean.FALSE.equals(obj.timeDependency()) | |
| || (obj.onTime() != null && obj.offTime() != null && obj.zoneId() != null); | |
| Method method = hasCoreFields && hasTimeFields ? Method.PUT : Method.PATCH; |
🤖 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/RuntimeDataInputPort.java`
around lines 29 - 35, The current check in RuntimeDataInputPort that sets method
= Method.PATCH treats any null in fields like onTime/offTime/zoneId as a partial
update even when timeDependency or sensorDependency explicitly allow those to be
null; update the conditional so that nulls for time-related fields are only
considered "partial" when Boolean.TRUE.equals(obj.timeDependency()) and likewise
only consider sensor-related fields partial when
Boolean.TRUE.equals(obj.sensorDependency()); leave other independent fields in
the original null-check. Locate the null-check block referencing obj.nol(),
obj.brightness(), obj.on(), obj.overflow(), obj.sensorDependency(),
obj.timeDependency(), obj.defaultRestartCountdown(), obj.offTime(),
obj.onTime(), obj.zoneId(), obj.pixelProgram(), obj.stepTime() and adjust the
logic to gate required presence of onTime/offTime/zoneId (and any
sensor-dependent fields) behind the corresponding dependency booleans before
setting Method.PATCH.
Comment on lines
+28
to
+30
| if (obj.ssid() == null || obj.password() == null) { | ||
| throw new InputPortException("SSID and password cannot be null"); | ||
| } |
There was a problem hiding this comment.
Reject blank SSID/password values before dispatch.
Current validation only rejects null, so empty/blank values still pass through and may produce invalid Wi-Fi updates.
Suggested fix
- if (obj.ssid() == null || obj.password() == null) {
- throw new InputPortException("SSID and password cannot be null");
+ if (obj.ssid() == null || obj.ssid().isBlank()
+ || obj.password() == null || obj.password().isBlank()) {
+ throw new InputPortException("SSID and password cannot be null or blank");
}🤖 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/WifiConfigInputPort.java`
around lines 28 - 30, The current null-only check in the WifiConfigInputPort
validation allows empty or whitespace-only SSID/password; update the validation
that checks obj.ssid() and obj.password() to reject blank values as well (e.g.,
use String.isBlank() or trim().isEmpty()) and throw the existing
InputPortException with the same message; modify the guard around obj.ssid() ==
null || obj.password() == null in the method that performs validation
(references to obj.ssid() and obj.password()) so it also checks for blank/empty
strings before dispatching the update.
Comment on lines
+25
to
+27
| } catch (Exception e) { | ||
| throw new OutputPortException("Cannot load device info", e); | ||
| } |
There was a problem hiding this comment.
Preserve thread interruption when wrapping exceptions.
Catching Exception here can absorb InterruptedException without restoring interrupt status, which can break cooperative cancellation.
Proposed fix
- } catch (Exception e) {
+ } catch (Exception e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
throw new OutputPortException("Cannot load device info", 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/output/DeviceInfoOutputPort.java`
around lines 25 - 27, The catch-all currently swallows InterruptedException;
change the exception handling in the DeviceInfoOutputPort code so
InterruptedException is caught separately, restore the interrupt status with
Thread.currentThread().interrupt(), and then wrap/throw it as an
OutputPortException (or rethrow) rather than letting it be absorbed — keep the
existing catch(Exception e) for other errors; reference OutputPortException and
the catch block in DeviceInfoOutputPort to locate the change.
Comment on lines
+26
to
+27
| } catch (Exception e) { | ||
| throw new OutputPortException("Cannot load users", e); |
There was a problem hiding this comment.
Avoid losing interrupt status when wrapping exceptions.
If an interrupt reaches this catch block, it should be re-signaled before wrapping to avoid cancellation bugs.
Proposed fix
- } catch (Exception e) {
+ } catch (Exception e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
throw new OutputPortException("Cannot load users", 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/output/UsersOutputPort.java` around
lines 26 - 27, The catch in UsersOutputPort that currently wraps all Exceptions
into OutputPortException is losing the interrupt status; update the catch logic
so that if the caught exception is an InterruptedException (or has an
InterruptedException cause) you call Thread.currentThread().interrupt() before
throwing the new OutputPortException, then throw the OutputPortException as
before; locate the catch block that throws new OutputPortException("Cannot load
users", e) and add the interrupt re-signal handling there.
Comment on lines
+25
to
+26
| RuntimeData data = new RuntimeData(1, 0, 1.0, 100, 5, false, true, | ||
| true, true, LocalTime.NOON, LocalTime.MIDNIGHT, ZoneId.of("UTC")); |
There was a problem hiding this comment.
Fix RuntimeData construction — current arguments don’t match the record signature.
This test data initializer appears incompatible with src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java:1-25 (wrong arity and several wrong types), which will break compilation.
🔧 Proposed fix
- RuntimeData data = new RuntimeData(1, 0, 1.0, 100, 5, false, true,
- true, true, LocalTime.NOON, LocalTime.MIDNIGHT, ZoneId.of("UTC"));
+ RuntimeData data = new RuntimeData("device-1", 1, 0, 100, true, 5, 80,
+ 1, 0, false, true, 30, ZoneId.of("UTC"));If you apply this, LocalTime imports are no longer needed in this test.
📝 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
| RuntimeData data = new RuntimeData(1, 0, 1.0, 100, 5, false, true, | |
| true, true, LocalTime.NOON, LocalTime.MIDNIGHT, ZoneId.of("UTC")); | |
| RuntimeData data = new RuntimeData("device-1", 1, 0, 100, true, 5, 80, | |
| 1, 0, false, true, 30, ZoneId.of("UTC")); |
🤖 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/RuntimeDataInputPortTest.java`
around lines 25 - 26, The RuntimeData construction in RuntimeDataInputPortTest
uses the wrong arity and types; update the RuntimeData(...) call to pass
arguments in the exact order and types defined by the RuntimeData record (use
the main RuntimeData record's field list/signatures from
src/main/java/pl/vtt/wpi/core/domain/model/device/RuntimeData.java) so the
constructor call matches the record signature, and remove the now-unused
LocalTime imports from the test. Ensure you reference the RuntimeData record's
field names/types when reordering or changing values so compilation succeeds.
Comment on lines
+30
to
+35
| @Test | ||
| void send_throwsWhenSsidOrPasswordMissing() { | ||
| WifiConfigInputPort port = new WifiConfigInputPort((method, target, payload) -> null, request -> {}); | ||
|
|
||
| assertThrows(InputPortException.class, () -> port.send(new WifiConfig("ssid", null))); | ||
| } |
There was a problem hiding this comment.
Test name overstates coverage (missing SSID case not tested).
send_throwsWhenSsidOrPasswordMissing currently verifies only password == null. Add the missing SSID assertion (or rename the test).
Proposed fix
void send_throwsWhenSsidOrPasswordMissing() {
WifiConfigInputPort port = new WifiConfigInputPort((method, target, payload) -> null, request -> {});
+ assertThrows(InputPortException.class, () -> port.send(new WifiConfig(null, "pass")));
assertThrows(InputPortException.class, () -> port.send(new WifiConfig("ssid", null)));
}🤖 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/WifiConfigInputPortTest.java`
around lines 30 - 35, Test currently only checks password == null; update the
test to also assert the SSID-missing case by calling WifiConfigInputPort.send
with a WifiConfig that has null SSID (e.g., new WifiConfig(null, "password"))
and expecting InputPortException, or rename the test to reflect it only covers
the password-null case; reference WifiConfigInputPort.send, WifiConfig, and
InputPortException when adding the additional assertion.
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.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests