Conversation
…and UnlockConnector
…geBoxId, Integer connectorId) and getOcppTagOfTransaction(Integer transaction_pk);
…tor for RemoteStart, RemoteStop and UnlockConnector
…tation o ChargeBoxPk;
…JasonIgnor annotation to ChargeBoxPk;
…ere is already a active transaction, methode postUnlockCon checks no that there is no active transaction, removed the ocpp tag check; Added comments
…TaskRestController class
…eferZero and PreferOthersWithStatusOfZero
…p_calling_logic # resolved Conflicts: # src/main/java/de/rwth/idsg/steve/service/ChargePointService12_Client.java # src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWriteEnum.java
…nd equal-sign(TaskRestController)
…and unlock for differentiation of internal and external tasks
* refs/heads/upstream: (31 commits)
fix: update after Copilot review
fix: json array elements may have different sort
fix: update licence
fix: update after CodeRabbit review
Bump com.zaxxer:HikariCP from 6.2.1 to 7.0.1
Bump org.apache.httpcomponents.client5:httpclient5 from 5.4.2 to 5.5
Bump actions/checkout from 4 to 5
retire ubuntu 20, use ubuntu 24
Bump plugin.license-maven.version from 4.6 to 5.0.0
Bump flyway.version from 11.4.0 to 11.5.0
Bump spring.security.version from 6.4.3 to 6.4.4
Bump jetty.version from 12.0.17 to 12.0.18
add website to set and change WebUser API password
Improvements on webUser password change. Only own password can be changed
WebUserForm.java: pwError changed from AssertTrue to AssertFalse
resolve style checks
GenericRepositoryImpl: re-add 'import org.jooq.impl.DSL;' (wrong decision on reslove conflict)
WebUserController removed password comparison; webuserAdd.jsp, webuserPassword.jsp removed error-message on pwError
BugFix: Set the ".requestMatchers(prefix + "/**").hasAuthority("ADMIN")" at the end of the SecurityFilterChain. Otherwise a USER get's an Error 500 on login
Adding Enum WebUserAuthority and adapt the code using it
...
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds caller-aware task constructors and service client overloads; extends repositories with single-item queries; introduces REST controllers for connectors, remote start/stop/unlock, and task overview; enhances multiple DTOs with OpenAPI/JSON annotations; updates JSP to include a new strategy filter; adjusts a config property. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant RemoteAPI as RemoteStartStopRestController
participant RepoCP as ChargePointRepository
participant RepoTx as TransactionRepository
participant Service as ChargePointServiceClient
participant Task as Remote*Task
participant Bg as BackgroundService
Client->>RemoteAPI: POST /api/v1/remote/start {chargeBoxId, connectorId, ocppTag}
RemoteAPI->>RepoTx: getActiveTransactionId(chargeBoxId, connectorId)
RepoTx-->>RemoteAPI: null or txId
alt no active tx
RemoteAPI->>RepoCP: getChargePointSelect(chargeBoxId)
RepoCP-->>RemoteAPI: List<ChargePointSelect>
RemoteAPI->>Service: remoteStartTransaction(params, caller)
Service->>Task: new RemoteStartTransactionTask(params, caller)
Service->>Bg: forFirst(task, callbacks)
Bg-->>Service: taskId
Service-->>RemoteAPI: taskId
RemoteAPI-->>Client: 200 {taskId}
else active tx exists
RemoteAPI-->>Client: 400 BadRequest
end
sequenceDiagram
participant Client as API Client
participant ConnectorAPI as ConnectorRestController
participant Helper as ChargePointHelperService
participant Repo as ChargePointRepository
Client->>ConnectorAPI: GET /api/v1/connectors?chargeBoxId=&status=&strategy=
ConnectorAPI->>Repo: getChargeBoxIds()
Repo-->>ConnectorAPI: List<String>
ConnectorAPI->>Helper: getChargePointConnectorStatus(queryParams)
Helper-->>ConnectorAPI: List<ConnectorStatus>
alt strategy = PreferZero
ConnectorAPI->>ConnectorAPI: filterAndPreferZero()
else
ConnectorAPI->>ConnectorAPI: filterAndPreferOthersWithStatusOfZero()
end
ConnectorAPI-->>Client: 200 ApiConnectorList
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (38)
src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWriteEnum.java (2)
28-31: Consider documenting the enum in OpenAPI for clearer API docsGiven the PR adds OpenAPI/JSON annotations across DTOs, annotating this enum will improve generated docs and client UX. Optional, non-functional.
Apply this diff to add a schema annotation:
public enum ConfigurationKeyReadWriteEnum { - R, - RW + R, + RW }And annotate the type (placed just above the enum):
+@io.swagger.v3.oas.annotations.media.Schema( + description = "Access mode of a configuration key", + example = "RW" +) public enum ConfigurationKeyReadWriteEnum {If you prefer explicit imports, add:
import io.swagger.v3.oas.annotations.media.Schema;and use @Schema instead of the FQN.
28-31: Optional refactor: align enum naming by dropping the “Enum” suffixBased on a quick survey (35 enums total, only 4 use the “Enum” suffix), we generally avoid appending “Enum” to enum types. If you pick up this refactor, consider renaming and updating references accordingly (this is API-breaking):
• File to update:
src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWriteEnum.java
• Suggested new name: ConfigurationKeyAccessExample diff:
- public enum ConfigurationKeyReadWriteEnum { + public enum ConfigurationKeyAccess { R, RW }src/main/resources/config/dev/main.properties (1)
24-24: Use a clear dev-only placeholder for webapi.valueA static value like “1234” in
src/main/resources/config/dev/main.propertiescan slip into production. Replace it with an unmistakable placeholder (or an env-var) and document how to override locally.
- File: src/main/resources/config/dev/main.properties, line 24
- Change:
- webapi.value = 1234 + webapi.value = DEV_API_VALUE_UNSET # dev only; override via env or untracked local filesrc/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java (1)
34-40: Polish OpenAPI descriptions and enumerate allowed strategy values.Align wording with other DTOs and surface the allowed enum values/default in the schema to help clients.
- @Schema(description = "Charge Box Id") + @Schema(description = "Charge Box ID") private String chargeBoxId; - @Schema(description = "Connector Status") + @Schema(description = "Connector status") private String status; - @Schema(description = "Strategy of listing the connector") + @Schema( + description = "Strategy for listing the connector", + allowableValues = {"PreferZero", "PreferOthersWithStatusOfZero"}, + defaultValue = "PreferZero" + ) private Strategy strategy = Strategy.PreferZero;src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointStart.java (2)
21-25: @requiredargsconstructor is superfluous here.There are no final or @nonnull fields; Lombok will generate a public no-args ctor anyway. Removing the annotation (and its import) reduces noise.
-import lombok.RequiredArgsConstructor; +import lombok.NoArgsConstructor; @@ -@RequiredArgsConstructor +@NoArgsConstructor public class ApiChargePointStart {
35-41: Harmonize OCPP tag naming and declare required fields on the Remote Start DTOTo keep our JSON contract consistent and ensure the Remote Start endpoint validates its inputs, please:
In
ApiChargePointStart.java
• Rename fieldocppTag→ocppIdTag(update getter/setter and JSON property)
• Add@Schema(description = "OCPP tag identifier", requiredMode = RequiredMode.REQUIRED)
• Add a validation annotation (@NotBlankor@NotNull)In
RemoteStartStopRestController.java
• Change all uses ofparams.getOcppTag()→params.getOcppIdTag()
• Update the error message to reference “OCPP ID Tag”In tests (
TransactionRestControllerTest.java)
• Update JSON parameters and expectations to useocppIdTagIn JSP views under
src/main/resources/webapp/WEB-INF/views/data-man/
• Replace any references toocppTagwithocppIdTag(e.g. query parameters, form paths)Optionally review other DTOs and query-form classes to ensure any required properties are marked with
requiredModeor validation annotationsThis will align naming with the rest of the API (which predominantly uses
ocppIdTag) and make the Remote Start contract explicit.src/main/java/de/rwth/idsg/steve/repository/dto/TaskOverview.java (1)
36-47: Minor wording fixes in Schema descriptions.Tighten phrasing; improves generated docs without behavior changes.
- @Schema(description = "Starttime") + @Schema(description = "Start time") private final DateTime start; - @Schema(description = "Endtime") + @Schema(description = "End time") private final DateTime end; - @Schema(description = "Task triggered internal or external") + @Schema(description = "Task origin (internal or external)") private final TaskOrigin origin;src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java (2)
40-47: Unify wording/casing in schema descriptions.Align with other DTOs and common acronyms.
- @Schema(description = "Charge Box Id") + @Schema(description = "Charge Box ID") private final String chargeBoxId; - @Schema(description = "Connected since") + @Schema(description = "Connected since") private final String connectedSince; - @Schema(description = "Duration of the Connection") + @Schema(description = "Duration of the connection") private final String connectionDuration; - @Schema(description = "Ocpp version") + @Schema(description = "OCPP version") private final OcppVersion version;
41-45: Align DTO with machine-readable timestamp
The builder already populates both the humanized string (connectedSince) and the rawDateTime(connectedSinceDT), but the schema hides the machine-readable field and exposes only the formatted string. For consistency with other DTOs, swap their visibility:• In
src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java(around lines 41–45):
- Annotate the humanized
connectedSinceas hidden:- @Schema(description = "Connected since") - private final String connectedSince; + @JsonIgnore + @Schema(hidden = true) + private final String connectedSince;- Expose the raw
connectedSinceDTin the OpenAPI schema:- @Schema(description = "Connected since as DT", hidden = true) + @Schema(description = "Connected since (ISO-8601 timestamp)") private final DateTime connectedSinceDT;This will serialize a proper ISO timestamp in the JSON payload and hide the formatted string. If backward compatibility is a concern, you can introduce a new field instead of immediately removing the old one.
src/main/java/de/rwth/idsg/steve/web/controller/HomeController.java (1)
81-86: Handle null or future strategy values gracefullyIf params.getStrategy() is null (e.g., older clients) you currently default to PreferOthersWithStatusOfZero. Consider defaulting to PreferZero when strategy is null to preserve prior behavior and make this more future-proof.
Apply this diff:
- List<ConnectorStatus> filteredList; - if (params.getStrategy() == ConnectorStatusForm.Strategy.PreferZero) { - filteredList = ConnectorStatusFilter.filterAndPreferZero(latestList); - } else { - filteredList = ConnectorStatusFilter.filterAndPreferOthersWithStatusOfZero(latestList); - } + List<ConnectorStatus> filteredList; + ConnectorStatusForm.Strategy strategy = params.getStrategy(); + if (strategy == null || strategy == ConnectorStatusForm.Strategy.PreferZero) { + filteredList = ConnectorStatusFilter.filterAndPreferZero(latestList); + } else if (strategy == ConnectorStatusForm.Strategy.PreferOthersWithStatusOfZero) { + filteredList = ConnectorStatusFilter.filterAndPreferOthersWithStatusOfZero(latestList); + } else { + // Default for any future/unknown enum values + filteredList = ConnectorStatusFilter.filterAndPreferZero(latestList); + }src/main/java/de/rwth/idsg/steve/repository/ChargePointRepository.java (1)
48-49: Fix parameter name typo for consistency and readabilityUse chargeBoxId consistently across the codebase (camelCase, lowercase “d”). Current name chageBoxID has a typo and inconsistent casing.
Apply this diff:
- List<ChargePointSelect> getChargePointSelect(String chageBoxID); + List<ChargePointSelect> getChargePointSelect(String chargeBoxId);Optional: If this method is intended to return at most one entry, consider documenting that or exposing Optional for clarity in a follow-up change.
src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java (1)
76-79: Default null caller to a sensible value to avoid null propagationExternal integrations may omit caller; defaulting prevents null in logs/metrics and keeps semantics clear.
Apply this diff:
- public CommunicationTask(S params, String caller) { - this(params, TaskOrigin.EXTERNAL, caller); - } + public CommunicationTask(S params, String caller) { + this(params, TaskOrigin.EXTERNAL, caller == null ? "External" : caller); + }src/main/java/de/rwth/idsg/steve/repository/TransactionRepository.java (2)
33-34: Interface addition looks fine; consider explicit nullability contract
getTransaction(int transactionPk)returnsTransaction(notOptional). If the implementation may return null, please document it, or returnOptional<Transaction>to make absence explicit.Would you like me to switch this to
Optional<Transaction>across interface + impl + call sites in this PR?
40-40: Prefer primitive connectorId to prevent nulls and clarify contractIf
connectorIdmust be provided, useintinstead ofIntegerat the interface level.Apply this change here:
- Integer getActiveTransactionId(String chargeBoxId, Integer connectorId); + Integer getActiveTransactionId(String chargeBoxId, int connectorId);Note: The impl already treats it as required; changing to primitive aligns types and avoids accidental nulls.
src/main/resources/webapp/WEB-INF/views/connectorStatus.jsp (1)
54-61: Label mismatch: change second “Status” label to “Strategy”Prevents UI confusion with two “Status” rows.
Apply this diff:
- <tr> - <td>Status:</td> + <tr> + <td>Strategy:</td> <td><form:select path="strategy"> <option value="PreferZero" selected >Prefer Connector Zero</option> <option value="PreferOthersWithStatusOfZero">Prefer Others</option> </form:select> </td> </tr>src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
101-112: Limit to 1 and fetch deterministically; align param type with interface
- Add
.limit(1)and usefetchOne(...)to both express intent and avoid scanning extra rows.- If you change the interface to
int connectorId, mirror it here.Apply this diff:
- public Integer getActiveTransactionId(String chargeBoxId, Integer connectorId) { + public Integer getActiveTransactionId(String chargeBoxId, int connectorId) { return ctx.select(TRANSACTION.TRANSACTION_PK) .from(TRANSACTION) .join(CONNECTOR) .on(TRANSACTION.CONNECTOR_PK.equal(CONNECTOR.CONNECTOR_PK)) .and(CONNECTOR.CHARGE_BOX_ID.equal(chargeBoxId)) .where(TRANSACTION.STOP_TIMESTAMP.isNull()) .and(CONNECTOR.CONNECTOR_ID.equal(connectorId)) - .orderBy(TRANSACTION.TRANSACTION_PK.desc()) // to avoid fetching ghost transactions, fetch the latest - .fetchAny(TRANSACTION.TRANSACTION_PK); + .orderBy(TRANSACTION.TRANSACTION_PK.desc()) // to avoid fetching ghost transactions, fetch the latest + .limit(1) + .fetchOne(TRANSACTION.TRANSACTION_PK); }src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskInfo.java (3)
62-66: Avoid exposing AtomicInteger in API DTOUsing AtomicInteger in a web DTO leaks concurrency primitives into the API surface and creates mutable references to the task’s internal counters. Prefer plain int and copy the value.
Change the field types:
- @Schema(description = "Error count") - private AtomicInteger errorCount = new AtomicInteger(0); - @Schema(description = "Response count") - private AtomicInteger responseCount = new AtomicInteger(0); + @Schema(description = "Error count") + private int errorCount; + @Schema(description = "Response count") + private int responseCount;
82-84: Copy counter values instead of sharing AtomicInteger referencesComplete the refactor by assigning scalar values.
- this.errorCount = r.getErrorCount(); - this.responseCount = r.getResponseCount(); + this.errorCount = r.getErrorCount().get(); + this.responseCount = r.getResponseCount().get();
57-60: Remove redundant default for startTimestamp or make it a fallbackThe default initialization to DateTime.now() is immediately overwritten. Either remove it or keep it as a fallback if the task’s start is null. Suggesting the fallback for robustness.
- @Schema(description = "Starttime") - private DateTime startTimestamp = DateTime.now(); + @Schema(description = "Starttime") + private DateTime startTimestamp; @@ - this.startTimestamp = r.getStartTimestamp(); + this.startTimestamp = (r.getStartTimestamp() != null) ? r.getStartTimestamp() : DateTime.now();Also applies to: 79-81
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointList.java (2)
45-57: Make inner DTO static to avoid synthetic outer reference and serialization pitfallsNon-static inner classes retain an implicit reference to the outer instance, which is unnecessary here and can complicate Jackson serialization. Make it static (and public if you expect reuse).
- class ChargePointInfo { + public static class ChargePointInfo {
35-36: @requiredargsconstructor is redundant hereThere are no final or @nonnull fields requiring constructor generation in ApiChargePointList. Consider removing the annotation to reduce noise.
src/main/java/de/rwth/idsg/steve/web/api/TaskRestController.java (2)
51-55: Prefer constructor injection over field injectionYou already have @requiredargsconstructor; switch TaskStore to a final field and remove @Autowired for consistency and testability.
-@RequiredArgsConstructor +@RequiredArgsConstructor public class TaskRestController { - @Autowired private TaskStore taskStore; + private final TaskStore taskStore;
70-81: Reduce duplication of identical ApiResponses annotationsThe same response contract is repeated three times. Consider extracting a composed annotation or a meta-annotation to keep it DRY.
Also applies to: 89-100, 108-119
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiConnectorList.java (3)
46-47: Defensively expose an unmodifiable statusFilterValuesIf ALL_STATUS_VALUES is mutable, external callers could accidentally change it via this DTO. Wrap it to ensure immutability.
- private final Set<String> statusFilterValues = ConnectorStatusCountFilter.ALL_STATUS_VALUES; + private final Set<String> statusFilterValues = Set.copyOf(ConnectorStatusCountFilter.ALL_STATUS_VALUES);
48-50: Boolean naming: prefer primitive boolean and 'filtered'Boolean isFiltered results in a getter named getIsFiltered(), which is a bit awkward. Consider using a primitive boolean named filtered for cleaner bean/JSON properties.
21-22: Remove commented-out importThe commented import clutters the file. Safe to remove.
-//import de.rwth.idsg.steve.ocpp.OcppTransport;src/main/java/de/rwth/idsg/steve/web/api/ConnectorRestController.java (5)
95-95: Fix typo and simplify filter helper (rename isFilterd -> isFiltered).Spelling fix improves readability; simplifying the helper reduces nesting.
Apply:
- conList.setIsFiltered(isFilterd(queryParams)); + conList.setIsFiltered(isFiltered(queryParams));And replace the helper:
- private Boolean isFilterd(ConnectorStatusForm queryParams) { - if (!isNull(queryParams)) { - if (!isNull(queryParams.getChargeBoxId())) { - if (!queryParams.getChargeBoxId().isBlank()) { - return true; - } - } - if (!isNull(queryParams.getStatus())) { - if (!queryParams.getStatus().isBlank()) { - return true; - } - } - } - return false; - } + private boolean isFiltered(ConnectorStatusForm queryParams) { + return queryParams != null + && ( + (queryParams.getChargeBoxId() != null && !queryParams.getChargeBoxId().isBlank()) + || (queryParams.getStatus() != null && !queryParams.getStatus().isBlank()) + ); + }Also applies to: 129-143
70-72: Prefer constructor injection (remove field injection and @Autowired import).You already use @requiredargsconstructor; make dependencies final and drop @Autowired for consistency and testability.
-import org.springframework.beans.factory.annotation.Autowired; +// import removed- @Autowired private ChargePointRepository chargePointRepository; - @Autowired private ChargePointHelperService chargePointHelperService; + private final ChargePointRepository chargePointRepository; + private final ChargePointHelperService chargePointHelperService;Also applies to: 32-32
90-91: Remove redundant @responsebody on @RestController methods.@RestController already implies @responsebody.
- @GetMapping(value = "") - @ResponseBody + @GetMapping(value = "")- @GetMapping(value = "OCPP_JSON_STATUS") - @ResponseBody + @GetMapping(value = "OCPP_JSON_STATUS")Also applies to: 120-121
98-102: Make default strategy explicit for readability.Currently null strategy falls into the else-branch implicitly. Consider deriving a local strategy with an explicit default.
Example:
ConnectorStatusForm.Strategy strategy = queryParams.getStrategy() == null ? ConnectorStatusForm.Strategy.PreferOthersWithStatusOfZero : queryParams.getStrategy();
51-56: Remove commented-out imports.Dead/commented imports add noise and risk drifting from the project’s import conventions.
-//import org.springframework.web.bind.annotation.DeleteMapping; - -//import org.springframework.web.bind.annotation.PathVariable; -//import org.springframework.web.bind.annotation.PostMapping; -//import org.springframework.web.bind.annotation.PutMapping;src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (1)
211-213: Annotate private varargs helpers with @SafeVarargs.Prevents unchecked varargs warnings; allowed on private methods.
- private int addRemoteStartTask(RemoteStartTransactionTask task, - OcppCallback<String>... callbacks) { + @SafeVarargs + private int addRemoteStartTask(RemoteStartTransactionTask task, + OcppCallback<String>... callbacks) {- private int addRemoteStopTask(RemoteStopTransactionTask task, - OcppCallback<String>... callbacks) { + @SafeVarargs + private int addRemoteStopTask(RemoteStopTransactionTask task, + OcppCallback<String>... callbacks) {- private int addUnlockConnectorTask(UnlockConnectorTask task, - OcppCallback<String>... callbacks) { + @SafeVarargs + private int addUnlockConnectorTask(UnlockConnectorTask task, + OcppCallback<String>... callbacks) {Also applies to: 238-240, 265-267
src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java (5)
74-81: Prefer constructor injection (remove field injection and @Autowired import).Align with @requiredargsconstructor by making fields final and removing @Autowired.
-import org.springframework.beans.factory.annotation.Autowired; +// import removed- @Autowired private ChargePointHelperService chargePointHelperService; - @Autowired private ChargePointRepository chargePointRepository; - @Autowired private TransactionRepository transactionRepository; - @Autowired private TaskStore taskStore; + private final ChargePointHelperService chargePointHelperService; + private final ChargePointRepository chargePointRepository; + private final TransactionRepository transactionRepository; + private final TaskStore taskStore;- @Autowired - private ChargePointServiceClient client; + private final ChargePointServiceClient client;Also applies to: 37-37
289-290: Use shared caller constant.Keep caller identity consistent and maintainable.
- return client.remoteStopTransaction(transactionParams, "SteveWebApi"); + return client.remoteStopTransaction(transactionParams, CALLER);
136-140: Remove redundant @responsebody on @RestController methods.@ResponeBody is implicit; removing it reduces noise. No behavior change.
Also applies to: 154-158, 172-176, 190-194, 213-216, 255-258, 304-307
198-200: Fix typos in comments.Minor spelling fixes improve clarity.
- // the methods return the taskID, check the sucess with the TaskRestController + // the methods return the taskID, check the success with the TaskRestController- // Check for acctive transactions on the connector, If a active transaction is found, don't send RemoteStart. + // Check for active transactions on the connector. If an active transaction is found, don't send RemoteStart.- // set the ChargPointSelectionList, maybe check nessesary that length is one + // Set the ChargePointSelect list; ensure the length is exactly oneAlso applies to: 224-224, 267-269
105-118: Note: race window in active task check.Between checking and scheduling, another task might be created. If strict exclusion is required, consider guarding in TaskStore or service layer.
Would you like a follow-up patch that adds an atomic reservation in TaskStore to enforce single-task-per-chargebox semantics?
src/main/java/de/rwth/idsg/steve/repository/dto/ConnectorStatus.java (1)
37-41: Optional: @JsonIgnore plus @Schema(hidden = true) is redundant.Either one suffices to hide from API consumers; keeping both is harmless but slightly noisy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
src/main/java/de/rwth/idsg/steve/ocpp/CommunicationTask.java(1 hunks)src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java(1 hunks)src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java(1 hunks)src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java(1 hunks)src/main/java/de/rwth/idsg/steve/repository/ChargePointRepository.java(1 hunks)src/main/java/de/rwth/idsg/steve/repository/TransactionRepository.java(1 hunks)src/main/java/de/rwth/idsg/steve/repository/dto/ConnectorStatus.java(2 hunks)src/main/java/de/rwth/idsg/steve/repository/dto/TaskOverview.java(2 hunks)src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java(1 hunks)src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java(2 hunks)src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java(3 hunks)src/main/java/de/rwth/idsg/steve/web/api/ConnectorRestController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/TaskRestController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointList.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointStart.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiConnectorList.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskInfo.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskList.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/controller/HomeController.java(1 hunks)src/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java(2 hunks)src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java(2 hunks)src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWriteEnum.java(1 hunks)src/main/resources/config/dev/main.properties(1 hunks)src/main/resources/webapp/WEB-INF/views/connectorStatus.jsp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-05T13:09:46.659Z
Learnt from: juherr
PR: juherr/evolve#1
File: src/main/resources/OCPP-2.0.1_part3_JSON_schemas/GetLocalListVersionRequest.json:1-28
Timestamp: 2025-04-05T13:09:46.659Z
Learning: The JSON schemas for OCPP 2.0.1 are provided by the Open Charge Alliance (OCA) and should not be modified from their official specification.
Applied to files:
src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java
🧬 Code Graph Analysis (9)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskInfo.java (2)
src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java (1)
Getter(33-50)src/main/java/de/rwth/idsg/steve/repository/dto/TaskOverview.java (1)
Getter(32-56)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskList.java (1)
src/main/java/de/rwth/idsg/steve/repository/dto/TaskOverview.java (1)
Getter(32-56)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiConnectorList.java (1)
src/main/java/de/rwth/idsg/steve/repository/dto/ConnectorStatus.java (1)
Getter(34-66)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointStart.java (4)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointList.java (2)
Getter(33-58)Getter(45-57)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiConnectorList.java (1)
Getter(37-53)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskList.java (1)
Getter(33-39)src/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java (1)
Getter(30-45)
src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java (4)
src/main/java/de/rwth/idsg/steve/web/api/exception/BadRequestException.java (1)
BadRequestException(21-26)src/main/java/de/rwth/idsg/steve/repository/dto/Transaction.java (1)
Schema(35-99)src/main/java/de/rwth/idsg/steve/web/api/TaskRestController.java (1)
Slf4j(48-126)src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (1)
Slf4j(67-396)
src/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java (1)
src/main/java/de/rwth/idsg/steve/repository/dto/Transaction.java (1)
Schema(35-99)
src/main/java/de/rwth/idsg/steve/repository/dto/TaskOverview.java (1)
src/main/java/de/rwth/idsg/steve/repository/dto/Transaction.java (1)
Schema(35-99)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointList.java (4)
src/main/java/de/rwth/idsg/steve/web/api/dto/ApiChargePointStart.java (1)
Getter(31-41)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiConnectorList.java (1)
Getter(37-53)src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskList.java (1)
Getter(33-39)src/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java (1)
Getter(30-45)
src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java (1)
src/main/java/de/rwth/idsg/steve/repository/dto/Transaction.java (1)
Schema(35-99)
🪛 GitHub Check: checkstyle
src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java
[failure] 228-228: [checkstyle] src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java#L228 <com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck>
',' is not followed by whitespace.
[failure] 235-235: [checkstyle] src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java#L235 <com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck>
',' is not followed by whitespace.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (ubuntu-22.04, 17, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 17, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 17, mariadb:10.4.30)
🔇 Additional comments (8)
src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWriteEnum.java (1)
1-18: Header/license reformat only — no behavioral changeLooks good. No API or runtime impact.
src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStopTransactionTask.java (1)
37-39: LGTM: caller-aware constructor correctly delegates to base classThe overload is consistent with the new CommunicationTask API and keeps the original constructor intact for backward compatibility.
src/main/java/de/rwth/idsg/steve/ocpp/task/UnlockConnectorTask.java (1)
37-39: LGTM: added overload mirrors other tasks and maintains compatibilityConstructor correctly forwards caller context to the base class.
src/main/java/de/rwth/idsg/steve/ocpp/task/RemoteStartTransactionTask.java (1)
37-39: LGTM: caller-aware constructor is consistent and correctly delegates to superThe overload mirrors the pattern used across tasks and cleanly propagates caller context.
src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
69-76: I’ve confirmed thatTransactionQueryForm.periodTypeis initialized toALL(and its getter defaults toALLif somehow null), so there’s no NPE risk inprocessType. The only remaining suggestion is to usefetchOne(...)instead offetchAny(...)for clarity when querying by primary key.Change
fetchAny→fetchOnefor clearer intent
fetchOne(...)better expresses “at most one result” when querying by PK.• File:
src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.javapublic Transaction getTransaction(int transactionPk) { TransactionQueryForm form = new TransactionQueryForm(); form.setTransactionPk(transactionPk); form.setReturnCSV(false); form.setType(TransactionQueryForm.QueryType.ALL); - return getInternal(form).fetchAny(new TransactionMapper()); + return getInternal(form).fetchOne(new TransactionMapper()); }src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskList.java (1)
33-39: LGTM as a thin wrapper DTOThe DTO is minimal and fits its usage in TaskRestController. No issues found.
src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (1)
199-203: Good centralization via helpers; overloads with caller look consistent.The new overloads delegate cleanly to shared helpers and keep forFirst semantics intact. Nice consolidation.
Also applies to: 205-209, 252-256, 259-263
src/main/java/de/rwth/idsg/steve/repository/dto/ConnectorStatus.java (1)
37-66: DTO enhancements look good; field-level docs and new timestamp are clear.The addition of statusTimestamp alongside the humanized timeStamp is useful, and hiding chargeBoxPk from API is appropriate.
| @Override // returns List of zero or one ChargeBox | ||
| public List<ChargePointSelect> getChargePointSelect(String chageBoxID) { | ||
| return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS, CHARGE_BOX.OCPP_PROTOCOL) | ||
| .from(CHARGE_BOX) | ||
| .where(CHARGE_BOX.CHARGE_BOX_ID.eq(chageBoxID)) | ||
| .fetch() | ||
| .map(r -> new ChargePointSelect(OcppProtocol.fromCompositeValue(r.value3()), | ||
| r.value1(), r.value2())); | ||
| } |
There was a problem hiding this comment.
Fix potential NPE when OCPP_PROTOCOL is null; also fix parameter typo
- If
CHARGE_BOX.OCPP_PROTOCOLis null,OcppProtocol.fromCompositeValue(...)can NPE. Guard it (consistent with usage elsewhere in this class). - Rename
chageBoxID→chargeBoxId(typo).
Apply this diff:
- @Override // returns List of zero or one ChargeBox
- public List<ChargePointSelect> getChargePointSelect(String chageBoxID) {
- return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS, CHARGE_BOX.OCPP_PROTOCOL)
- .from(CHARGE_BOX)
- .where(CHARGE_BOX.CHARGE_BOX_ID.eq(chageBoxID))
- .fetch()
- .map(r -> new ChargePointSelect(OcppProtocol.fromCompositeValue(r.value3()),
- r.value1(), r.value2()));
- }
+ @Override // returns List of zero or one ChargeBox
+ public List<ChargePointSelect> getChargePointSelect(String chargeBoxId) {
+ return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS, CHARGE_BOX.OCPP_PROTOCOL)
+ .from(CHARGE_BOX)
+ .where(CHARGE_BOX.CHARGE_BOX_ID.eq(chargeBoxId))
+ .fetch()
+ .map(r -> new ChargePointSelect(
+ r.value3() == null ? null : OcppProtocol.fromCompositeValue(r.value3()),
+ r.value1(),
+ r.value2()
+ ));
+ }Optional: consider .and(CHARGE_BOX.ENDPOINT_ADDRESS.isNotNull()) if callers always require a usable endpoint.
📝 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 // returns List of zero or one ChargeBox | |
| public List<ChargePointSelect> getChargePointSelect(String chageBoxID) { | |
| return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS, CHARGE_BOX.OCPP_PROTOCOL) | |
| .from(CHARGE_BOX) | |
| .where(CHARGE_BOX.CHARGE_BOX_ID.eq(chageBoxID)) | |
| .fetch() | |
| .map(r -> new ChargePointSelect(OcppProtocol.fromCompositeValue(r.value3()), | |
| r.value1(), r.value2())); | |
| } | |
| @Override // returns List of zero or one ChargeBox | |
| public List<ChargePointSelect> getChargePointSelect(String chargeBoxId) { | |
| return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS, CHARGE_BOX.OCPP_PROTOCOL) | |
| .from(CHARGE_BOX) | |
| .where(CHARGE_BOX.CHARGE_BOX_ID.eq(chargeBoxId)) | |
| .fetch() | |
| .map(r -> new ChargePointSelect( | |
| r.value3() == null ? null : OcppProtocol.fromCompositeValue(r.value3()), | |
| r.value1(), | |
| r.value2() | |
| )); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java
around lines 106 to 114, the method parameter name has a typo and the mapping
can NPE when OCPP_PROTOCOL is null; rename parameter chageBoxID to chargeBoxId
and, in the fetch().map(...) lambda, guard the protocol conversion by checking
r.value3() for null (use the class's existing pattern: pass null to
OcppProtocol.fromCompositeValue only if non-null or use a conditional to set
protocol to null), then construct new ChargePointSelect with the safely-resolved
protocol and the two values; optionally add
.and(CHARGE_BOX.ENDPOINT_ADDRESS.isNotNull()) to the where clause if callers
require a non-null endpoint.
| public final int remoteStopTransaction(RemoteStopTransactionParams params, | ||
| OcppCallback<String>... callbacks) { | ||
| RemoteStopTransactionTask task = new RemoteStopTransactionTask(params); | ||
| return addRemoteStopTask(task,callbacks); |
There was a problem hiding this comment.
Fix Checkstyle: add whitespace after comma.
Static analysis flagged missing whitespace after ','.
- return addRemoteStopTask(task,callbacks);
+ return addRemoteStopTask(task, callbacks);- return addRemoteStopTask(task,callbacks);
+ return addRemoteStopTask(task, callbacks);Also applies to: 235-235
🧰 Tools
🪛 GitHub Check: checkstyle
[failure] 228-228: [checkstyle] src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java#L228 <com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck>
',' is not followed by whitespace.
🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java around
lines 228 and 235 the method call arguments lack a space after the comma (e.g.
"task,callbacks"); update the call(s) to insert a single space after each comma
so they read "task, callbacks" (and similarly for any other comma-separated
arguments on the noted lines) to satisfy Checkstyle.
| //this.ocppVersion = r.getOcppVersion(); | ||
| this.operationName = r.getOperationName(); | ||
| this.origin = r.getOrigin(); | ||
| this.caller = r.getCaller(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Populate ocppVersion; it's currently always null
The DTO exposes ocppVersion but never assigns it. Given CommunicationTask exposes getOcppVersion(), this looks unintended and will result in nulls in the API.
Apply this diff:
- //this.ocppVersion = r.getOcppVersion();
+ this.ocppVersion = r.getOcppVersion();📝 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.
| //this.ocppVersion = r.getOcppVersion(); | |
| this.operationName = r.getOperationName(); | |
| this.origin = r.getOrigin(); | |
| this.caller = r.getCaller(); | |
| this.ocppVersion = r.getOcppVersion(); | |
| this.operationName = r.getOperationName(); | |
| this.origin = r.getOrigin(); | |
| this.caller = r.getCaller(); |
🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/dto/ApiTaskInfo.java around lines 70
to 74, the ocppVersion field on the DTO is never set (the assignment is
commented out) causing null values in the API; restore or add the assignment
this.ocppVersion = r.getOcppVersion(); (optionally guard with a null check if
you need to map or default missing values) so the DTO reflects the
CommunicationTask's OCPP version.
| try { | ||
| for (ChargePointSelect chargeBox : chargePoints) { | ||
| List<Integer> conList = chargePointRepository.getNonZeroConnectorIds(chargeBox.getChargeBoxId()); | ||
| if (!conList.isEmpty()) { | ||
| lsCp.addCP(chargeBox.getChargeBoxId(), conList); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| System.out.println(e.toString()); | ||
| } | ||
| return lsCp; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging instead of System.out; include stack trace.
Avoids stdout noise and preserves diagnostics.
- try {
- for (ChargePointSelect chargeBox : chargePoints) {
- List<Integer> conList = chargePointRepository.getNonZeroConnectorIds(chargeBox.getChargeBoxId());
- if (!conList.isEmpty()) {
- lsCp.addCP(chargeBox.getChargeBoxId(), conList);
- }
- }
- } catch (Exception e) {
- System.out.println(e.toString());
- }
+ try {
+ for (ChargePointSelect chargeBox : chargePoints) {
+ List<Integer> conList = chargePointRepository.getNonZeroConnectorIds(chargeBox.getChargeBoxId());
+ if (!conList.isEmpty()) {
+ lsCp.addCP(chargeBox.getChargeBoxId(), conList);
+ }
+ }
+ } catch (Exception e) {
+ log.warn("Failed to build charge point list", e);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| for (ChargePointSelect chargeBox : chargePoints) { | |
| List<Integer> conList = chargePointRepository.getNonZeroConnectorIds(chargeBox.getChargeBoxId()); | |
| if (!conList.isEmpty()) { | |
| lsCp.addCP(chargeBox.getChargeBoxId(), conList); | |
| } | |
| } | |
| } catch (Exception e) { | |
| System.out.println(e.toString()); | |
| } | |
| return lsCp; | |
| try { | |
| for (ChargePointSelect chargeBox : chargePoints) { | |
| List<Integer> conList = chargePointRepository.getNonZeroConnectorIds(chargeBox.getChargeBoxId()); | |
| if (!conList.isEmpty()) { | |
| lsCp.addCP(chargeBox.getChargeBoxId(), conList); | |
| } | |
| } | |
| } catch (Exception e) { | |
| log.warn("Failed to build charge point list", e); | |
| } | |
| return lsCp; |
🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java
around lines 92 to 102, replace the System.out.println(e.toString()) in the
catch block with your class logger and log the full exception (including stack
trace). If the class doesn't already have a logger, add a private static final
org.slf4j.Logger log =
org.slf4j.LoggerFactory.getLogger(RemoteStartStopRestController.class) and then
call log.error("Error while building lsCp for charge points", e) (or similar
contextual message) instead of printing to stdout.
| Boolean retValue = false; | ||
| List<TaskOverview> taskList = taskStore.getOverview(); | ||
| for (TaskOverview taOverview : taskList) { | ||
| CommunicationTask task = taskStore.get(taOverview.getTaskId()); | ||
| if (!task.isFinished()) { | ||
| if (!isNull(task.getResultMap().get(chargeBoxId))) { | ||
| retValue = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safe access to task results in activeTaskOnChargeBox.
Guard against a null result map to avoid potential NPEs.
- if (!task.isFinished()) {
- if (!isNull(task.getResultMap().get(chargeBoxId))) {
- retValue = true;
- break;
- }
- }
+ if (!task.isFinished()) {
+ var resultMap = task.getResultMap();
+ if (resultMap != null && resultMap.containsKey(chargeBoxId)) {
+ retValue = true;
+ break;
+ }
+ }📝 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.
| Boolean retValue = false; | |
| List<TaskOverview> taskList = taskStore.getOverview(); | |
| for (TaskOverview taOverview : taskList) { | |
| CommunicationTask task = taskStore.get(taOverview.getTaskId()); | |
| if (!task.isFinished()) { | |
| if (!isNull(task.getResultMap().get(chargeBoxId))) { | |
| retValue = true; | |
| break; | |
| } | |
| } | |
| Boolean retValue = false; | |
| List<TaskOverview> taskList = taskStore.getOverview(); | |
| for (TaskOverview taOverview : taskList) { | |
| CommunicationTask task = taskStore.get(taOverview.getTaskId()); | |
| if (!task.isFinished()) { | |
| var resultMap = task.getResultMap(); | |
| if (resultMap != null && resultMap.containsKey(chargeBoxId)) { | |
| retValue = true; | |
| break; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java
around lines 106 to 115, the loop accesses task.getResultMap().get(chargeBoxId)
without guarding against a null result map which can cause an NPE; update the
check to first verify task.getResultMap() is not null (and optionally that it
contains the chargeBoxId) before calling get(...), and only set retValue = true
and break when the map exists and the retrieved value is non-null.
| RemoteStopTransactionParams transactionParams = new RemoteStopTransactionParams(); | ||
| // set the ChargPointSelectionList, maybe check nessesary that length is one | ||
| transactionParams.setChargePointSelectList(chargePointRepository.getChargePointSelect(params.getChargeBoxId())); | ||
|
|
||
| // Get the transactionId of the active transaction on the connector. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate ChargePoint selection (remote stop).
Mirror the same single-target validation as remote start.
- RemoteStopTransactionParams transactionParams = new RemoteStopTransactionParams();
- // set the ChargPointSelectionList, maybe check nessesary that length is one
- transactionParams.setChargePointSelectList(chargePointRepository.getChargePointSelect(params.getChargeBoxId()));
+ RemoteStopTransactionParams transactionParams = new RemoteStopTransactionParams();
+ List<ChargePointSelect> cpList = chargePointRepository.getChargePointSelect(params.getChargeBoxId());
+ if (cpList.isEmpty()) {
+ throw new BadRequestException("Unknown chargeBoxId: " + params.getChargeBoxId());
+ }
+ if (cpList.size() > 1) {
+ throw new BadRequestException("Ambiguous chargeBoxId: " + params.getChargeBoxId());
+ }
+ transactionParams.setChargePointSelectList(cpList);🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java
around lines 266 to 270, the code sets the ChargePointSelectList for a remote
stop but does not validate that it targets a single charge point; mirror the
remote-start validation by fetching the list, verifying it contains exactly one
entry (and that entry matches the expected connector/chargeBoxId if applicable),
and return/throw the same error response used in remote start when the list is
empty or contains multiple entries; ensure you assign the validated single
selection to transactionParams and do not proceed if validation fails.
| String ocppTag = transactionRepository.getTransaction(transactionId).getOcppIdTag(); | ||
| if (!params.getOcppTag().contentEquals(ocppTag)) { | ||
| throw new BadRequestException("The transaction was authorised with another OCPP Tag!"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid NPE when validating OCPP tag.
Protect against null input.
- if (!params.getOcppTag().contentEquals(ocppTag)) {
+ if (params.getOcppTag() == null || !params.getOcppTag().contentEquals(ocppTag)) {
throw new BadRequestException("The transaction was authorised with another OCPP Tag!");
}📝 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.
| String ocppTag = transactionRepository.getTransaction(transactionId).getOcppIdTag(); | |
| if (!params.getOcppTag().contentEquals(ocppTag)) { | |
| throw new BadRequestException("The transaction was authorised with another OCPP Tag!"); | |
| } | |
| String ocppTag = transactionRepository.getTransaction(transactionId).getOcppIdTag(); | |
| if (params.getOcppTag() == null || !params.getOcppTag().contentEquals(ocppTag)) { | |
| throw new BadRequestException("The transaction was authorised with another OCPP Tag!"); | |
| } |
| UnlockConnectorParams transactionParams = new UnlockConnectorParams(); | ||
| transactionParams.setChargePointSelectList(chargePointRepository.getChargePointSelect(params.getChargeBoxId())); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate ChargePoint selection (unlock) and use caller constant.
Align unlock with the same single-target guard and constant usage.
- UnlockConnectorParams transactionParams = new UnlockConnectorParams();
- transactionParams.setChargePointSelectList(chargePointRepository.getChargePointSelect(params.getChargeBoxId()));
+ UnlockConnectorParams transactionParams = new UnlockConnectorParams();
+ List<ChargePointSelect> cpList = chargePointRepository.getChargePointSelect(params.getChargeBoxId());
+ if (cpList.isEmpty()) {
+ throw new BadRequestException("Unknown chargeBoxId: " + params.getChargeBoxId());
+ }
+ if (cpList.size() > 1) {
+ throw new BadRequestException("Ambiguous chargeBoxId: " + params.getChargeBoxId());
+ }
+ transactionParams.setChargePointSelectList(cpList);- transactionParams.setConnectorId(params.getConnectorId());
- return client.unlockConnector(transactionParams, "SteveWebApi");
+ transactionParams.setConnectorId(params.getConnectorId());
+ return client.unlockConnector(transactionParams, CALLER);Also applies to: 329-331
🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/RemoteStartStopRestController.java
around lines 315-317 (and likewise adjust lines 329-331), the code populates
UnlockConnectorParams with a chargePointSelectList without enforcing the
single-target guard or using the shared caller constant; update the logic to
validate that
chargePointRepository.getChargePointSelect(params.getChargeBoxId()) returns
exactly one ChargePoint (throw or return a meaningful error if not), set the
single selected charge point into UnlockConnectorParams accordingly, and replace
any hard-coded caller strings with the existing shared Caller constant used
elsewhere in the class so unlock behaves the same as other single-target
operations.
| @GetMapping(value = "task") | ||
| @ResponseBody | ||
| public ApiTaskInfo getTaskDetails(@RequestParam(name = "id") @Valid Integer taskId) { | ||
| ApiTaskInfo taskInfo = new ApiTaskInfo(taskId, taskStore.get(taskId)); | ||
| return taskInfo; | ||
| } |
There was a problem hiding this comment.
Handle missing task IDs (return 404) and validate parameter
taskStore.get(taskId) may return null, leading to an NPE and a 500 response. Return 404 instead. Also, apply a minimum constraint to taskId and enable method validation.
Apply these diffs:
- Imports
+import jakarta.validation.constraints.Min;
+import org.springframework.http.HttpStatus;
+import org.springframework.web.server.ResponseStatusException;
+import org.springframework.validation.annotation.Validated;- Class-level annotation
-@RequiredArgsConstructor
+@RequiredArgsConstructor
+@Validated
public class TaskRestController {- Method signature and body
- public ApiTaskInfo getTaskDetails(@RequestParam(name = "id") @Valid Integer taskId) {
- ApiTaskInfo taskInfo = new ApiTaskInfo(taskId, taskStore.get(taskId));
- return taskInfo;
- }
+ public ApiTaskInfo getTaskDetails(@RequestParam(name = "id") @Min(1) Integer taskId) {
+ var task = taskStore.get(taskId);
+ if (task == null) {
+ throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Task not found: " + taskId);
+ }
+ return new ApiTaskInfo(taskId, task);
+ }🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/api/TaskRestController.java around lines
120 to 125, add imports for javax.validation.constraints.Min and
org.springframework.validation.annotation.Validated (and
org.springframework.http.ResponseEntity if not already present), annotate the
controller class with @Validated, change the method to accept
@RequestParam(name="id") @Min(1) Integer taskId (remove @Valid), and update the
method to fetch the task from taskStore, returning ResponseEntity.ok(new
ApiTaskInfo(taskId, task)) when present and ResponseEntity.status(404).build()
(or ResponseEntity.notFound().build()) when the task is null so missing IDs
yield 404 instead of an NPE/500.
| @JsonIgnore | ||
| @Schema(description = "Charge Box Pk", hidden = true) | ||
| private final int chargeBoxPk; | ||
| private final String chargeBoxId, connectedSince; | ||
| @Schema(description = "Charge Box Id") | ||
| private final String chargeBoxId; | ||
| @Schema(description = "Connected since") | ||
| private final String connectedSince; | ||
| @Schema(description = "Duration of the Connection") | ||
| private final String connectionDuration; | ||
| @Schema(description = "Ocpp version") | ||
| private final OcppVersion version; | ||
| @Schema(description = "Connected since as DT", hidden = true) | ||
| private final DateTime connectedSinceDT; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
connectedSinceDT is hidden in OpenAPI but still serialized; add @JsonIgnore for consistency.
Transaction DTO hides UI-only fields with both @JsonIgnore and @Schema(hidden = true). Do the same here to avoid exposing duplicate timestamp data.
- @Schema(description = "Connected since as DT", hidden = true)
+ @JsonIgnore
+ @Schema(description = "Connected since as DT", hidden = true)
private final DateTime connectedSinceDT;🤖 Prompt for AI Agents
In src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java around lines 37
to 49, the connectedSinceDT field is marked @Schema(hidden = true) but not
ignored by Jackson, so it can still be serialized; add @JsonIgnore to the
connectedSinceDT field (above its declaration) to match the other UI-only DTO
fields and keep the existing @Schema(hidden = true) annotation in place.
juherr
force-pushed
the
fnkbsi/ApiRemoteStartStop
branch
from
f995a35 to
585c745
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR rebases steve#1291, adding Web API endpoints for connector status management and remote charge point operations, along with UI improvements for connector status filtering.
Key changes include:
- Addition of Web API endpoints for connector status (JSON), remote start/stop/unlock operations, and task management
- Enhanced connector status page with strategy selector for prioritizing connector status display
- Expanded OpenAPI/Swagger documentation with hidden internal identifiers
- Configuration update to include default API value for development environment
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/resources/webapp/WEB-INF/views/connectorStatus.jsp |
Adds strategy selector dropdown for connector status prioritization |
src/main/resources/config/dev/main.properties |
Sets default Web API value for development environment |
src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKeyReadWrite.java |
Renames enum and adds Swagger annotations |
src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ConfigurationKey.java |
Updates references to renamed enum |
src/main/java/de/rwth/idsg/steve/web/dto/OcppJsonStatus.java |
Adds Swagger annotations and JSON ignore directives |
src/main/java/de/rwth/idsg/steve/web/dto/ConnectorStatusForm.java |
Adds strategy enum and Swagger documentation |
| Various controller files | Updates enum references and adds new API controllers |
| API DTO files | New data transfer objects for Web API endpoints |
| Exception handling files | New exception classes and updated error handling |
| Repository implementations | New methods for transaction and charge point operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /* If a active transaction is found, don't unlock the connection. */ | ||
| var transactionId = transactionRepository.getActiveTransactionId(params.getChargeBoxId(), | ||
| params.getConnectorId()); | ||
| if (transactionId != null) { |
There was a problem hiding this comment.
The method getActiveTransactionId returns Optional<Integer>, but the code checks for null instead of using isPresent(). This should be if (transactionId.isPresent()).
| if (transactionId != null) { | |
| if (transactionId.isPresent()) { |
|
|
||
| @Getter | ||
| @Setter | ||
| //@RequiredArgsConstructor |
There was a problem hiding this comment.
Remove the commented-out annotation. Dead code should be removed to maintain code cleanliness.
| //@RequiredArgsConstructor |
| * | ||
| * @author fnkbsi | ||
| * since 20.10.2023 | ||
| * |
There was a problem hiding this comment.
The JavaDoc comment format is incomplete. Use @since tag properly: @since 20.10.2023 and remove empty lines within the JavaDoc comment.
| * | |
| * @author fnkbsi | |
| * @since 20.10.2023 |
| import java.util.Set; | ||
| import java.util.TreeMap; | ||
| import java.util.TreeSet; | ||
| import java.util.*; |
There was a problem hiding this comment.
Replace wildcard import with explicit imports. Wildcard imports can lead to naming conflicts and make dependencies unclear.
| import java.util.*; | |
| import java.util.Set; | |
| import java.util.Collections; | |
| import java.util.List; | |
| import java.util.TreeMap; | |
| import java.util.Map; | |
| import java.util.TreeSet; | |
| import java.util.Comparator; | |
| import java.util.EnumSet; |
juherr
force-pushed
the
fnkbsi/ApiRemoteStartStop
branch
from
585c745 to
27cc68c
Compare
juherr
force-pushed
the
fnkbsi/ApiRemoteStartStop
branch
from
27cc68c to
1870bad
Compare
Rebase steve-community#1291
Summary by CodeRabbit