Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll acknowledge your comments with a 👀 emoji and then get to work. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! I will automatically address your feedback. For any comments you don't want me to act on, just include (aside). For security, I will only act on instructions from the user who triggered this task for this pull request. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughRefactors OCPP task handling to a mapper-driven, version-aware framework: introduces TaskDefinition, RequestMapper, ResponseMapper, and OcppVersionHandler; updates CommunicationTask and RequestResult generics; migrates many tasks to the new model; removes legacy per-version handlers/subclasses; adapts SOAP/JSON invokers and service wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Invoker as ChargePointServiceInvokerImpl
participant Transport as ChargePointServiceSoapInvoker
participant Task as CommunicationTask
participant TD as TaskDefinition
participant VH as OcppVersionHandler
participant CP as ChargePoint
Caller->>Invoker: request operation (task)
Invoker->>Transport: invoke(cp, task, soapAction, jsonAction)
Transport->>Task: getRequest(ocppVersion)
Task->>TD: versionHandlers.get(ocppVersion)
TD->>VH: resolve handler
VH->>Task: RequestMapper.map(task) -> RequestType
Transport->>CP: send version-specific request
CP-->>Transport: response
Transport->>Task: createHandler(chargeBoxId)
VH->>Task: ResponseMapper used inside handler -> typed RESPONSE
Task->>Task: addNewResponse(chargeBoxId, RESPONSE)
Task-->>Invoker: results available
Invoker-->>Caller: return aggregated result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@google-labs-jules build is failing |
Great! I'm ready to address any feedback that comes from the review. |
|
Skipped: Cannot respond to another bot. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetConfigurationTask.java (1)
45-49: Fix NPE when unknownKeys is null
JOINER.join(unknownKeys)will NPE if response has no unknown keys. Guard it.+import java.util.Collections; @@ - private ResponseWrapper(List<KeyValue> configurationKeys, List<String> unknownKeys) { + private ResponseWrapper(List<KeyValue> configurationKeys, List<String> unknownKeys) { this.configurationKeys = configurationKeys; - this.unknownKeys = JOINER.join(unknownKeys); + this.unknownKeys = JOINER.join(unknownKeys == null ? Collections.emptyList() : unknownKeys); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
133-141: Null-guard addNewError to avoid NPE for unknown chargeBoxIdaddNewResponse guards unknown IDs; addNewError should mirror that.
- public void addNewError(String chargeBoxId, @Nullable String errorMessage) { - resultMap.get(chargeBoxId).setErrorMessage(errorMessage); + public void addNewError(String chargeBoxId, @Nullable String errorMessage) { + var result = resultMap.get(chargeBoxId); + if (result == null) { + log.warn("Received error for unknown chargeBoxId '{}'", chargeBoxId); + return; + } + result.setErrorMessage(errorMessage);
🧹 Nitpick comments (43)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/RequestResult.java (1)
31-34: Prevent ambiguous states: ensure only one of response/error is setRight now both fields can be non-null, which is ambiguous. Consider adding small helpers and guiding callers to use them, or enforce via factories.
Example additions (outside the shown lines):
public boolean isSuccess() { return errorMessage == null; } public static <U> RequestResult<U> success(@Nullable U response) { var r = new RequestResult<U>(); r.setResponse(response); return r; } public static <U> RequestResult<U> error(String message) { var r = new RequestResult<U>(); r.setErrorMessage(message); return r; }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/RequestMapper.java (2)
24-27: Align exception policy with ResponseMapper to avoid asymmetry.Either let
RequestMapper#mapdeclare a checked exception too, or makeResponseMapper#mapnon-throwing (preferred) and surface mapping errors as unchecked (e.g.,MappingException). This prevents awkward lambda usage inconsistencies.
19-27: Consider moving SPI out of.implpackage.Since this is part of the public contract used by tasks/handlers, place it under
...ocpp.task.api(or similar) to signal “stable API”, not implementation detail.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/ResponseMapper.java (1)
23-26: Prefer non-throwing mapper; use unchecked exceptions for mapping failures.Checked exceptions on functional interfaces make method refs and composition clumsy. Suggest removing
throws Exceptionand, where needed, throwing an uncheckedMappingExceptioninstead.Apply within this hunk:
@FunctionalInterface public interface ResponseMapper<T extends ResponseType, U> { - U map(T response) throws Exception; + U map(T response); }steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java (2)
61-65: Nice generic router; allow contravariance for broader method-ref compatibility.Widen the second type of
BiConsumerto? super Tto accept supertypes without additional casts.- private <T extends CommunicationTask<?, ?>> void invoke( + private <T extends CommunicationTask<?, ?>> void invoke( ChargePointSelect cp, T task, - BiConsumer<ChargePointSelect, T> soapAction, - BiConsumer<ChargePointSelect, T> jsonAction) { + BiConsumer<ChargePointSelect, ? super T> soapAction, + BiConsumer<ChargePointSelect, ? super T> jsonAction) {
102-108: Add missing @OverRide.All other interface impls are annotated; add for consistency and compiler checks.
- public void changeAvailability(ChargePointSelect cp, ChangeAvailabilityTask task) { + @Override + public void changeAvailability(ChargePointSelect cp, ChangeAvailabilityTask task) {steve-ocpp-transport-soap/src/main/java/de/rwth/idsg/steve/ocpp/soap/ChargePointServiceSoapInvoker.java (2)
295-311: Don’t reuse a static Exception instance; create a fresh, descriptive one per call.Reusing
EXCEPTION_V12/EXCEPTION_V15across threads loses stack context and reduces diagnostics.- case V_12 -> task.failed(cp.getChargeBoxId(), EXCEPTION_V12); + case V_12 -> task.failed(cp.getChargeBoxId(), unsupported(ocppVersion, "DataTransfer"));Add a helper (outside this hunk) and apply similarly to other unsupported cases:
private static IllegalArgumentException unsupported(OcppVersion v, String op) { return new IllegalArgumentException("Operation " + op + " is not supported by OCPP " + v + " station"); }
69-92: Guard against mismatched request type per version (debug-only).Because generic typing can’t enforce the concrete SOAP request class, consider a lightweight assert/log to catch wrong mapper wiring early (e.g., verify package prefix vs. version) before invoking
*Async. Optional, but helps during refactors.steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (3)
366-379: Extract hard-coded caller to a constant.Avoid magic string; centralize for reuse/testing.
public class ChargePointServiceClient { + private static final String CALLER_STEVE = "SteVe"; @@ - CancelReservationTask task = new CancelReservationTask(params, reservationRepository, "SteVe"); + CancelReservationTask task = new CancelReservationTask(params, reservationRepository, CALLER_STEVE);
414-421: Use the same constant for setChargingProfile.- SetChargingProfileTask task = new SetChargingProfileTask(params, details, chargingProfileRepository, "SteVe"); + SetChargingProfileTask task = new SetChargingProfileTask( + params, details, chargingProfileRepository, CALLER_STEVE);
366-421: API consistency: consider overloads that acceptcaller.You've added
callervariants for RemoteStart/Stop/Unlock; mirroring that for CancelReservation and SetChargingProfile would keep the client API uniform.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/TaskDefinition.java (1)
30-31: Provide a helper to fetch a handler with a clear error if missing.Improves diagnostics when a version isn’t configured.
// Add inside class (no change to the builder API) public OcppVersionHandler<S, U> handlerFor(OcppVersion v) { OcppVersionHandler<S, U> h = versionHandlers.get(v); if (h == null) { throw new IllegalArgumentException("No OCPP handler configured for version: " + v); } return h; }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java (2)
32-56: Reduce duplication and add lightweight param validation in request mappers.
- The three lambdas are identical except package/type names. Consider a small static helper to validate connectorId and build the request; reduces repetition and guards bad input early.
- OcppVersion.V_12, - new OcppVersionHandler<>( - task -> new ocpp.cp._2010._08.UnlockConnectorRequest() - .withConnectorId( - task.getParams().getConnectorId()), + OcppVersion.V_12, + new OcppVersionHandler<>( + task -> new ocpp.cp._2010._08.UnlockConnectorRequest() + .withConnectorId(requirePositiveConnectorId(task.getParams())), (ocpp.cp._2010._08.UnlockConnectorResponse r) -> r.getStatus().value()), @@ - OcppVersion.V_15, + OcppVersion.V_15, new OcppVersionHandler<>( - task -> new ocpp.cp._2012._06.UnlockConnectorRequest() - .withConnectorId( - task.getParams().getConnectorId()), + task -> new ocpp.cp._2012._06.UnlockConnectorRequest() + .withConnectorId(requirePositiveConnectorId(task.getParams())), (ocpp.cp._2012._06.UnlockConnectorResponse r) -> r.getStatus().value()), @@ - OcppVersion.V_16, + OcppVersion.V_16, new OcppVersionHandler<>( - task -> new ocpp.cp._2015._10.UnlockConnectorRequest() - .withConnectorId( - task.getParams().getConnectorId()), + task -> new ocpp.cp._2015._10.UnlockConnectorRequest() + .withConnectorId(requirePositiveConnectorId(task.getParams())), (ocpp.cp._2015._10.UnlockConnectorResponse r) -> r.getStatus().value())))Add (outside the shown hunk):
private static int requirePositiveConnectorId(UnlockConnectorParams p) { int id = p.getConnectorId(); if (id <= 0) throw new IllegalArgumentException("connectorId must be > 0"); return id; }If upstream DTO validation already enforces this, feel free to skip and confirm.
32-56: Consider returning typed enums instead of String for status.Using the OCPP enum (e.g., UnlockStatus) avoids fragile string matching downstream.
Would you like a follow-up patch converting the generic type from
Stringto the concrete status enums for this and similar tasks?steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java (2)
32-56: Validate transactionId and consider de-duplicating mapper boilerplate.
- Add a simple positive check for
transactionId.- The three mappers are identical; a helper would reduce repetition.
- task -> new ocpp.cp._2010._08.RemoteStopTransactionRequest() - .withTransactionId( - task.getParams().getTransactionId()), + task -> new ocpp.cp._2010._08.RemoteStopTransactionRequest() + .withTransactionId(requirePositiveTxId(task.getParams())), @@ - task -> new ocpp.cp._2012._06.RemoteStopTransactionRequest() - .withTransactionId( - task.getParams().getTransactionId()), + task -> new ocpp.cp._2012._06.RemoteStopTransactionRequest() + .withTransactionId(requirePositiveTxId(task.getParams())), @@ - task -> new ocpp.cp._2015._10.RemoteStopTransactionRequest() - .withTransactionId( - task.getParams().getTransactionId()), + task -> new ocpp.cp._2015._10.RemoteStopTransactionRequest() + .withTransactionId(requirePositiveTxId(task.getParams())),Add (outside the shown hunk):
private static int requirePositiveTxId(RemoteStopTransactionParams p) { int id = p.getTransactionId(); if (id <= 0) throw new IllegalArgumentException("transactionId must be > 0"); return id; }
32-56: Return typed enums instead of String for status to avoid fragile comparisons.If agreeable, I can draft a follow-up converting the generic type to the status enum and adapting downstream uses.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetCompositeScheduleTask.java (2)
35-48: Validate params before building the request.Guard common mistakes early (duration <= 0, connectorId <= 0); keeps failures deterministic before transport.
- new OcppVersionHandler<>( - task -> { - GetCompositeScheduleParams p = task.getParams(); - return new GetCompositeScheduleRequest() - .withConnectorId(p.getConnectorId()) - .withDuration(p.getDurationInSeconds()) - .withChargingRateUnit(p.getChargingRateUnit()); - }, + new OcppVersionHandler<>( + task -> { + GetCompositeScheduleParams p = task.getParams(); + if (p.getConnectorId() <= 0) { + throw new IllegalArgumentException("connectorId must be > 0"); + } + if (p.getDurationInSeconds() <= 0) { + throw new IllegalArgumentException("durationInSeconds must be > 0"); + } + return new GetCompositeScheduleRequest() + .withConnectorId(p.getConnectorId()) + .withDuration(p.getDurationInSeconds()) + .withChargingRateUnit(p.getChargingRateUnit()); + },
35-49: Consider centralizing “status/response passthrough” mappers.Multiple tasks use identical patterns; a small factory (e.g., Mappers.statusValue(), Mappers.identity()) would cut boilerplate and ensure consistency.
I can draft a minimal utility in
task.implif desired.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java (3)
71-74: Log the unexpected enum value for easier diagnosticsInclude the actual
filterTypein the log entry.Apply:
- log.warn("Unexpected {} enum value", ClearChargingProfileFilterType.class.getSimpleName()); + log.warn("Unexpected {} value: {}", + ClearChargingProfileFilterType.class.getSimpleName(), + getParams().getFilterType());
86-93: Harden error-to-string handlingAvoid possible nulls; use safe conversion for both
errorand exception message.- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }
110-118: Apply same safe error conversion in defaultCallbackMirror the above null-safe handling.
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java (1)
92-99: Null-safe error propagation in defaultCallbackUse safe conversions.
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetLocalListVersionTask.java (1)
64-71: Null-safe error propagation in defaultCallbackSame pattern as other tasks.
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java (1)
78-85: Null-safe error propagation in defaultCallbackApply the same safe conversions.
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ReserveNowTask.java (2)
91-101: Null-safe error propagation (constructor callback and defaultCallback)Use safe conversions for
errorand exception messages.- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e.getMessage() != null ? e.getMessage() : e.toString()); }Also applies to: 118-125
38-73: Minor duplication between 1.5 and 1.6 mappersConsider extracting a small helper to build common fields; pass only the version-specific request type constructor. Keeps the lambdas tighter.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/OcppVersionHandler.java (2)
31-32: Fail fast on misconfiguration: mark mappers as non-null.Constructor-level null-checks avoid late NPEs when resolving handlers.
Apply this diff:
- private final RequestMapper<? extends CommunicationTask<S, U>, ? extends RequestType> requestMapper; - private final ResponseMapper<? extends ResponseType, U> responseMapper; + private final @NonNull RequestMapper<? extends CommunicationTask<S, U>, ? extends RequestType> requestMapper; + private final @NonNull ResponseMapper<? extends ResponseType, U> responseMapper;Add this import near other Lombok imports:
import lombok.NonNull;
30-33: Optional: make this a Java record to remove Lombok and express intent.A record cleanly models this as an immutable tuple and drops Lombok.
Example:
public record OcppVersionHandler<S extends ChargePointSelection, U>( RequestMapper<? extends CommunicationTask<S, U>, ? extends RequestType> requestMapper, ResponseMapper<? extends ResponseType, U> responseMapper) {}steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java (1)
66-83: Default callback is duplicated across tasks—centralize in base.The same success/successError/failed logic appears in multiple tasks; consider providing a protected default in CommunicationTask to DRY this.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java (2)
39-44: Guard against null triggerMessage to avoid NPE during mapping.Add a null check before calling value()/fromValue.
Apply this diff:
- task -> new ocpp.cp._2015._10.TriggerMessageRequest() - .withConnectorId(task.getParams().getConnectorId()) - .withRequestedMessage(MessageTrigger.fromValue(task.getParams() - .getTriggerMessage() - .value())), + task -> { + var p = task.getParams(); + Objects.requireNonNull(p.getTriggerMessage(), "triggerMessage must not be null"); + return new ocpp.cp._2015._10.TriggerMessageRequest() + .withConnectorId(p.getConnectorId()) + .withRequestedMessage( + MessageTrigger.fromValue(p.getTriggerMessage().value())); + },Add import:
import java.util.Objects;
56-73: Same callback duplication note as other tasks.Consider centralizing the default callback in the base class.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java (2)
37-58: Validate idTag and connectorId before sending requests.OCPP requires non-empty idTag; connectorId, when present, must be > 0. Add lightweight validation to fail fast.
Apply this diff:
- task -> new ocpp.cp._2010._08.RemoteStartTransactionRequest() - .withIdTag(task.getParams().getIdTag()) - .withConnectorId( - task.getParams().getConnectorId()), + task -> new ocpp.cp._2010._08.RemoteStartTransactionRequest() + .withIdTag(requireNonBlank(task.getParams().getIdTag(), "idTag")) + .withConnectorId( + validateConnectorId(task.getParams().getConnectorId())), @@ - task -> new ocpp.cp._2012._06.RemoteStartTransactionRequest() - .withIdTag(task.getParams().getIdTag()) - .withConnectorId( - task.getParams().getConnectorId()), + task -> new ocpp.cp._2012._06.RemoteStartTransactionRequest() + .withIdTag(requireNonBlank(task.getParams().getIdTag(), "idTag")) + .withConnectorId( + validateConnectorId(task.getParams().getConnectorId())), @@ - task -> new ocpp.cp._2015._10.RemoteStartTransactionRequest() - .withIdTag(task.getParams().getIdTag()) - .withConnectorId( - task.getParams().getConnectorId()), + task -> new ocpp.cp._2015._10.RemoteStartTransactionRequest() + .withIdTag(requireNonBlank(task.getParams().getIdTag(), "idTag")) + .withConnectorId( + validateConnectorId(task.getParams().getConnectorId())),Add these helpers in the class:
private static Integer validateConnectorId(Integer cid) { if (cid == null) return null; if (cid <= 0) throw new IllegalArgumentException("connectorId must be > 0"); return cid; } private static String requireNonBlank(String s, String name) { if (s == null || s.isBlank()) throw new IllegalArgumentException(name + " must not be blank"); return s; }
70-87: Callback duplication.Same DRY suggestion as in other tasks: move the default callback to the base class.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java (1)
90-107: Callback duplication note.Same DRY suggestion: centralize default callback in CommunicationTask.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java (2)
41-69: Avoid brittle cast inside mapper
(SendLocalListTask) taskworks but couples the mapper to the concrete class. Consider shaping the framework so request mappers receive the concrete task type (or captureocppTagsServicevia instance-aware mapper) to avoid accidentalClassCastException.
100-121: Minor: drop param increateOcpp16RequestThe method uses the field; the param is redundant.
- private ocpp.cp._2015._10.SendLocalListRequest createOcpp16Request(OcppTagsService ocppTagsService) { + private ocpp.cp._2015._10.SendLocalListRequest createOcpp16Request() { @@ - auths.addAll(ocppTagsService.getAuthData(getParams().getAddUpdateList())); + auths.addAll(this.ocppTagsService.getAuthData(getParams().getAddUpdateList())); @@ - values = ocppTagsService.getAuthDataOfAllTags(); + values = this.ocppTagsService.getAuthDataOfAllTags();And in mappers:
- ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = t.createOcpp16Request(t.ocppTagsService); + ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = t.createOcpp16Request();steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java (1)
40-47: Prevent log/monitoring bloat in toString()Large
datapayloads can overwhelm logs. Truncate defensively.- if (data != null) { - builder.append(" / Data: ").append(data); + if (data != null) { + var s = data.length() > 512 ? (data.substring(0, 512) + "...") : data; + builder.append(" / Data: ").append(s); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetConfigurationTask.java (2)
64-72: Only set keys when provided (preserve “all keys” semantics)When
getAllKeys()is null/empty, omit.withKey(...)to request all keys per spec.- task -> new ocpp.cp._2012._06.GetConfigurationRequest() - .withKey(task.getParams().getAllKeys()), + task -> { + var req = new ocpp.cp._2012._06.GetConfigurationRequest(); + var keys = task.getParams().getAllKeys(); + if (keys != null && !keys.isEmpty()) { + req.withKey(keys); + } + return req; + },
75-83: Mirror the same keys handling for 1.6Apply the same guard to V_16 mapping.
- task -> new ocpp.cp._2015._10.GetConfigurationRequest() - .withKey(task.getParams().getAllKeys()), + task -> { + var req = new ocpp.cp._2015._10.GetConfigurationRequest(); + var keys = task.getParams().getAllKeys(); + if (keys != null && !keys.isEmpty()) { + req.withKey(keys); + } + return req; + },steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java (3)
85-88: Use the actual sent connectorId for DB updateTie the DB assignment to the connectorId in the request payload, not the original params.
- repo.setProfile(details.getChargingProfilePk(), chargeBoxId, params.getConnectorId()); + repo.setProfile(details.getChargingProfilePk(), chargeBoxId, request.getConnectorId());
125-133: Minor: improve readability of lambda variableRename k to periodDto for clarity.
- var schedulePeriods = details.getPeriods().stream() - .map(k -> { + var schedulePeriods = details.getPeriods().stream() + .map(periodDto -> { ChargingSchedulePeriod p = new ChargingSchedulePeriod(); - p.setStartPeriod(k.getStartPeriodInSeconds()); - p.setLimit(k.getPowerLimit()); - p.setNumberPhases(k.getNumberPhases()); + p.setStartPeriod(periodDto.getStartPeriodInSeconds()); + p.setLimit(periodDto.getPowerLimit()); + p.setNumberPhases(periodDto.getNumberPhases()); return p; }) .toList();
45-46: Hide version-specific request: make request getter privateVerified: no external call sites invoke a no-arg getRequest(); external callers use CommunicationTask.getRequest(OcppVersion). Change steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java to suppress the Lombok-generated public getter for the request field and provide a private accessor used internally by TASK_DEFINITION.
Suggested change (minimal):
@Getter
public class SetChargingProfileTask extends CommunicationTask<...> {
@Getter(lombok.AccessLevel.NONE) // prevent public getter for the request field
private SetChargingProfileRequest request;
...
private SetChargingProfileRequest getRequest() { return request; }
}steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
153-161: Avoid potential ConcurrentModificationException when iterating callbacksSnapshot the callback list before iterating. Low risk, but safer if callbacks are added concurrently.
- public void success(String chargeBoxId, RESPONSE response) { - for (OcppCallback<RESPONSE> c : callbackList) { + public void success(String chargeBoxId, RESPONSE response) { + var callbacks = new ArrayList<>(callbackList); + for (OcppCallback<RESPONSE> c : callbacks) { try { c.success(chargeBoxId, response); } catch (Exception e) { log.error("Exception occurred in OcppCallback", e); } } } - public void failed(String chargeBoxId, Exception exception) { - for (OcppCallback<RESPONSE> c : callbackList) { + public void failed(String chargeBoxId, Exception exception) { + var callbacks = new ArrayList<>(callbackList); + for (OcppCallback<RESPONSE> c : callbacks) { try { c.failed(chargeBoxId, exception); } catch (Exception e) { log.error("Exception occurred in OcppCallback", e); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java(5 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/RequestResult.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetCompositeScheduleTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetConfigurationTask.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetLocalListVersionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ReserveNowTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskAdhoc.java(0 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java(0 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/OcppVersionHandler.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/RequestMapper.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/ResponseMapper.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/TaskDefinition.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java(2 hunks)steve-ocpp-transport-soap/src/main/java/de/rwth/idsg/steve/ocpp/soap/ChargePointServiceSoapInvoker.java(2 hunks)steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java(2 hunks)
💤 Files with no reviewable changes (2)
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskAdhoc.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-05T13:09:02.595Z
Learnt from: juherr
PR: juherr/evolve#1
File: src/main/resources/OCPP-2.0.1_part3_JSON_schemas/ClearCacheRequest.json:21-27
Timestamp: 2025-04-05T13:09:02.595Z
Learning: In the OCPP (Open Charge Point Protocol) standard, the ClearCacheRequest schema is correctly designed with only the optional customData property, as this operation doesn't require additional parameters.
Applied to files:
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java
🔇 Additional comments (25)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/RequestResult.java (3)
32-33: Type-safe response field — good refactorSwitching
responseto@Nullable Taligns with the mapper-based design and removes casting at call sites.
21-24: Nullness annotations consistency — resolved: repository consistently uses org.jspecify.annotations.NullableSearch for imports matching
import\s+.*\.Nullable;returned onlyorg.jspecify.annotations.Nullableacross the codebase; nojavax,org.jetbrains,androidx, or other@Nullableimports found.
32-32: Fix raw RequestResult usages — no getResponse() call sites found
- Repo search found no usages of getResponse(); the original claim that callers do
String s = result.getResponse()is incorrect.- Raw RequestResult declarations to update:
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskInfo.java:23
- steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java:23
Change those to RequestResult<...> with the appropriate response type (e.g., RequestResult if that’s intended).Likely an incorrect or invalid review comment.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/RequestMapper.java (1)
24-27: Solid, minimal SPI. LGTM.Simple, focused functional interface enabling clean mapper injection.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/ResponseMapper.java (1)
19-26: Otherwise looks good.Interface is clear and complements
RequestMapper.steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java (1)
73-209: Router simplification LGTM.Replacing per-method
cp.isSoap()branches with a single helper reduces duplication without changing behavior.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java (1)
82-100: Don't remove — defaultCallback() is required by CommunicationTask.CommunicationTask declares defaultCallback() as abstract and calls it in its constructor (callbackList.add(defaultCallback()); — steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java:104) and the abstract method is declared at :163; CommunicationContext also calls task.defaultCallback().successError(...) (steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java:79). Subclasses must implement this override even if they add additional callbacks in their constructors.
Likely an incorrect or invalid review comment.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java (2)
62-75: DB cleanup happens regardless of response; confirm intended parityYou clear local profiles for both Accepted and non-Accepted responses. This matches the log message, but please confirm it matches prior behavior and operational intent (risk: CP may retain profiles if the request was rejected).
43-49: Incorrect — chargingProfilePk is intentionally used as the OCPP ChargingProfileIdSetChargingProfileTask builds the OCPP ChargingProfile with details.getChargingProfilePk() (SetChargingProfileTask.java:143) and stores/uses that PK via repo.setProfile(...); ClearChargingProfileTask sends params.getChargingProfilePk() in ClearChargingProfileRequest (ClearChargingProfileTask.java:44) and calls chargingProfileRepository.clearProfile(...). The DB PK is used consistently as the OCPP id — no mapper change required.
Likely an incorrect or invalid review comment.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java (1)
34-73: Version-mapped request/response wiring looks solidGood use of TaskDefinition and per-version mappers; response normalization to "OK" is consistent.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetLocalListVersionTask.java (1)
32-45: Straightforward mapping; aligns with new architecturePer-version request and integer-to-string mapping LGTM.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java (2)
32-59: Per-version Reset mapping looks correctUse of
fromValue(params.getResetType().value())across versions is consistent.
37-43: Validate ResetType string mapping across OCPP versionsResetParams.resetType.value() -> "Hard"/"Soft" (steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ResetType.java). The generated ocpp.cp.* ResetType enum sources are not in this repository — verify that ocpp.cp._2010._08.ResetType, ocpp.cp._2012._06.ResetType and ocpp.cp._2015._10.ResetType accept the exact literals "Hard"/"Soft"; if any differ, add an explicit mapping in steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java (current block around lines 35–58). Also applies to the analogous call sites (lines 45–51 and 53–59).
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ReserveNowTask.java (2)
35-74: Version-mapped ReserveNow construction looks goodRequest fields and status extraction for 1.5/1.6 are correct.
84-101: Repository side‑effects are correct for non‑Accepted / failed pathsReservationRepository.delete(...) is documented for “station does not accept” or technical failures and ReservationRepository.accepted(...) sets the reservation status to ACCEPTED; ReserveNowTask calls accepted() on "Accepted" and delete(...) on all other/error paths — behavior matches the repository API and prior expectations.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java (2)
32-56: Nice consolidation of per-version wiring.The TaskDefinition + OcppVersionHandler mapping removes duplication and is clear.
37-55: Validate ChangeConfiguration key/value earlyChangeConfigurationParams permits null/blank: confKey/customConfKey are @nullable and the @notblank for value is commented out; only keyType is @NotNull (steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ChangeConfigurationParams.java, ~lines 36–49). Add lightweight precondition checks in the request lambda to reject null/blank key or value before sending the OCPP request, or re-enable bean validation on the DTO (aware @NotBlank/@pattern were disabled due to issues steve-community#148 and steve-community#920). Confirm intended behavior.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java (1)
39-44: Confirm enum value alignment with ocpp.cp._2015._10.MessageTriggerTriggerMessageEnum.value() = BootNotification, DiagnosticsStatusNotification, FirmwareStatusNotification, Heartbeat, MeterValues, StatusNotification. ocpp.cp._2015._10.MessageTrigger source not in the repo — confirm MessageTrigger.fromValue(...) accepts these exact strings; if not, update TriggerMessageEnum or map/handle the values (or catch IllegalArgumentException) before calling.
Locations: steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java (call site) and steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/TriggerMessageEnum.java.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java (1)
39-79: No additional checks needed — DTO enforces constraints.GetDiagnosticsParams (steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/GetDiagnosticsParams.java) declares @NotBlank/@pattern for location, @min(1) for retries/retryInterval, @past on start/stop and an @AssertTrue isValid() that ensures stop.isAfter(start).
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java (1)
70-76: Constructors look goodConstructors correctly delegate to the new TaskDefinition-based super signatures.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java (2)
110-119: Confirm FULL update semantics for empty listWhen
sendEmptyListWhenFullis null, you send an empty list (clears CP’s list). Validate this matches intended behavior; otherwise treat null asfalse.
132-141: IdTagInfo.status could be null — verify assumption
ocpp16.getStatus().value()will NPE if status is absent. If upstream guarantees non-null, fine; otherwise add a guard or default.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java (1)
32-50: LGTMVersion mapping and response extraction are correct for 1.2/1.5/1.6.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java (1)
50-73: Validate mandatory vendorIdOCPP 1.5/1.6 require
vendorId. Ensure DTO enforces non-null/non-empty to avoid REJECTED responses.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
165-175: Version-driven request mapping looks solidClean separation via TaskDefinition/RequestMapper with explicit unsupported-version guard. No issues.
| @SuppressWarnings("unchecked") | ||
| public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) { | ||
| OcppVersion version = versionMap.get(chargeBoxId); | ||
| OcppVersionHandler<S, RESPONSE> handler = | ||
| taskDefinition.getVersionHandlers().get(version); | ||
| if (handler == null) { | ||
| throw new UnsupportedOperationException("Operation not supported for version " + version); | ||
| } | ||
| ResponseMapper<T, RESPONSE> mapper = (ResponseMapper<T, RESPONSE>) handler.getResponseMapper(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate chargeBoxId before resolving version
Provide a clearer error when chargeBoxId is not part of this task, instead of an UnsupportedOperationException with version null.
- public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) {
- OcppVersion version = versionMap.get(chargeBoxId);
+ public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) {
+ if (!versionMap.containsKey(chargeBoxId)) {
+ throw new IllegalArgumentException("Unknown chargeBoxId for this task: " + chargeBoxId);
+ }
+ OcppVersion version = versionMap.get(chargeBoxId);
OcppVersionHandler<S, RESPONSE> handler =
taskDefinition.getVersionHandlers().get(version);
if (handler == null) {
throw new UnsupportedOperationException("Operation not supported for version " + version);
}📝 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.
| @SuppressWarnings("unchecked") | |
| public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) { | |
| OcppVersion version = versionMap.get(chargeBoxId); | |
| OcppVersionHandler<S, RESPONSE> handler = | |
| taskDefinition.getVersionHandlers().get(version); | |
| if (handler == null) { | |
| throw new UnsupportedOperationException("Operation not supported for version " + version); | |
| } | |
| ResponseMapper<T, RESPONSE> mapper = (ResponseMapper<T, RESPONSE>) handler.getResponseMapper(); | |
| @SuppressWarnings("unchecked") | |
| public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) { | |
| if (!versionMap.containsKey(chargeBoxId)) { | |
| throw new IllegalArgumentException("Unknown chargeBoxId for this task: " + chargeBoxId); | |
| } | |
| OcppVersion version = versionMap.get(chargeBoxId); | |
| OcppVersionHandler<S, RESPONSE> handler = | |
| taskDefinition.getVersionHandlers().get(version); | |
| if (handler == null) { | |
| throw new UnsupportedOperationException("Operation not supported for version " + version); | |
| } | |
| ResponseMapper<T, RESPONSE> mapper = (ResponseMapper<T, RESPONSE>) handler.getResponseMapper(); |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java
around lines 177 to 186, the code calls versionMap.get(chargeBoxId) and proceeds
even if the key is absent, which results in an UnsupportedOperationException
containing "version null"; validate the chargeBoxId first and fail fast with a
clear error: check that versionMap.containsKey(chargeBoxId) (or that the
returned version is non-null) and throw an IllegalArgumentException or a
descriptive exception stating that the given chargeBoxId is not registered for
this task, before attempting to resolve the version and handler so subsequent
code never operates on a null version.
| addCallback(new OcppCallback<>() { | ||
| @Override | ||
| public void success(String chargeBoxId, String statusValue) { | ||
| addNewResponse(chargeBoxId, statusValue); | ||
|
|
||
| if ("Accepted".equalsIgnoreCase(statusValue)) { | ||
| reservationRepository.cancelled(params.getReservationId()); | ||
| reservationRepository.cancelled(getParams().getReservationId()); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public CancelReservationRequest getOcpp15Request() { | ||
| return new CancelReservationRequest().withReservationId(params.getReservationId()); | ||
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
|
|
||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make repository side-effect idempotent across multi-target executions.
If this task targets multiple charge boxes, multiple “Accepted” responses could trigger repeated cancelled() calls. Guard to execute once.
- public CancelReservationTask(
- CancelReservationParams params, ReservationRepository reservationRepository, String caller) {
+ public CancelReservationTask(
+ CancelReservationParams params, ReservationRepository reservationRepository, String caller) {
super(params, caller, TASK_DEFINITION);
-
- addCallback(new OcppCallback<>() {
+ java.util.concurrent.atomic.AtomicBoolean done = new java.util.concurrent.atomic.AtomicBoolean(false);
+ addCallback(new OcppCallback<>() {
@@
- public void success(String chargeBoxId, String statusValue) {
+ public void success(String chargeBoxId, String statusValue) {
addNewResponse(chargeBoxId, statusValue);
- if ("Accepted".equalsIgnoreCase(statusValue)) {
- reservationRepository.cancelled(getParams().getReservationId());
- }
+ if ("Accepted".equalsIgnoreCase(statusValue)
+ && done.compareAndSet(false, true)) {
+ reservationRepository.cancelled(getParams().getReservationId());
+ }
}📝 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.
| addCallback(new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String statusValue) { | |
| addNewResponse(chargeBoxId, statusValue); | |
| if ("Accepted".equalsIgnoreCase(statusValue)) { | |
| reservationRepository.cancelled(params.getReservationId()); | |
| reservationRepository.cancelled(getParams().getReservationId()); | |
| } | |
| } | |
| }; | |
| } | |
| @Override | |
| public CancelReservationRequest getOcpp15Request() { | |
| return new CancelReservationRequest().withReservationId(params.getReservationId()); | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, error.toString()); | |
| } | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.getMessage()); | |
| } | |
| }); | |
| } | |
| java.util.concurrent.atomic.AtomicBoolean done = new java.util.concurrent.atomic.AtomicBoolean(false); | |
| addCallback(new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String statusValue) { | |
| addNewResponse(chargeBoxId, statusValue); | |
| if ("Accepted".equalsIgnoreCase(statusValue) | |
| && done.compareAndSet(false, true)) { | |
| reservationRepository.cancelled(getParams().getReservationId()); | |
| } | |
| } | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, error.toString()); | |
| } | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.getMessage()); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java
around lines 56 to 76, the success callback may call
reservationRepository.cancelled(...) multiple times if multiple charge boxes
return "Accepted"; make the side-effect idempotent by ensuring cancelled(...) is
invoked only once per task execution — add a task-scoped guard (e.g. an
AtomicBoolean or synchronized boolean flag) that flips when you first call
cancelled and check that flag before subsequent calls, or alternatively call a
repository method that is itself idempotent (e.g. cancelledIfNotAlready) and
handle concurrency there so repeated successes do not trigger duplicate state
changes.
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
|
|
||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize error messages (avoid nulls, add exception type).
- public void successError(String chargeBoxId, Object error) {
- addNewError(chargeBoxId, error.toString());
+ public void successError(String chargeBoxId, Object error) {
+ addNewError(chargeBoxId, String.valueOf(error));
@@
- public void failed(String chargeBoxId, Exception e) {
- addNewError(chargeBoxId, e.getMessage());
+ public void failed(String chargeBoxId, Exception e) {
+ String msg = e.getClass().getSimpleName() + ": " + (e.getMessage() != null ? e.getMessage() : "<no message>");
+ addNewError(chargeBoxId, msg);Also applies to: 90-98
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java
around lines 66-75 (and similarly 90-98), the error handling calls pass raw
error.toString() or e.getMessage() which can be null and omit exception type;
change both to build a normalized string that includes the error/exception type
and a non-null message (e.g. for Exception use e.getClass().getSimpleName() + ":
" + (e.getMessage() != null ? e.getMessage() : "<no message>"), and for the
Object error check for null and use error.getClass().getSimpleName() + ": " +
(String.valueOf(error) or "<no message>") ), then pass that constructed string
to addNewError so logs always include type and a safe message.
| private static final TaskDefinition<ChangeAvailabilityParams, String> TASK_DEFINITION = | ||
| TaskDefinition.<ChangeAvailabilityParams, String>builder() | ||
| .versionHandlers(Map.of( | ||
| OcppVersion.V_12, | ||
| new OcppVersionHandler<>( | ||
| task -> new ocpp.cp._2010._08.ChangeAvailabilityRequest() | ||
| .withConnectorId( | ||
| task.getParams().getConnectorId()) | ||
| .withType(ocpp.cp._2010._08.AvailabilityType.fromValue( | ||
| task.getParams() | ||
| .getAvailType() | ||
| .value())), | ||
| (ocpp.cp._2010._08.ChangeAvailabilityResponse r) -> | ||
| r.getStatus().value()), | ||
| OcppVersion.V_15, | ||
| new OcppVersionHandler<>( | ||
| task -> new ocpp.cp._2012._06.ChangeAvailabilityRequest() | ||
| .withConnectorId( | ||
| task.getParams().getConnectorId()) | ||
| .withType(ocpp.cp._2012._06.AvailabilityType.fromValue( | ||
| task.getParams() | ||
| .getAvailType() | ||
| .value())), | ||
| (ocpp.cp._2012._06.ChangeAvailabilityResponse r) -> | ||
| r.getStatus().value()), | ||
| OcppVersion.V_16, | ||
| new OcppVersionHandler<>( | ||
| task -> new ocpp.cp._2015._10.ChangeAvailabilityRequest() | ||
| .withConnectorId( | ||
| task.getParams().getConnectorId()) | ||
| .withType(ocpp.cp._2015._10.AvailabilityType.fromValue( | ||
| task.getParams() | ||
| .getAvailType() | ||
| .value())), | ||
| (ocpp.cp._2015._10.ChangeAvailabilityResponse r) -> | ||
| r.getStatus().value()))) | ||
| .build(); |
There was a problem hiding this comment.
NPE risk: guard null availType before .value()
getParams().getAvailType() may be null; .value() will NPE. Add a null check (or validation) in each mapper.
Apply:
@@
-import java.util.Map;
+import java.util.Map;
+import java.util.Objects;
@@
- task -> new ocpp.cp._2010._08.ChangeAvailabilityRequest()
- .withConnectorId(
- task.getParams().getConnectorId())
- .withType(ocpp.cp._2010._08.AvailabilityType.fromValue(
- task.getParams()
- .getAvailType()
- .value())),
+ task -> {
+ var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType");
+ return new ocpp.cp._2010._08.ChangeAvailabilityRequest()
+ .withConnectorId(task.getParams().getConnectorId())
+ .withType(ocpp.cp._2010._08.AvailabilityType.fromValue(type.value()));
+ },
@@
- task -> new ocpp.cp._2012._06.ChangeAvailabilityRequest()
- .withConnectorId(
- task.getParams().getConnectorId())
- .withType(ocpp.cp._2012._06.AvailabilityType.fromValue(
- task.getParams()
- .getAvailType()
- .value())),
+ task -> {
+ var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType");
+ return new ocpp.cp._2012._06.ChangeAvailabilityRequest()
+ .withConnectorId(task.getParams().getConnectorId())
+ .withType(ocpp.cp._2012._06.AvailabilityType.fromValue(type.value()));
+ },
@@
- task -> new ocpp.cp._2015._10.ChangeAvailabilityRequest()
- .withConnectorId(
- task.getParams().getConnectorId())
- .withType(ocpp.cp._2015._10.AvailabilityType.fromValue(
- task.getParams()
- .getAvailType()
- .value())),
+ task -> {
+ var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType");
+ return new ocpp.cp._2015._10.ChangeAvailabilityRequest()
+ .withConnectorId(task.getParams().getConnectorId())
+ .withType(ocpp.cp._2015._10.AvailabilityType.fromValue(type.value()));
+ },📝 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.
| private static final TaskDefinition<ChangeAvailabilityParams, String> TASK_DEFINITION = | |
| TaskDefinition.<ChangeAvailabilityParams, String>builder() | |
| .versionHandlers(Map.of( | |
| OcppVersion.V_12, | |
| new OcppVersionHandler<>( | |
| task -> new ocpp.cp._2010._08.ChangeAvailabilityRequest() | |
| .withConnectorId( | |
| task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2010._08.AvailabilityType.fromValue( | |
| task.getParams() | |
| .getAvailType() | |
| .value())), | |
| (ocpp.cp._2010._08.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()), | |
| OcppVersion.V_15, | |
| new OcppVersionHandler<>( | |
| task -> new ocpp.cp._2012._06.ChangeAvailabilityRequest() | |
| .withConnectorId( | |
| task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2012._06.AvailabilityType.fromValue( | |
| task.getParams() | |
| .getAvailType() | |
| .value())), | |
| (ocpp.cp._2012._06.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()), | |
| OcppVersion.V_16, | |
| new OcppVersionHandler<>( | |
| task -> new ocpp.cp._2015._10.ChangeAvailabilityRequest() | |
| .withConnectorId( | |
| task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2015._10.AvailabilityType.fromValue( | |
| task.getParams() | |
| .getAvailType() | |
| .value())), | |
| (ocpp.cp._2015._10.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()))) | |
| .build(); | |
| private static final TaskDefinition<ChangeAvailabilityParams, String> TASK_DEFINITION = | |
| TaskDefinition.<ChangeAvailabilityParams, String>builder() | |
| .versionHandlers(Map.of( | |
| OcppVersion.V_12, | |
| new OcppVersionHandler<>( | |
| task -> { | |
| var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType"); | |
| return new ocpp.cp._2010._08.ChangeAvailabilityRequest() | |
| .withConnectorId(task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2010._08.AvailabilityType.fromValue(type.value())); | |
| }, | |
| (ocpp.cp._2010._08.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()), | |
| OcppVersion.V_15, | |
| new OcppVersionHandler<>( | |
| task -> { | |
| var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType"); | |
| return new ocpp.cp._2012._06.ChangeAvailabilityRequest() | |
| .withConnectorId(task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2012._06.AvailabilityType.fromValue(type.value())); | |
| }, | |
| (ocpp.cp._2012._06.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()), | |
| OcppVersion.V_16, | |
| new OcppVersionHandler<>( | |
| task -> { | |
| var type = Objects.requireNonNull(task.getParams().getAvailType(), "availType"); | |
| return new ocpp.cp._2015._10.ChangeAvailabilityRequest() | |
| .withConnectorId(task.getParams().getConnectorId()) | |
| .withType(ocpp.cp._2015._10.AvailabilityType.fromValue(type.value())); | |
| }, | |
| (ocpp.cp._2015._10.ChangeAvailabilityResponse r) -> | |
| r.getStatus().value()))) | |
| .build(); |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java
around lines 32 to 68, the mappers call task.getParams().getAvailType().value()
and will NPE if getAvailType() is null; guard against null before calling
.value() in each OcppVersionHandler mapper by first extracting the availType
into a local variable, validate it (e.g. if (availType == null) throw new
IllegalArgumentException("availType is required for ChangeAvailability task") or
a task-specific validation exception), and only then call .value() (or map to an
appropriate default/response) so the code fails fast with a clear error instead
of throwing an NPE.
| public OcppCallback<String> defaultCallback() { | ||
| return new OcppCallback<>() { | ||
| @Override | ||
| public void success(String chargeBoxId, String response) { | ||
| addNewResponse(chargeBoxId, response); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public AsyncHandler<ocpp.cp._2012._06.ChangeAvailabilityResponse> getOcpp15Handler(String chargeBoxId) { | ||
| return res -> { | ||
| try { | ||
| success(chargeBoxId, res.get().getStatus().value()); | ||
| } catch (Exception e) { | ||
| failed(chargeBoxId, e); | ||
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public AsyncHandler<ocpp.cp._2015._10.ChangeAvailabilityResponse> getOcpp16Handler(String chargeBoxId) { | ||
| return res -> { | ||
| try { | ||
| success(chargeBoxId, res.get().getStatus().value()); | ||
| } catch (Exception e) { | ||
| failed(chargeBoxId, e); | ||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make callback error handling null-safe and more informative
Use String.valueOf(error) and e.toString() to avoid NPE and retain exception type.
- public void successError(String chargeBoxId, Object error) {
- addNewError(chargeBoxId, error.toString());
+ public void successError(String chargeBoxId, Object error) {
+ addNewError(chargeBoxId, String.valueOf(error));
}
@@
- public void failed(String chargeBoxId, Exception e) {
- addNewError(chargeBoxId, e.getMessage());
+ public void failed(String chargeBoxId, Exception e) {
+ addNewError(chargeBoxId, e.toString());
}📝 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.
| public OcppCallback<String> defaultCallback() { | |
| return new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String response) { | |
| addNewResponse(chargeBoxId, response); | |
| } | |
| }; | |
| } | |
| @Override | |
| public AsyncHandler<ocpp.cp._2012._06.ChangeAvailabilityResponse> getOcpp15Handler(String chargeBoxId) { | |
| return res -> { | |
| try { | |
| success(chargeBoxId, res.get().getStatus().value()); | |
| } catch (Exception e) { | |
| failed(chargeBoxId, e); | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, error.toString()); | |
| } | |
| }; | |
| } | |
| @Override | |
| public AsyncHandler<ocpp.cp._2015._10.ChangeAvailabilityResponse> getOcpp16Handler(String chargeBoxId) { | |
| return res -> { | |
| try { | |
| success(chargeBoxId, res.get().getStatus().value()); | |
| } catch (Exception e) { | |
| failed(chargeBoxId, e); | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.getMessage()); | |
| } | |
| }; | |
| public OcppCallback<String> defaultCallback() { | |
| return new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String response) { | |
| addNewResponse(chargeBoxId, response); | |
| } | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, String.valueOf(error)); | |
| } | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.toString()); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java
around lines 79 to 95, the callback error handlers call error.toString() and
e.getMessage(), which can produce NPEs or lose exception type information;
change them to use String.valueOf(error) in successError and e.toString() in
failed so they are null-safe and include exception type, then update the
addNewError calls to pass those String results.
| @Override | ||
| public OcppCallback<String> defaultCallback() { | ||
| return new StringOcppCallback(); | ||
| } | ||
|
|
||
| @Override | ||
| public ocpp.cp._2012._06.SendLocalListRequest getOcpp15Request() { | ||
| ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = getOcpp16Request(); | ||
|
|
||
| return new ocpp.cp._2012._06.SendLocalListRequest() | ||
| .withListVersion(ocpp16Request.getListVersion()) | ||
| .withUpdateType(ocpp.cp._2012._06.UpdateType.fromValue( | ||
| ocpp16Request.getUpdateType().value())) | ||
| .withLocalAuthorisationList(toOcpp15(ocpp16Request.getLocalAuthorizationList())); | ||
| } | ||
|
|
||
| @Override | ||
| public ocpp.cp._2015._10.SendLocalListRequest getOcpp16Request() { | ||
| return request; | ||
| } | ||
| return new OcppCallback<>() { | ||
| @Override | ||
| public void success(String chargeBoxId, String response) { | ||
| addNewResponse(chargeBoxId, response); | ||
| } | ||
|
|
||
| @Override | ||
| public AsyncHandler<ocpp.cp._2012._06.SendLocalListResponse> getOcpp15Handler(String chargeBoxId) { | ||
| return res -> { | ||
| try { | ||
| success(chargeBoxId, res.get().getStatus().value()); | ||
| } catch (Exception e) { | ||
| failed(chargeBoxId, e); | ||
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public AsyncHandler<ocpp.cp._2015._10.SendLocalListResponse> getOcpp16Handler(String chargeBoxId) { | ||
| return res -> { | ||
| try { | ||
| success(chargeBoxId, res.get().getStatus().value()); | ||
| } catch (Exception e) { | ||
| failed(chargeBoxId, e); | ||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safe callback errors
Align with other tasks; avoid NPE on error == null and preserve exception types.
- public void successError(String chargeBoxId, Object error) {
- addNewError(chargeBoxId, error.toString());
+ public void successError(String chargeBoxId, Object error) {
+ addNewError(chargeBoxId, String.valueOf(error));
}
@@
- public void failed(String chargeBoxId, Exception e) {
- addNewError(chargeBoxId, e.getMessage());
+ public void failed(String chargeBoxId, Exception e) {
+ addNewError(chargeBoxId, e.toString());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public OcppCallback<String> defaultCallback() { | |
| return new StringOcppCallback(); | |
| } | |
| @Override | |
| public ocpp.cp._2012._06.SendLocalListRequest getOcpp15Request() { | |
| ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = getOcpp16Request(); | |
| return new ocpp.cp._2012._06.SendLocalListRequest() | |
| .withListVersion(ocpp16Request.getListVersion()) | |
| .withUpdateType(ocpp.cp._2012._06.UpdateType.fromValue( | |
| ocpp16Request.getUpdateType().value())) | |
| .withLocalAuthorisationList(toOcpp15(ocpp16Request.getLocalAuthorizationList())); | |
| } | |
| @Override | |
| public ocpp.cp._2015._10.SendLocalListRequest getOcpp16Request() { | |
| return request; | |
| } | |
| return new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String response) { | |
| addNewResponse(chargeBoxId, response); | |
| } | |
| @Override | |
| public AsyncHandler<ocpp.cp._2012._06.SendLocalListResponse> getOcpp15Handler(String chargeBoxId) { | |
| return res -> { | |
| try { | |
| success(chargeBoxId, res.get().getStatus().value()); | |
| } catch (Exception e) { | |
| failed(chargeBoxId, e); | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, error.toString()); | |
| } | |
| }; | |
| } | |
| @Override | |
| public AsyncHandler<ocpp.cp._2015._10.SendLocalListResponse> getOcpp16Handler(String chargeBoxId) { | |
| return res -> { | |
| try { | |
| success(chargeBoxId, res.get().getStatus().value()); | |
| } catch (Exception e) { | |
| failed(chargeBoxId, e); | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.getMessage()); | |
| } | |
| }; | |
| @Override | |
| public OcppCallback<String> defaultCallback() { | |
| return new OcppCallback<>() { | |
| @Override | |
| public void success(String chargeBoxId, String response) { | |
| addNewResponse(chargeBoxId, response); | |
| } | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, String.valueOf(error)); | |
| } | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.toString()); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java
around lines 80-97, the callback implementations can NPE when error == null and
they lose exception type info by converting Exceptions to messages; update
successError to guard against null (e.g., use Objects.toString(error, "null") or
a null check) and update failed to pass the Exception itself to addNewError (or
overload/change addNewError to accept Throwable/Object) so the original
exception type is preserved instead of calling e.getMessage().
| addCallback(new OcppCallback<>() { | ||
| @Override | ||
| public void success(String chargeBoxId, String statusValue) { | ||
| addNewResponse(chargeBoxId, statusValue); | ||
| if ("Accepted".equalsIgnoreCase(statusValue)) { | ||
| repo.setProfile(details.getChargingProfilePk(), chargeBoxId, params.getConnectorId()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
|
|
||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Remove duplicate result accounting in custom callback
This callback calls addNewResponse/addNewError while the default callback already records results, causing double increments and premature task completion. Keep this callback side-effect-only (DB update) and let the default callback manage resultMap.
Apply:
addCallback(new OcppCallback<>() {
@Override
public void success(String chargeBoxId, String statusValue) {
- addNewResponse(chargeBoxId, statusValue);
- if ("Accepted".equalsIgnoreCase(statusValue)) {
- repo.setProfile(details.getChargingProfilePk(), chargeBoxId, params.getConnectorId());
- }
+ if ("Accepted".equalsIgnoreCase(statusValue)) {
+ repo.setProfile(details.getChargingProfilePk(), chargeBoxId, request.getConnectorId());
+ }
}
@Override
public void successError(String chargeBoxId, Object error) {
- addNewError(chargeBoxId, error.toString());
+ // no-op; default callback records error
}
@Override
public void failed(String chargeBoxId, Exception e) {
- addNewError(chargeBoxId, e.getMessage());
+ // no-op; default callback records error
}
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java
around lines 82 to 101, the custom OcppCallback currently calls
addNewResponse/addNewError which duplicates the default callback's result
accounting; remove those calls so this callback performs only side-effects (e.g.
DB update via repo.setProfile) and does not touch the task resultMap or
completion tracking. Specifically, in success(...) remove addNewResponse(...)
but keep the repo.setProfile(...) call; in successError(...) and failed(...)
remove addNewError(...) and only perform any necessary non-result actions (or
nothing) so the default callback remains solely responsible for recording
responses and errors.
| private static void checkAdditionalConstraints(SetChargingProfileRequest request) { | ||
| Optional.ofNullable(request.getCsChargingProfiles()) | ||
| .map(ocpp.cp._2015._10.ChargingProfile::getChargingProfilePurpose) | ||
| .ifPresent(purpose -> { | ||
| if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE | ||
| && request.getConnectorId() != 0) { | ||
| throw new SteveException.InternalError( | ||
| "ChargePointMaxProfile can only be set at Charge Point ConnectorId 0"); | ||
| } | ||
|
|
||
| if (ChargingProfilePurposeType.TX_PROFILE == purpose && request.getConnectorId() < 1) { | ||
| throw new SteveException.InternalError("TxProfile should only be set at Charge Point ConnectorId > 0"); | ||
| } | ||
| if (purpose == ChargingProfilePurposeType.TX_PROFILE && request.getConnectorId() < 1) { | ||
| throw new SteveException.InternalError( | ||
| "TxProfile should only be set at Charge Point ConnectorId > 0"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null connectorId to avoid NPE during unboxing
Ad-hoc requests may omit connectorId; current comparisons auto-unbox and can NPE.
- Optional.ofNullable(request.getCsChargingProfiles())
+ final Integer connectorId = request.getConnectorId();
+ if (connectorId == null) {
+ throw new SteveException.InternalError("ConnectorId must be provided for SetChargingProfile");
+ }
+ Optional.ofNullable(request.getCsChargingProfiles())
.map(ocpp.cp._2015._10.ChargingProfile::getChargingProfilePurpose)
.ifPresent(purpose -> {
- if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE
- && request.getConnectorId() != 0) {
+ if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE
+ && connectorId != 0) {
throw new SteveException.InternalError(
"ChargePointMaxProfile can only be set at Charge Point ConnectorId 0");
}
- if (purpose == ChargingProfilePurposeType.TX_PROFILE && request.getConnectorId() < 1) {
+ if (purpose == ChargingProfilePurposeType.TX_PROFILE && connectorId < 1) {
throw new SteveException.InternalError(
"TxProfile should only be set at Charge Point ConnectorId > 0");
}
});📝 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.
| private static void checkAdditionalConstraints(SetChargingProfileRequest request) { | |
| Optional.ofNullable(request.getCsChargingProfiles()) | |
| .map(ocpp.cp._2015._10.ChargingProfile::getChargingProfilePurpose) | |
| .ifPresent(purpose -> { | |
| if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE | |
| && request.getConnectorId() != 0) { | |
| throw new SteveException.InternalError( | |
| "ChargePointMaxProfile can only be set at Charge Point ConnectorId 0"); | |
| } | |
| if (ChargingProfilePurposeType.TX_PROFILE == purpose && request.getConnectorId() < 1) { | |
| throw new SteveException.InternalError("TxProfile should only be set at Charge Point ConnectorId > 0"); | |
| } | |
| if (purpose == ChargingProfilePurposeType.TX_PROFILE && request.getConnectorId() < 1) { | |
| throw new SteveException.InternalError( | |
| "TxProfile should only be set at Charge Point ConnectorId > 0"); | |
| } | |
| }); | |
| private static void checkAdditionalConstraints(SetChargingProfileRequest request) { | |
| final Integer connectorId = request.getConnectorId(); | |
| if (connectorId == null) { | |
| throw new SteveException.InternalError("ConnectorId must be provided for SetChargingProfile"); | |
| } | |
| Optional.ofNullable(request.getCsChargingProfiles()) | |
| .map(ocpp.cp._2015._10.ChargingProfile::getChargingProfilePurpose) | |
| .ifPresent(purpose -> { | |
| if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE | |
| && connectorId != 0) { | |
| throw new SteveException.InternalError( | |
| "ChargePointMaxProfile can only be set at Charge Point ConnectorId 0"); | |
| } | |
| if (purpose == ChargingProfilePurposeType.TX_PROFILE && connectorId < 1) { | |
| throw new SteveException.InternalError( | |
| "TxProfile should only be set at Charge Point ConnectorId > 0"); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java
around lines 159 to 173, the code compares request.getConnectorId() using
primitive comparisons which can NPE if connectorId is null; guard against null
by first retrieving the Integer connectorId = request.getConnectorId() and if
it's null throw a clear SteveException (e.g. BadRequest or InternalError per
project convention) indicating connectorId is missing, otherwise perform the
existing comparisons against the non-null Integer (or convert to int) to avoid
unboxing NPEs.
| @Override | ||
| public void successError(String chargeBoxId, Object error) { | ||
| addNewError(chargeBoxId, error.toString()); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public AsyncHandler<ocpp.cp._2015._10.UnlockConnectorResponse> getOcpp16Handler(String chargeBoxId) { | ||
| return res -> { | ||
| try { | ||
| success(chargeBoxId, res.get().getStatus().value()); | ||
| } catch (Exception e) { | ||
| failed(chargeBoxId, e); | ||
| @Override | ||
| public void failed(String chargeBoxId, Exception e) { | ||
| addNewError(chargeBoxId, e.getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden error reporting to avoid null-related NPEs and include exception type.
error.toString() NPEs if null; e.getMessage() may be null.
- public void successError(String chargeBoxId, Object error) {
- addNewError(chargeBoxId, error.toString());
+ public void successError(String chargeBoxId, Object error) {
+ addNewError(chargeBoxId, String.valueOf(error));
@@
- public void failed(String chargeBoxId, Exception e) {
- addNewError(chargeBoxId, e.getMessage());
+ public void failed(String chargeBoxId, Exception e) {
+ String msg = e.getClass().getSimpleName() + ": " + (e.getMessage() != null ? e.getMessage() : "<no message>");
+ addNewError(chargeBoxId, msg);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, error.toString()); | |
| } | |
| }; | |
| } | |
| @Override | |
| public AsyncHandler<ocpp.cp._2015._10.UnlockConnectorResponse> getOcpp16Handler(String chargeBoxId) { | |
| return res -> { | |
| try { | |
| success(chargeBoxId, res.get().getStatus().value()); | |
| } catch (Exception e) { | |
| failed(chargeBoxId, e); | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| addNewError(chargeBoxId, e.getMessage()); | |
| } | |
| @Override | |
| public void successError(String chargeBoxId, Object error) { | |
| addNewError(chargeBoxId, String.valueOf(error)); | |
| } | |
| @Override | |
| public void failed(String chargeBoxId, Exception e) { | |
| String msg = e.getClass().getSimpleName() + ": " + (e.getMessage() != null ? e.getMessage() : "<no message>"); | |
| addNewError(chargeBoxId, msg); | |
| } |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java
around lines 74 to 82, the current handlers call error.toString() and
e.getMessage() which can be null and cause NPEs or lose useful context; change
them to produce safe, non-null strings and include the exception type: for the
successError use a null-safe conversion (e.g. Objects.toString(error, "null") or
String.valueOf(error)) and for failed construct a message that prefixes the
exception class name (e.getClass().getSimpleName()) and appends a null-safe
message (falling back to String.valueOf(e) or a default text) before passing
into addNewError so no NPEs occur and the exception type is recorded.
| .withRetries( | ||
| task.getParams().getRetries()) | ||
| .withRetryInterval( | ||
| task.getParams().getRetryInterval()), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm parameter semantics (units and optionality)
Ensure retryInterval matches OCPP units (seconds) and that retries/retryInterval nulls are handled per CP version expectations.
Run to verify DTO docs and types:
Also applies to: 68-71
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "UpdateFirmwareParams definition and fields:"
rg -n --type=java -C3 'class\s+UpdateFirmwareParams\b|getRetryInterval|getRetries|getRetrieve|getLocation'Length of output: 20062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find UpdateFirmwareParams definitions ==="
rg -n --hidden -S 'class\s+UpdateFirmwareParams\b' || true
echo
echo "=== steve-core: UpdateFirmwareParams (ocpp) ==="
sed -n '1,240p' steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/UpdateFirmwareParams.java || true
echo
echo "=== steve-core: UpdateFirmwareParams (common) ==="
sed -n '1,240p' steve-core/src/main/java/de/rwth/idsg/steve/web/dto/common/UpdateFirmwareParams.java || true
echo
echo "=== MultipleChargePointSelect (common) ==="
sed -n '1,240p' steve-core/src/main/java/de/rwth/idsg/steve/web/dto/common/MultipleChargePointSelect.java || true
echo
echo "=== UpdateFirmwareTask (around usage) ==="
sed -n '1,240p' steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java || true
echo
echo "=== Search for retries / retryInterval usages & definitions ==="
rg -n --hidden -S '\b(retries|retryInterval|getRetries|getRetryInterval|setRetries|setRetryInterval)\b' || true
echo
echo "=== Search for generated OCPP UpdateFirmwareRequest / withRetries signatures ==="
rg -n --hidden -S 'class\s+UpdateFirmwareRequest\b|withRetries\(|withRetryInterval\(' || trueLength of output: 17874
Confirm parameter semantics — allow 0 for retries; retryInterval is seconds
- Change DTO validation: replace @min(1) with @min(0) for retries and retryInterval in steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/UpdateFirmwareParams.java and steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/GetDiagnosticsParams.java — OCPP allows 0 (meaning "no retries") and treats retryInterval as seconds. (ocpp-spec.org)
- Ensure null-handling: when retries/retryInterval == null the generated UpdateFirmware/GetDiagnostics request MUST omit those elements (WSDL minOccurs=0) — verify withRetries()/withRetryInterval() marshalling does not force a default.
- Update UI/docs (steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/op-forms/UpdateFirmwareForm.jsp) to state "0 = no retries" and "retryInterval in seconds".
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java
around lines 44-47, and the related DTOs and UI files: change DTO validation
annotations in
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/UpdateFirmwareParams.java
and
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/GetDiagnosticsParams.java
from @Min(1) to @Min(0) for both retries and retryInterval; update
UpdateFirmwareTask.java so it does not call withRetries(...) or
withRetryInterval(...) when the DTO values are null (only call those builder
methods when value != null) to ensure the generated request omits the elements
(minOccurs=0); verify the marshaller does not insert defaults if methods are not
called; and update
steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/op-forms/UpdateFirmwareForm.jsp
to document "0 = no retries" and "retryInterval in seconds".
juherr
force-pushed
the
refactor-task-management
branch
from
00a7c85 to
fcc05f3
Compare
This commit refactors the OCPP task management system to be more generic and maintainable by introducing a mapper-based architecture. - Decouples task logic from OCPP versions using `TaskDefinition` and mappers. - Groups version-specific mappers in `OcppVersionHandler`. - Simplifies the `CommunicationTask` base class and all concrete task implementations. - Updates the `ChargePointServiceInvoker` to use the new generic system.
…ds and null handling
juherr
force-pushed
the
refactor-task-management
branch
from
fcc05f3 to
a284a8c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Great! I'm ready to address any feedback that comes from the review. |
|
Skipped: Cannot respond to another bot. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/TransactionMapper.java (1)
56-59: Fix NullAway: method can return null but signature is non-null.
return null;triggers the pipeline warning. Mark the method as nullable (or return a default if your DTO supports it).Apply:
+import javax.annotation.Nullable; @@ - private static TransactionStopEventActor toDto(jooq.steve.db.enums.TransactionStopEventActor actor) { + private static @Nullable TransactionStopEventActor toDto(jooq.steve.db.enums.TransactionStopEventActor actor) { if (actor == null) { return null; }steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (1)
208-209: Align nullability with interface: parameter should be @nullableThe interface declares
@Nullable ConnectorStatusForm; annotate the override to fix NullAway warning.- public List<ConnectorStatus> getChargePointConnectorStatus(ConnectorStatusForm form) { + public List<ConnectorStatus> getChargePointConnectorStatus(@Nullable ConnectorStatusForm form) {steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-cp-misc.jsp (1)
37-45: Fix latitude/longitude condition typo to enable Maps linkThe condition checks longitude twice; include latitude.
- <c:if test="${(not empty cp.locationLongitude) and (not empty cp.locationLongitude)}"> + <c:if test="${(not empty cp.locationLatitude) and (not empty cp.locationLongitude)}">steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java (1)
104-111: Fix build: replace Stream.toList() (JDK 16+) with Collectors.toList()Likely cause of the reported build failure on older JDK targets. Use Collectors.toList() and add the missing import.
@@ - return ocpp16.stream() - .map(k -> new ocpp.cp._2012._06.AuthorisationData() - .withIdTag(k.getIdTag()) - .withIdTagInfo(toOcpp15(k.getIdTagInfo()))) - .toList(); + return ocpp16.stream() + .map(k -> new ocpp.cp._2012._06.AuthorisationData() + .withIdTag(k.getIdTag()) + .withIdTagInfo(toOcpp15(k.getIdTagInfo()))) + .collect(java.util.stream.Collectors.toList());@@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.stream.Collectors; // if you prefer a simple name, adjust the call above accordinglyAlso applies to: 30-33
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java (1)
68-92: Only clear DB when status is Accepted; also null‑guard filterType.Clearing profiles on non‑Accepted responses desynchronizes DB and CP state; switching on a null filterType will NPE.
Apply:
- public void success(String chargeBoxId, String statusValue) { - addNewResponse(chargeBoxId, statusValue); - - switch (params.getFilterType()) { + public void success(String chargeBoxId, String statusValue) { + addNewResponse(chargeBoxId, statusValue); + final var filterType = params.getFilterType(); + if (filterType == null) { + log.warn("ClearChargingProfile: filterType is null for chargebox '{}'", chargeBoxId); + return; + } + + if ("Accepted".equalsIgnoreCase(statusValue)) { + switch (filterType) { case ChargingProfileId -> chargingProfileRepository.clearProfile(params.getChargingProfilePk(), chargeBoxId); case OtherParameters -> chargingProfileRepository.clearProfile( chargeBoxId, params.getConnectorId(), params.getChargingProfilePurpose(), params.getStackLevel()); - default -> { - log.warn("Unexpected {} enum value", ClearChargingProfileFilterType.class.getSimpleName()); - return; - } - } - - if (!"Accepted".equalsIgnoreCase(statusValue)) { - log.info( - "Deleted charging profile(s) for chargebox '{}' from DB even though the response was {}", - chargeBoxId, - statusValue); - } + default -> log.warn("Unexpected {} enum value", + ClearChargingProfileFilterType.class.getSimpleName()); + } + } else { + log.info("ClearChargingProfile rejected for '{}': {}", chargeBoxId, statusValue); + } }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
134-142: Fix NullAway: guard null from resultMap.get()resultMap.get(chargeBoxId) can be null; addNull check mirrors addNewResponse().
- public void addNewError(String chargeBoxId, @Nullable String errorMessage) { - resultMap.get(chargeBoxId).setErrorMessage(errorMessage); + public void addNewError(String chargeBoxId, @Nullable String errorMessage) { + var result = resultMap.get(chargeBoxId); + if (result == null) { + log.warn("Received error for unknown chargeBoxId '{}'", chargeBoxId); + return; + } + result.setErrorMessage(errorMessage);
♻️ Duplicate comments (8)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java (1)
54-60: Constructor wiring to TASK_DEFINITION: good; prior NPE risks from handler strings are gone.Removing per-version handlers also eliminates earlier
error.toString()/e.getMessage()pitfalls flagged before.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java (2)
62-83: Race condition when canceling reservations across multiple charge boxes.When this task targets multiple charge boxes, multiple "Accepted" responses could trigger repeated
cancelled()calls on the same reservation. This could lead to inconsistent state or errors if the repository method is not idempotent.Apply this diff to ensure the cancellation is executed only once:
@Override protected OcppCallback<String> createDefaultCallback() { + java.util.concurrent.atomic.AtomicBoolean cancelled = new java.util.concurrent.atomic.AtomicBoolean(false); return new OcppCallback<>() { @Override public void success(String chargeBoxId, String statusValue) { addNewResponse(chargeBoxId, statusValue); - if ("Accepted".equalsIgnoreCase(statusValue)) { + if ("Accepted".equalsIgnoreCase(statusValue) && cancelled.compareAndSet(false, true)) { reservationRepository.cancelled(params.getReservationId()); } }
73-81: Improve error handling to avoid NPEs and include exception types.The error handlers can throw NPEs if the error object is null or if the exception message is null. Additionally, logging just the message without the exception type loses valuable debugging context.
Apply this diff to make error handling more robust:
@Override public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + addNewError(chargeBoxId, String.valueOf(error)); } @Override public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + String msg = e.getClass().getSimpleName() + ": " + (e.getMessage() != null ? e.getMessage() : "<no message>"); + addNewError(chargeBoxId, msg); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java (1)
31-63: Verify null handling for optional retry parameters.The code directly passes
getRetries()andgetRetryInterval()to the request builders without null checks. According to OCPP specification, these are optional parameters (minOccurs=0) and should be omitted from the request when null.Let me verify how the UpdateFirmwareParams handles these optional fields:
#!/bin/bash # Check the UpdateFirmwareParams DTO for validation annotations and null handling echo "=== UpdateFirmwareParams definition and validations ===" cat steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/UpdateFirmwareParams.java 2>/dev/null || \ fd -t f "UpdateFirmwareParams.java" --exec cat {} echo -e "\n=== Check if the OCPP request builders handle null values ===" # Search for withRetries/withRetryInterval method signatures in generated code fd -e java -x grep -l "withRetries\|withRetryInterval" {} \; | head -5 | xargs grep -A2 "withRetries\|withRetryInterval"Based on the verification results, apply this improvement to handle null values properly:
.versionHandler( OcppVersion.V_12, new OcppVersionHandler<>( - task -> new ocpp.cp._2010._08.UpdateFirmwareRequest() - .withLocation(task.getParams().getLocation()) - .withRetrieveDate(toOffsetDateTime( - task.getParams().getRetrieve())) - .withRetries(task.getParams().getRetries()) - .withRetryInterval(task.getParams().getRetryInterval()), + task -> { + var req = new ocpp.cp._2010._08.UpdateFirmwareRequest() + .withLocation(task.getParams().getLocation()) + .withRetrieveDate(toOffsetDateTime( + task.getParams().getRetrieve())); + if (task.getParams().getRetries() != null) { + req.withRetries(task.getParams().getRetries()); + } + if (task.getParams().getRetryInterval() != null) { + req.withRetryInterval(task.getParams().getRetryInterval()); + } + return req; + }, r -> "OK"))Apply the same pattern for V_15 and V_16 handlers.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java (1)
34-38: Guard null availType before.value()to avoid NPE.
getAvailType()can be null; current.value()will NPE. Validate first and pass the mapped enum.Apply:
@@ - task -> new ocpp.cp._2010._08.ChangeAvailabilityRequest() - .withConnectorId(task.getParams().getConnectorId()) - .withType(ocpp.cp._2010._08.AvailabilityType.fromValue(task.getParams() - .getAvailType() - .value())), + task -> { + var type = java.util.Objects.requireNonNull( + task.getParams().getAvailType(), "availType"); + return new ocpp.cp._2010._08.ChangeAvailabilityRequest() + .withConnectorId(task.getParams().getConnectorId()) + .withType(ocpp.cp._2010._08.AvailabilityType.fromValue(type.value())); + }, @@ - task -> new ocpp.cp._2012._06.ChangeAvailabilityRequest() + task -> { + var type = java.util.Objects.requireNonNull( + task.getParams().getAvailType(), "availType"); + return new ocpp.cp._2012._06.ChangeAvailabilityRequest() .withConnectorId(task.getParams().getConnectorId()) - .withType(ocpp.cp._2012._06.AvailabilityType.fromValue(task.getParams() - .getAvailType() - .value())), + .withType(ocpp.cp._2012._06.AvailabilityType.fromValue(type.value())); + }, @@ - task -> new ocpp.cp._2015._10.ChangeAvailabilityRequest() + task -> { + var type = java.util.Objects.requireNonNull( + task.getParams().getAvailType(), "availType"); + return new ocpp.cp._2015._10.ChangeAvailabilityRequest() .withConnectorId(task.getParams().getConnectorId()) - .withType(ocpp.cp._2015._10.AvailabilityType.fromValue(task.getParams() - .getAvailType() - .value())), + .withType(ocpp.cp._2015._10.AvailabilityType.fromValue(type.value())); + },Also applies to: 46-49, 56-58
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java (1)
40-44: Fix SOAP date types: use XMLGregorianCalendar instead of OffsetDateTime.
withStartTime/withStopTimein ocpp.cp SOAP bindings expect XMLGregorianCalendar; passing OffsetDateTime will break marshalling. Convert via DatatypeFactory (or a shared util) and update all three version handlers.Apply this diff to the shown lines:
- .withStartTime(toOffsetDateTime( - task.getParams().getStart())) - .withStopTime(toOffsetDateTime( - task.getParams().getStop())), + .withStartTime(toXmlDateTime(toOffsetDateTime(task.getParams().getStart()))) + .withStopTime(toXmlDateTime(toOffsetDateTime(task.getParams().getStop()))),Repeat the same replacement for V_15 and V_16 blocks.
Add a helper in this class (or in DateTimeUtils and import statically):
private static javax.xml.datatype.XMLGregorianCalendar toXmlDateTime(java.time.OffsetDateTime odt) { if (odt == null) return null; var gc = java.util.GregorianCalendar.from(odt.toZonedDateTime()); try { return javax.xml.datatype.DatatypeFactory.newInstance().newXMLGregorianCalendar(gc); } catch (javax.xml.datatype.DatatypeConfigurationException e) { throw new IllegalStateException("Cannot create XMLGregorianCalendar", e); } }Also applies to: 52-56, 64-68
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java (1)
147-161: Avoid unboxing NPE; validate connectorId upfront.
request.getConnectorId()is an Integer; comparing with primitives can NPE. Also enforce presence per OCPP spec.- private static void checkAdditionalConstraints(SetChargingProfileRequest request) { - Optional.ofNullable(request.getCsChargingProfiles()) + private static void checkAdditionalConstraints(SetChargingProfileRequest request) { + final Integer connectorId = request.getConnectorId(); + if (connectorId == null) { + throw new SteveException.InternalError("ConnectorId must be provided for SetChargingProfile"); + } + Optional.ofNullable(request.getCsChargingProfiles()) .map(ocpp.cp._2015._10.ChargingProfile::getChargingProfilePurpose) .ifPresent(purpose -> { if (purpose == ChargingProfilePurposeType.CHARGE_POINT_MAX_PROFILE - && request.getConnectorId() != 0) { + && connectorId != 0) { throw new SteveException.InternalError( "ChargePointMaxProfile can only be set at Charge Point ConnectorId 0"); } - if (purpose == ChargingProfilePurposeType.TX_PROFILE && request.getConnectorId() < 1) { + if (purpose == ChargingProfilePurposeType.TX_PROFILE && connectorId < 1) { throw new SteveException.InternalError( "TxProfile should only be set at Charge Point ConnectorId > 0"); } }); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
193-201: Validate chargeBoxId before resolving version (prevents "version null")Add an explicit check and fail fast with a clear message.
public <T extends ResponseType> AsyncHandler<T> createHandler(String chargeBoxId) { - var version = versionMap.get(chargeBoxId); + if (!versionMap.containsKey(chargeBoxId)) { + throw new IllegalArgumentException("Unknown chargeBoxId for this task: " + chargeBoxId); + } + var version = versionMap.get(chargeBoxId); var handler = taskDefinition.getVersionHandlers().get(version); if (handler == null) { throw new UnsupportedOperationException("Operation not supported for version " + version); } var mapper = (ResponseMapper<T, RESPONSE>) handler.getResponseMapper();Also applies to: 202-209
🧹 Nitpick comments (30)
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/TransactionMapper.java (2)
60-60: Use locale‑independent uppercasing.Avoid locale surprises (e.g., Turkish i). Use
Locale.ROOT.Apply:
+import java.util.Locale; @@ - return TransactionStopEventActor.valueOf(actor.getLiteral().toUpperCase()); + return TransactionStopEventActor.valueOf(actor.getLiteral().toUpperCase(Locale.ROOT));
56-61: Behavior change: mapping now uses DB literal instead of enum name. Verify one‑to‑one alignment.If any DB literal doesn’t match the repository enum (modulo uppercasing),
valueOfwill throw. Confirm literals align or add a guarded fallback.Optionally harden:
- return TransactionStopEventActor.valueOf(actor.getLiteral().toUpperCase(Locale.ROOT)); + final String lit = actor.getLiteral(); + if (lit == null) return null; + try { + return TransactionStopEventActor.valueOf(lit.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException ex) { + // TODO: consider logging and/or mapping to a safe default if available + return null; + }steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java (2)
77-80: Align with callback semantics and document intent
getDefaultCallback().successError(...)reads odd; if this is the intended “error path,” add a short comment to clarify that this forwards JSON-RPC errors to the task’s default callback.
64-65: Consider clearing handlers after use to avoid stale referencesIf these handlers are one-shot per request, clear them once a response/error arrives to prevent accidental reuse or memory retention on long-lived sessions.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/RequestResult.java (1)
32-34: Add helpers to enforce invariants and reduce null checksWith both
responseanderrorMessagenullable, callers can easily misuse. Provide small helpers likeboolean isSuccess(),T getOrThrow(), and factory methodssuccess(T)/error(String)to encode invariants.public class RequestResult<T> { private @Nullable T response; private @Nullable String errorMessage; + + public boolean isSuccess() { return errorMessage == null; } + public T getOrThrow() { + if (response != null) return response; + throw new IllegalStateException(errorMessage == null ? "No response" : errorMessage); + } + public static <T> RequestResult<T> success(T r) { + var rr = new RequestResult<T>(); + rr.setResponse(r); + return rr; + } + public static <T> RequestResult<T> error(String msg) { + var rr = new RequestResult<T>(); + rr.setErrorMessage(msg); + return rr; + } }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/OcppVersionHandler.java (1)
31-33: Wildcards erode type-safety; consider parameterizing over Task and RequestThe current
? extendswildcards force unchecked casts in consumers. Prefer a fully-parameterized handler to preserve compile-time safety.-public class OcppVersionHandler<S extends ChargePointSelection, U> { - private final RequestMapper<? extends CommunicationTask<S, U>, ? extends RequestType> requestMapper; - private final ResponseMapper<? extends ResponseType, U> responseMapper; +public class OcppVersionHandler< + S extends ChargePointSelection, + U, + C extends CommunicationTask<S, U>, + R extends RequestType> { + private final RequestMapper<C, R> requestMapper; + private final ResponseMapper<? extends ResponseType, U> responseMapper; }If refactoring scope is tight, at least add an internal, clearly documented unchecked cast helper that returns
RequestMapper<C,R>to centralize the suppression.steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargePoint.java (1)
47-61: Nullable annotations for optional DB-backed fieldsMost of these detail fields can be null at the DB level. Mark them
@Nullableto match reality and silence downstream NullAway issues when mappers pass nulls.import lombok.Getter; +import org.jspecify.annotations.Nullable; ... - private final String endpointAddress; + private final @Nullable String endpointAddress; private final String ocppProtocol; - private final String chargePointVendor; - private final String chargePointModel; - private final String chargePointSerialNumber; - private final String chargeBoxSerialNumber; - private final String fwVersion; - private final Instant fwUpdateTimestamp; - private final String iccid; - private final String imsi; - private final String meterType; - private final String meterSerialNumber; - private final String diagnosticsStatus; - private final Instant diagnosticsTimestamp; - private final Instant lastHeartbeatTimestamp; + private final @Nullable String chargePointVendor; + private final @Nullable String chargePointModel; + private final @Nullable String chargePointSerialNumber; + private final @Nullable String chargeBoxSerialNumber; + private final @Nullable String fwVersion; + private final @Nullable Instant fwUpdateTimestamp; + private final @Nullable String iccid; + private final @Nullable String imsi; + private final @Nullable String meterType; + private final @Nullable String meterSerialNumber; + private final @Nullable String diagnosticsStatus; + private final @Nullable Instant diagnosticsTimestamp; + private final @Nullable Instant lastHeartbeatTimestamp;steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargePointMapper.java (1)
34-35: Remove no-op call tor.getAdminAddress()This early call has no side effects in jOOQ records and is redundant since you set it on the builder later.
- r.getAdminAddress();steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (2)
61-66: Add summary line to Javadoc to satisfy style checkProvide a one-line summary before tags.
-/** - * @author Sevket Goekay <sevketgokay@gmail.com> - * @since 14.08.2014 - */ +/** + * Repository implementation for ChargePoint queries and mutations. + * + * @author Sevket Goekay <sevketgokay@gmail.com> + * @since 14.08.2014 + */
173-184: Avoid default time-zone; pass an explicit ZoneIdReplace
LocalDate.now()withLocalDate.now(zoneId)to meet the rule and avoid environment-dependent results.- switch (form.getHeartbeatPeriod()) { + var zoneId = java.time.ZoneId.systemDefault(); + switch (form.getHeartbeatPeriod()) { case ALL -> {} case TODAY -> - selectQuery.addConditions( - date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP).eq(LocalDate.now())); + selectQuery.addConditions(date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP).eq(LocalDate.now(zoneId))); case YESTERDAY -> - selectQuery.addConditions(date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP) - .eq(LocalDate.now().minusDays(1))); + selectQuery.addConditions(date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP) + .eq(LocalDate.now(zoneId).minusDays(1))); case EARLIER -> - selectQuery.addConditions(date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP) - .lessThan(LocalDate.now().minusDays(1))); + selectQuery.addConditions(date(CHARGE_BOX.LAST_HEARTBEAT_TIMESTAMP) + .lessThan(LocalDate.now(zoneId).minusDays(1)));steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java (1)
107-111: Handle error cases when response is null
getResponse()can be null; consider surfacingerrorMessageto the view or guarding to avoid NPEs in JSPs.- var response = extractResult(task, chargeBoxId).getResponse(); + var rr = extractResult(task, chargeBoxId); + var response = rr.getResponse(); + model.addAttribute("errorMessage", rr.getErrorMessage());Also applies to: 115-119
steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/chargepointDetails.jsp (1)
87-101: Bindings migrated to cp.*: OK; format timestamps to avoid raw toString and add null-safe fallback.Suggest formatting date-like fields; current output may be unlocalized or “null”.
Apply to the three timestamp rows:
- <tr><td>Firmware Update Timestamp:</td><td><encode:forHtml value="${cp.fwUpdateTimestamp}" /></td></tr> + <tr><td>Firmware Update Timestamp:</td><td><c:choose><c:when test="${not empty cp.fwUpdateTimestamp}"><fmt:formatDate value="${cp.fwUpdateTimestamp}" pattern="yyyy-MM-dd HH:mm:ss" /></c:when><c:otherwise>-</c:otherwise></c:choose></td></tr> - <tr><td>Diagnostics Timestamp:</td><td><encode:forHtml value="${cp.diagnosticsTimestamp}" /></td></tr> + <tr><td>Diagnostics Timestamp:</td><td><c:choose><c:when test="${not empty cp.diagnosticsTimestamp}"><fmt:formatDate value="${cp.diagnosticsTimestamp}" pattern="yyyy-MM-dd HH:mm:ss" /></c:when><c:otherwise>-</c:otherwise></c:choose></td></tr> - <tr><td>Last Heartbeat Timestamp:</td><td><encode:forHtml value="${cp.lastHeartbeatTimestamp}" /></td></tr> + <tr><td>Last Heartbeat Timestamp:</td><td><c:choose><c:when test="${not empty cp.lastHeartbeatTimestamp}"><fmt:formatDate value="${cp.lastHeartbeatTimestamp}" pattern="yyyy-MM-dd HH:mm:ss" /></c:when><c:otherwise>-</c:otherwise></c:choose></td></tr>Please confirm these three fields are
java.util.Date/java.timetypes; if already Strings, we’ll keep the current<encode:forHtml>and add a “-” fallback instead.steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java (2)
61-71: Nice dispatch dedup; consider making helper static and add minimal precondition.
invokeis pure; make itstaticand guardcpwithObjects.requireNonNull(cp)for clearer NPEs.- private <T extends CommunicationTask<?, ?>> void invoke( + private static <T extends CommunicationTask<?, ?>> void invoke( ChargePointSelect cp, T task, BiConsumer<ChargePointSelect, T> soapAction, BiConsumer<ChargePointSelect, T> jsonAction) { - if (cp.isSoap()) { + java.util.Objects.requireNonNull(cp, "cp"); + if (cp.isSoap()) { soapAction.accept(cp, task); } else { jsonAction.accept(cp, task); } }
102-108: Add @OverRide to implemented public methods in ChargePointServiceInvokerImplMultiple public methods in steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java lack @OverRide; add @OverRide to all implemented interface/superclass methods to catch signature drift.
Affected methods: reset, clearCache, getDiagnostics, updateFirmware, unlockConnector, changeAvailability, changeConfiguration, remoteStartTransaction, remoteStopTransaction, dataTransfer, getConfiguration, getLocalListVersion, sendLocalList, reserveNow, cancelReservation, clearChargingProfile, setChargingProfile, getCompositeSchedule, triggerMessage.
- public void changeAvailability(ChargePointSelect cp, ChangeAvailabilityTask task) { + @Override + public void changeAvailability(ChargePointSelect cp, ChangeAvailabilityTask task) {steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java (1)
29-53: Validate connectorId before building requests.
withConnectorId(...)will blow up on null; fail fast with a clear message.+import java.util.Objects; @@ - task -> new ocpp.cp._2010._08.UnlockConnectorRequest() - .withConnectorId(task.getParams().getConnectorId()), + task -> new ocpp.cp._2010._08.UnlockConnectorRequest() + .withConnectorId(Objects.requireNonNull(task.getParams().getConnectorId(), "connectorId")), @@ - task -> new ocpp.cp._2012._06.UnlockConnectorRequest() - .withConnectorId(task.getParams().getConnectorId()), + task -> new ocpp.cp._2012._06.UnlockConnectorRequest() + .withConnectorId(Objects.requireNonNull(task.getParams().getConnectorId(), "connectorId")), @@ - task -> new ocpp.cp._2015._10.UnlockConnectorRequest() - .withConnectorId(task.getParams().getConnectorId()), + task -> new ocpp.cp._2015._10.UnlockConnectorRequest() + .withConnectorId(Objects.requireNonNull(task.getParams().getConnectorId(), "connectorId")),steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java (1)
29-55: Add null/blank guard for key/value.OCPP expects both; enforce minimal validation to surface issues early.
+import java.util.Objects; @@ - task -> new ocpp.cp._2010._08.ChangeConfigurationRequest() - .withKey(task.getParams().getKey()) - .withValue(task.getParams().getValue()), + task -> new ocpp.cp._2010._08.ChangeConfigurationRequest() + .withKey(Objects.requireNonNull(task.getParams().getKey(), "key")) + .withValue(Objects.requireNonNull(task.getParams().getValue(), "value")), @@ - task -> new ocpp.cp._2012._06.ChangeConfigurationRequest() - .withKey(task.getParams().getKey()) - .withValue(task.getParams().getValue()), + task -> new ocpp.cp._2012._06.ChangeConfigurationRequest() + .withKey(Objects.requireNonNull(task.getParams().getKey(), "key")) + .withValue(Objects.requireNonNull(task.getParams().getValue(), "value")), @@ - task -> new ocpp.cp._2015._10.ChangeConfigurationRequest() - .withKey(task.getParams().getKey()) - .withValue(task.getParams().getValue()), + task -> new ocpp.cp._2015._10.ChangeConfigurationRequest() + .withKey(Objects.requireNonNull(task.getParams().getKey(), "key")) + .withValue(Objects.requireNonNull(task.getParams().getValue(), "value")),steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java (3)
82-88: Null-safe handling for add/update and delete listsgetDeleteList() and getAddUpdateList() may be null; current loop/addAll would NPE. Guard with empty lists.
- if (getParams().getUpdateType() == SendLocalListUpdateType.DIFFERENTIAL) { - List<ocpp.cp._2015._10.AuthorizationData> auths = new ArrayList<>(); - for (String idTag : getParams().getDeleteList()) { + if (getParams().getUpdateType() == SendLocalListUpdateType.DIFFERENTIAL) { + List<ocpp.cp._2015._10.AuthorizationData> auths = new ArrayList<>(); + List<String> deleteList = getParams().getDeleteList() != null + ? getParams().getDeleteList() : Collections.emptyList(); + for (String idTag : deleteList) { auths.add(new ocpp.cp._2015._10.AuthorizationData().withIdTag(idTag)); } - auths.addAll(ocppTagsService.getAuthData(getParams().getAddUpdateList())); + List<String> addUpdateList = getParams().getAddUpdateList() != null + ? getParams().getAddUpdateList() : Collections.emptyList(); + auths.addAll(ocppTagsService.getAuthData(addUpdateList));
49-51: Tidy: drop method parameter and use the field directlycreateOcpp16Request receives OcppTagsService but always uses the task’s field. Simplify the signature and call sites.
@@ - ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = - t.createOcpp16Request(t.ocppTagsService); + ocpp.cp._2015._10.SendLocalListRequest ocpp16Request = + t.createOcpp16Request(); @@ - return t.createOcpp16Request(t.ocppTagsService); + return t.createOcpp16Request(); @@ - private ocpp.cp._2015._10.SendLocalListRequest createOcpp16Request(OcppTagsService ocppTagsService) { + private ocpp.cp._2015._10.SendLocalListRequest createOcpp16Request() {Also applies to: 66-67, 81-81
113-122: Defensive mapping: handle null IdTagInfo.status gracefullyIf status is unexpectedly null, AuthorizationStatus.fromValue(...) will NPE. Safe fallback avoids crashing the task.
return new ocpp.cp._2012._06.IdTagInfo() .withParentIdTag(ocpp16.getParentIdTag()) .withExpiryDate(ocpp16.getExpiryDate()) - .withStatus(ocpp.cp._2012._06.AuthorizationStatus.fromValue( - ocpp16.getStatus().value())); + .withStatus(ocpp16.getStatus() == null + ? null + : ocpp.cp._2012._06.AuthorizationStatus.fromValue(ocpp16.getStatus().value()));steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java (1)
34-37: Guard against null resetType before fromValue(...)A null resetType will NPE at .value(). Add a quick precondition.
- task -> new ocpp.cp._2010._08.ResetRequest() - .withType(ocpp.cp._2010._08.ResetType.fromValue(task.getParams() - .getResetType() - .value())), + task -> { + var rt = java.util.Objects.requireNonNull( + task.getParams().getResetType(), "resetType is required"); + return new ocpp.cp._2010._08.ResetRequest() + .withType(ocpp.cp._2010._08.ResetType.fromValue(rt.value())); + }, @@ - task -> new ocpp.cp._2012._06.ResetRequest() - .withType(ocpp.cp._2012._06.ResetType.fromValue(task.getParams() - .getResetType() - .value())), + task -> { + var rt = java.util.Objects.requireNonNull( + task.getParams().getResetType(), "resetType is required"); + return new ocpp.cp._2012._06.ResetRequest() + .withType(ocpp.cp._2012._06.ResetType.fromValue(rt.value())); + }, @@ - task -> new ocpp.cp._2015._10.ResetRequest() - .withType(ocpp.cp._2015._10.ResetType.fromValue(task.getParams() - .getResetType() - .value())), + task -> { + var rt = java.util.Objects.requireNonNull( + task.getParams().getResetType(), "resetType is required"); + return new ocpp.cp._2015._10.ResetRequest() + .withType(ocpp.cp._2015._10.ResetType.fromValue(rt.value())); + },Also applies to: 43-46, 53-55
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java (1)
37-44: Null-safe toString to avoid NPE when status is nullStringBuilder(String) throws NPE on null. Wrap with String.valueOf.
- public String toString() { - StringBuilder builder = new StringBuilder(status); + public String toString() { + StringBuilder builder = new StringBuilder(String.valueOf(status)); if (data != null) { builder.append(" / Data: ").append(data); } return builder.toString(); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java (1)
94-102: Null‑safe, more informative error recording.Use String.valueOf(error) and e.toString().
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e == null ? "null" : e.toString()); }steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetConfigurationTask.java (2)
48-53: Avoid NPE when unknownKey list is null.JAXB may return null for
getUnknownKey(). Sanitize before joining.- return new ResponseWrapper(keyValues, r.getUnknownKey()); + var u = r.getUnknownKey(); + return new ResponseWrapper(keyValues, u == null ? java.util.List.of() : u); @@ - return new ResponseWrapper(keyValues, r.getUnknownKey()); + var u = r.getUnknownKey(); + return new ResponseWrapper(keyValues, u == null ? java.util.List.of() : u);Also applies to: 59-64
75-83: Consider preserving unknownKeys as List for callers.Flattening to a single String loses structure; exposing both
List<String> unknownKeysand a derivedString unknownKeysJoinedkeeps flexibility.Do consumers rely on a String here, or would a List be preferable?
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java (2)
92-97: Use request connectorId for DB write; make error recording null‑safe.Align stored connectorId with the request; improve error recording.
- addNewResponse(chargeBoxId, statusValue); - if ("Accepted".equalsIgnoreCase(statusValue)) { - repo.setProfile(details.getChargingProfilePk(), chargeBoxId, params.getConnectorId()); - } + addNewResponse(chargeBoxId, statusValue); + if ("Accepted".equalsIgnoreCase(statusValue)) { + repo.setProfile(details.getChargingProfilePk(), chargeBoxId, request.getConnectorId()); + } @@ - public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error)); } @@ - public void failed(String chargeBoxId, Exception e) { - addNewError(chargeBoxId, e.getMessage()); + public void failed(String chargeBoxId, Exception e) { + addNewError(chargeBoxId, e == null ? "null" : e.toString()); }Also applies to: 100-106
47-55: Minor: casting to this class inside mapper couples TASK_DEFINITION to impl.Safe here, but consider passing the request via params or a supplier to avoid
(SetChargingProfileTask) taskcast.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (4)
101-106: Make callbacks collection concurrency-safeArrayList can throw ConcurrentModificationException if addCallback is called while iterating in success/failed. Prefer CopyOnWriteArrayList.
@@ -import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicInteger; @@ - private final List<OcppCallback<RESPONSE>> callbackList; + private final CopyOnWriteArrayList<OcppCallback<RESPONSE>> callbackList; @@ - this.callbackList = new ArrayList<>(2); + this.callbackList = new CopyOnWriteArrayList<>();Also applies to: 72-72, 39-41
164-175: Avoid potential NPE in successErrorUse String.valueOf(error) instead of error.toString().
- public void successError(String chargeBoxId, Object error) { - addNewError(chargeBoxId, error.toString()); + public void successError(String chargeBoxId, Object error) { + addNewError(chargeBoxId, String.valueOf(error));
183-191: Address TypeParameterUnusedInFormals warning (minimal or refactor)The generic T is only in the return type; Error Prone flags this.
Minimal (suppress warning now):
- @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "TypeParameterUnusedInFormals"}) public <T extends RequestType> T getRequest(OcppVersion version) {Better (follow-up): add a type token to bind T and remove the cast:
- public <T extends RequestType> T getRequest(OcppVersion version) { + public <T extends RequestType> T getRequest(OcppVersion version, Class<T> requestType) { @@ - var mapper = (RequestMapper<CommunicationTask<S, RESPONSE>, T>) handler.getRequestMapper(); - return mapper.map(this); + var mapper = handler.getRequestMapper(); + return requestType.cast(((RequestMapper<CommunicationTask<S, RESPONSE>, ?>) mapper).map(this));If you choose the refactor, I can help generate call-site changes across tasks.
145-152: Note: iteration safety depends on callbacks list fixOnce callbackList uses CopyOnWriteArrayList, these loops are safe even if callbacks are added concurrently. Otherwise, they risk CME.
Also applies to: 155-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java(7 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/RequestResult.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/CancelReservationTask.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeAvailabilityTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearChargingProfileTask.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetCompositeScheduleTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetConfigurationTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetLocalListVersionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ReserveNowTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskAdhoc.java(0 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java(0 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/UpdateFirmwareTask.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/OcppVersionHandler.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/RequestMapper.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/ResponseMapper.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/TaskDefinition.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargePoint.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java(2 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargePointMapper.java(2 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/TransactionMapper.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java(1 hunks)steve-ocpp-transport-soap/src/main/java/de/rwth/idsg/steve/ocpp/soap/ChargePointServiceSoapInvoker.java(2 hunks)steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/config/OcppConfiguration.java(1 hunks)steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java(1 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java(1 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-cp-misc.jsp(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/chargepointDetails.jsp(1 hunks)steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java(2 hunks)
💤 Files with no reviewable changes (2)
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskAdhoc.java
🚧 Files skipped from review as they are similar to previous changes (7)
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/TaskDefinition.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/ResponseMapper.java
- steve-ocpp-transport-soap/src/main/java/de/rwth/idsg/steve/ocpp/soap/ChargePointServiceSoapInvoker.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/impl/RequestMapper.java
- steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/TriggerMessageTask.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ReserveNowTask.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-05T13:09:02.595Z
Learnt from: juherr
PR: juherr/evolve#1
File: src/main/resources/OCPP-2.0.1_part3_JSON_schemas/ClearCacheRequest.json:21-27
Timestamp: 2025-04-05T13:09:02.595Z
Learning: In the OCPP (Open Charge Point Protocol) standard, the ClearCacheRequest schema is correctly designed with only the optional customData property, as this operation doesn't require additional parameters.
Applied to files:
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java
🪛 GitHub Actions: build and run tests
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/TransactionMapper.java
[warning] 58-58: NullAway: returning @nullable expression from method with @nonnull return type
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java
[warning] 61-61: MissingSummary: A summary line is required on public/protected Javadocs.
[warning] 177-177: JavaTimeDefaultTimeZone: LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
[warning] 180-180: JavaTimeDefaultTimeZone: LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
[warning] 183-183: JavaTimeDefaultTimeZone: LocalDate.now() is not allowed because it silently uses the system default time-zone. You must pass an explicit time-zone (e.g., ZoneId.of("America/Los_Angeles")) to this method.
[warning] 209-209: NullAway: parameter form is @nonnull, but parameter in superclass method de.rwth.idsg.steve.repository.ChargePointRepository.getChargePointConnectorStatus(...) is @nullable
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java
[warning] 135-135: NullAway: dereferenced expression resultMap.get(chargeBoxId) is @nullable
[warning] 184-184: TypeParameterUnusedInFormals: Declaring a type parameter that is only used in the return type is a misuse of generics.
🔇 Additional comments (18)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (1)
205-206: LGTM: switching from Optional to nullable for mapper interopPassing
ar.orElse(null)matches the new mapper signature.steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-cp-misc.jsp (1)
29-31: LGTM: bind Admin Address directly fromcp.adminAddressUI link logic now matches the DTO change.
steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java (1)
71-75: Validate handler type contract and document DummyResponseFound: CommunicationTask#createHandler is declared as
public <T extends ResponseType> AsyncHandler<T> createHandler(String)(steve-core/.../CommunicationTask.java:194). I could not locate a definitivehandleResponse(...)declaration nor theDummyResponseimplementation in the search results. Before removing the TODO in CommunicationContext (steve-ocpp-transport-websocket/.../CommunicationContext.java:71-75):
- Confirm the exact handler method signature (should be
void handleResponse(Response<T> res)if usingjavax.xml.ws.AsyncHandler<T>) and thatDummyResponseimplements the matchingResponse<T>for the same T returned by createHandler(...).- If the types don't match, construct/pass a proper
Response<T>(or adapt via a typed factory) instead of relying on erasure-based casts.- Replace the inline TODO with a one-line comment explaining why
DummyResponseis required (whatResponsetype it implements and why a wrapper is needed).steve/src/main/java/de/rwth/idsg/steve/ocpp/ChargePointServiceInvokerImpl.java (1)
73-209: Method refactors to invoke(...): LGTM.Consistent SOAP/JSON dispatch via method references reduces duplication and risk of divergence.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ChangeConfigurationTask.java (1)
57-63: Constructors aligned to TASK_DEFINITION: LGTM.Matches the new architecture and keeps API consistent with other tasks.
steve-ocpp-transport-websocket/src/main/java/de/rwth/idsg/steve/config/OcppConfiguration.java (1)
56-60: Uniform request resolver looks good — getRequest fails fast.
getRequest(OcppVersion) throws UnsupportedOperationException("Operation not supported for version " + version), so task -> task.getRequest(V_X) will fail fast with a clear message.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetCompositeScheduleTask.java (1)
21-44: LGTM! Clean migration to mapper-based architecture.The refactoring successfully migrates from the legacy
Ocpp16AndAboveTaskto the newTaskDefinition-based approach. The version handler properly maps request parameters and returns the response unchanged, which is appropriate for this operation.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java (1)
29-52: LGTM! Comprehensive version coverage with consistent status extraction.The TaskDefinition properly handles all three OCPP versions (1.2, 1.5, 1.6) with consistent status extraction pattern. The implementation correctly passes the transaction ID and extracts the status value from each version-specific response.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetLocalListVersionTask.java (1)
29-43: LGTM! Clean implementation with proper string conversion.The task definition correctly handles both OCPP 1.5 and 1.6 versions. The use of
String.valueOf()for the list version ensures proper null handling and consistent string representation.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SendLocalListTask.java (1)
93-101: Confirm FULL update default when sendEmptyListWhenFull is nullBoolean.FALSE.equals(...) treats null as “send empty list.” Is that intended for backward compatibility? If not, default to false explicitly.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ResetTask.java (1)
29-58: LGTM: clean, versioned mappingConsistent TaskDefinition across V_12/V_15/V_16; status extraction is uniform. Constructors look good.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/DataTransferTask.java (1)
47-67: LGTM: versioned request/response mapping is consistentClear per-version constructors and uniform ResponseWrapper creation.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/ClearCacheTask.java (1)
29-49: LGTM: straightforward, uniform ClearCache mapping across versionsSimple request creation and status mapping; constructors wired correctly.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java (2)
29-55: Confirm omission of optional ChargingProfile (OCPP 1.6)If RemoteStartTransactionParams supports a charging profile, it’s not mapped here. Please confirm it’s intentional or add mapping per version.
29-58: LGTM: base mapping and status extraction look correctIdTag/connectorId mapping matches 1.x schemas across versions.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/GetDiagnosticsTask.java (1)
31-69: Mapper wiring looks clean and version-accurate.Good consolidation into a single TaskDefinition with filename mapping per version.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (2)
57-58: Good: decoupling via TaskDefinitionInjecting TaskDefinition<S, RESPONSE> is a solid step to centralize version mapping and reduce subclass surface.
94-99: No change required — project targets Java 21; HashMap.newHashMap is allowed.pom.xml sets <java.version>21 and the maven-compiler-plugin uses ${java.version}>, so HashMap.newHashMap(int) is supported; keep as-is.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java
Show resolved
Hide resolved
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargePointMapper.java
Show resolved
Hide resolved
1 participant
This commit refactors the OCPP task management system to be more generic and maintainable. The previous implementation had a rigid class hierarchy and coupled the task logic directly to specific OCPP versions, making it difficult to add new tasks or support new protocol versions.
The new architecture introduces a mapper-based approach to decouple the core task logic from the version-specific OCPP message formats.
Key changes:
TaskDefinition: A new interface to define the parameters of a task.RequestMapperandResponseMapper: Interfaces for mapping between the genericTaskDefinitionand the version-specific OCPP request/response objects.OcppVersionHandler: A new interface that groups theRequestMapperandResponseMapperfor a specific OCPP version. This was implemented based on user feedback to improve the design.CommunicationTask: The base task class has been refactored to use the new mapper-based system, removing the need for version-specific abstract methods.ResetTask,SetChargingProfileTask, etc.) have been updated to use the new architecture.ChargePointServiceInvokerImpl: The invoker has been updated to work with the new generic task system.This refactoring significantly improves the extensibility of the task management system, reduces code duplication, and makes it easier to add support for future OCPP versions or custom tasks.
PR created automatically by Jules for task 10686966425854007946
Summary by CodeRabbit
New Features
Refactor
Documentation