Conversation
WalkthroughReplaced map-based schedule periods with a validated List across DTO, mapper, JSP, JS, and repository; repository now sorts and batch-inserts periods and getDetails returns Optional. Converted WebSocketLogger to an injectable component and updated logging wiring; consolidated exception handling into nested SteveException types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant JSP as JSP Form (schedulePeriods[])
participant JS as schedulePeriodsTable.js
participant Controller as ChargingProfilesController
participant Repo as ChargingProfileRepositoryImpl
participant DB as Database
User->>JSP: Open page (mapper supplies List)
JSP->>JS: Add/Delete rows (numeric ids)
JS-->>JSP: Update inputs schedulePeriods[i].*
User->>Controller: Submit ChargingProfileForm
Controller->>Repo: save(form)
Repo->>Repo: filter, sort by startPeriodInSeconds
Repo->>DB: batchInsert(ChargingSchedulePeriodRecords)
DB-->>Repo: OK
Repo-->>Controller: Persisted
Controller-->>User: Success
sequenceDiagram
autonumber
participant Endpoint as AbstractWebSocketEndpoint
participant Sender
participant PingTask
participant Logger as WebSocketLogger
Endpoint->>Logger: injected at construction
Sender->>Logger: injected at construction
Endpoint->>PingTask: new PingTask(Logger, chargeBoxId, session)
PingTask->>Logger: webSocketLogger.sendingPing(...)
Sender->>Logger: webSocketLogger.sending(...)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (1)
295-297: Wrong returning field; breaks jOOQ insert returningThe insertInto(CHARGING_PROFILE) statement returns CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK, which is from a different table. This will fail at runtime.
Apply this diff:
- .returning(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK) + .returning(CHARGING_PROFILE.CHARGING_PROFILE_PK)
🧹 Nitpick comments (5)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (1)
79-79: Differentiate list-level vs entry-level validation messageUsing the same message “Schedule Periods cannot be empty” both on the field and on the @AssertTrue method can confuse users. Consider changing the AssertTrue message to e.g. “At least one schedule period must be fully specified.”
steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (1)
260-264: Enforce deterministic ordering of schedule periods (optional)The current implementation retrieves schedule periods without any
ORDER BYclause, and the UI mapper simply preserves whatever sequence the database returns. Since relational databases do not guarantee row order without an explicit sort, the list of periods shown in the details view may appear in arbitrary order.Consider one of the following optional refactors to ensure periods are always displayed sorted by their start time:
• At the repository level, add an
ORDER BYonSTART_PERIOD_IN_SECONDSto the jOOQ query:--- a/steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java +++ b/steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java @@ -260,3 +260,4 @@ public ChargingProfile.Details getDetails(int chargingProfilePk) { var periods = ctx.selectFrom(CHARGING_SCHEDULE_PERIOD) - .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(chargingProfilePk)) - .fetch(); + .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(chargingProfilePk)) + .orderBy(CHARGING_SCHEDULE_PERIOD.START_PERIOD_IN_SECONDS.asc()) + .fetch(); return new ChargingProfile.Details(profile, periods);• Or in the UI mapper, sort before mapping:
--- a/steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java @@ -58,7 +58,11 @@ private static ChargingProfileForm.SchedulePeriod mapToFormPeriod(ChargingSchedul } public static ChargingProfileForm mapToForm(ChargingProfile.Details details) { - var periods = details.getPeriods(); + var periods = details.getPeriods().stream() + .sorted(Comparator.comparingInt(ChargingSchedulePeriodRecord::getStartPeriodInSeconds)) + .toList(); var form = new ChargingProfileForm(); form.setChargingProfilePk(profile.getChargingProfilePk()); … form.setSchedulePeriods( - periods.stream().map(ChargingProfileDetailsMapper::mapToFormPeriod).toList()); + periods.stream() + .map(ChargingProfileDetailsMapper::mapToFormPeriod) + .toList());Both approaches guarantee that the UI always displays schedule periods in ascending order of
startPeriodInSeconds.steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java (1)
59-62: Preserve UX by sorting periods by startPeriodInSecondsWithout sorting, rows may appear in arbitrary order. Sort before mapping.
Apply this diff:
- form.setSchedulePeriods(periods.stream() - .map(ChargingProfileDetailsMapper::mapToFormPeriod) - .toList()); + form.setSchedulePeriods(periods.stream() + .sorted(java.util.Comparator.comparingInt(ChargingSchedulePeriodRecord::getStartPeriodInSeconds)) + .map(ChargingProfileDetailsMapper::mapToFormPeriod) + .toList());steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp (1)
77-83: Surface per-field validation errors to usersAdd form:errors under each input so users see messages from @assertfalse validators.
Example:
- <td><form:input path="schedulePeriods[${status.index}].startPeriodInSeconds"/></td> + <td> + <form:input path="schedulePeriods[${status.index}].startPeriodInSeconds"/> + <form:errors path="schedulePeriods[${status.index}].startPeriodInSeconds" cssClass="error"/> + </td> - <td><form:input path="schedulePeriods[${status.index}].powerLimit"/></td> + <td> + <form:input path="schedulePeriods[${status.index}].powerLimit"/> + <form:errors path="schedulePeriods[${status.index}].powerLimit" cssClass="error"/> + </td>steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js (1)
7-18: Prefer const/template literals and consider reindexing after deletionsMinor cleanup and more robust row creation:
- let last = $('#periodsTable tbody tr').last(); - // If there is no row (because table is empty), start with id 0. Otherwise, increment last row's id. - let id = last.length === 0 ? 0 : parseInt(last[0].id, 10) + 1; - var startPeriodInSeconds = "<td><input id=\"schedulePeriods" + id + ".startPeriodInSeconds\" name=\"schedulePeriods[" + id + "].startPeriodInSeconds\" type=\"text\" value=\"\"/></td>"; - var powerLimit = "<td><input id=\"schedulePeriods" + id + ".powerLimit\" name=\"schedulePeriods[" + id + "].powerLimit\" type=\"text\" value=\"\"/></td>"; - var numberPhases = "<td><input id=\"schedulePeriods" + id + ".numberPhases\" name=\"schedulePeriods[" + id + "].numberPhases\" type=\"text\" value=\"\" placeholder=\"if empty, 3 will be assumed\"/></td>"; - var deleteButton = "<td><input type=\"button\" class=\"removeRow\" value=\"Delete\"></td>"; - var row = "<tr id=" + id + ">" + startPeriodInSeconds + powerLimit + numberPhases + deleteButton + "</tr>"; + const last = $('#periodsTable tbody tr').last(); + const id = last.length === 0 ? 0 : parseInt(last[0].id, 10) + 1; + const startPeriodInSeconds = `<td><input id="schedulePeriods${id}.startPeriodInSeconds" name="schedulePeriods[${id}].startPeriodInSeconds" type="text" value=""/></td>`; + const powerLimit = `<td><input id="schedulePeriods${id}.powerLimit" name="schedulePeriods[${id}].powerLimit" type="text" value=""/></td>`; + const numberPhases = `<td><input id="schedulePeriods${id}.numberPhases" name="schedulePeriods[${id}].numberPhases" type="text" value="" placeholder="if empty, 3 will be assumed"/></td>`; + const deleteButton = `<td><input type="button" class="removeRow" value="Delete"></td>`; + const row = `<tr id="${id}">${startPeriodInSeconds}${powerLimit}${numberPhases}${deleteButton}</tr>`;Optional: after a deletion, reindex row ids and input names to keep indices dense, reducing gaps sent to the server.
📜 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 (5)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java(3 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (2)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/QueryForm.java (1)
Getter(35-86)steve-core/src/main/java/de/rwth/idsg/steve/web/dto/OcppTagForm.java (1)
Getter(37-67)
🔇 Additional comments (3)
steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (2)
381-397: Batch insert pipeline looks good; consider guarding against unsorted/null inputs upstreamYou already filter isNonEmpty and sort by startPeriodInSeconds before batchInsert; this is solid. No further issues if DTO fix (defaulting numberPhases) lands.
382-385: Manual verification required: confirm removal ofschedulePeriodMapA repository-wide search for the literal
schedulePeriodMapyielded no matches. Please manually verify that:
- All code, tests, documentation, and configuration no longer reference the old
schedulePeriodMapfield.- Any migration steps or data transformations that depended on the map have been fully adapted to the new
schedulePeriodsapproach.steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java (1)
66-72: Helper method is clean and aligns with DTO defaultsMapping is straightforward; setNumberPhases() will apply the default when null. LGTM.
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java
Outdated
Show resolved
Hide resolved
| private Integer startPeriodInSeconds; // from the startSchedule | ||
|
|
||
| @NotNull(message = "Schedule period: Power Limit has to be set") private BigDecimal powerLimit; | ||
| private BigDecimal powerLimit; | ||
|
|
||
| private Integer numberPhases; | ||
|
|
||
| public Integer getNumberPhases() { | ||
| return Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | ||
| } | ||
|
|
||
| public void setNumberPhases(Integer numberPhases) { | ||
| this.numberPhases = Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | ||
| } | ||
|
|
||
| public boolean isNonEmpty() { | ||
| return (startPeriodInSeconds != null) && (powerLimit != null) && (numberPhases != null); | ||
| } | ||
|
|
||
| @AssertFalse(message = "Schedule period: Power Limit has to be set") public boolean isPowerLimitMissing() { | ||
| return (startPeriodInSeconds != null) && (powerLimit == null) && (numberPhases != null); | ||
| } | ||
|
|
||
| @AssertFalse(message = "Schedule period: Start Period has to be set") public boolean isStartPeriodInSecondsMissing() { | ||
| return (startPeriodInSeconds == null) && (powerLimit != null) && (numberPhases != null); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default numberPhases via getter and relax validations to match UI contract
Align with OcppTagForm’s “default in getter” pattern and ensure periods with start+power but empty numberPhases pass validation and persist with default=3.
Apply this diff:
class SchedulePeriod {
@@
- private Integer numberPhases;
+ private Integer numberPhases;
@@
public void setNumberPhases(Integer numberPhases) {
this.numberPhases = Objects.requireNonNullElse(numberPhases, defaultNumberPhases);
}
+
+ // Ensure default of 3 if not provided (consistent with UI hint and repository usage)
+ public Integer getNumberPhases() {
+ return Objects.requireNonNullElse(numberPhases, defaultNumberPhases);
+ }
@@
- public boolean isNonEmpty() {
- return (startPeriodInSeconds != null) && (powerLimit != null) && (numberPhases != null);
- }
+ public boolean isNonEmpty() {
+ // Only require the essential fields; numberPhases defaults to 3 if omitted.
+ return (startPeriodInSeconds != null) && (powerLimit != null);
+ }
@@
- @AssertFalse(message = "Schedule period: Power Limit has to be set") public boolean isPowerLimitMissing() {
- return (startPeriodInSeconds != null) && (powerLimit == null) && (numberPhases != null);
- }
+ @AssertFalse(message = "Schedule period: Power Limit has to be set") public boolean isPowerLimitMissing() {
+ return (startPeriodInSeconds != null) && (powerLimit == null);
+ }
@@
- @AssertFalse(message = "Schedule period: Start Period has to be set") public boolean isStartPeriodInSecondsMissing() {
- return (startPeriodInSeconds == null) && (powerLimit != null) && (numberPhases != null);
- }
+ @AssertFalse(message = "Schedule period: Start Period has to be set") public boolean isStartPeriodInSecondsMissing() {
+ return (startPeriodInSeconds == null) && (powerLimit != null);
+ }Optionally add constraints to catch bad inputs:
You’ll need to import jakarta.validation.constraints.Min/Max/Positive as needed.
📝 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 Integer startPeriodInSeconds; // from the startSchedule | |
| @NotNull(message = "Schedule period: Power Limit has to be set") private BigDecimal powerLimit; | |
| private BigDecimal powerLimit; | |
| private Integer numberPhases; | |
| public Integer getNumberPhases() { | |
| return Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | |
| } | |
| public void setNumberPhases(Integer numberPhases) { | |
| this.numberPhases = Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | |
| } | |
| public boolean isNonEmpty() { | |
| return (startPeriodInSeconds != null) && (powerLimit != null) && (numberPhases != null); | |
| } | |
| @AssertFalse(message = "Schedule period: Power Limit has to be set") public boolean isPowerLimitMissing() { | |
| return (startPeriodInSeconds != null) && (powerLimit == null) && (numberPhases != null); | |
| } | |
| @AssertFalse(message = "Schedule period: Start Period has to be set") public boolean isStartPeriodInSecondsMissing() { | |
| return (startPeriodInSeconds == null) && (powerLimit != null) && (numberPhases != null); | |
| } | |
| } | |
| private Integer startPeriodInSeconds; // from the startSchedule | |
| private BigDecimal powerLimit; | |
| private Integer numberPhases; | |
| public void setNumberPhases(Integer numberPhases) { | |
| this.numberPhases = Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | |
| } | |
| // Ensure default of 3 if not provided (consistent with UI hint and repository usage) | |
| public Integer getNumberPhases() { | |
| return Objects.requireNonNullElse(numberPhases, defaultNumberPhases); | |
| } | |
| public boolean isNonEmpty() { | |
| // Only require the essential fields; numberPhases defaults to 3 if omitted. | |
| return (startPeriodInSeconds != null) && (powerLimit != null); | |
| } | |
| @AssertFalse(message = "Schedule period: Power Limit has to be set") | |
| public boolean isPowerLimitMissing() { | |
| return (startPeriodInSeconds != null) && (powerLimit == null); | |
| } | |
| @AssertFalse(message = "Schedule period: Start Period has to be set") | |
| public boolean isStartPeriodInSecondsMissing() { | |
| return (startPeriodInSeconds == null) && (powerLimit != null); | |
| } |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java
around lines 126 to 147, change the handling of numberPhases to follow the
"default in getter" pattern: remove the requireNonNullElse in the setter so
numberPhases can be left null, implement getNumberPhases() to return
numberPhases != null ? numberPhases : 3 (default), and adjust the validation
methods so a period with startPeriodInSeconds and powerLimit set but
numberPhases null is considered valid (it will persist as default=3); optionally
add jakarta.validation annotations on fields (startPeriodInSeconds:
@PositiveOrZero, powerLimit: @Positive, numberPhases: @Min(1) @Max(3)) and
import Min, Max, Positive, PositiveOrZero as needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (1)
138-148: Per-period validations now match UI contractisNonEmpty relies on start+power only; missing-field guards no longer depend on numberPhases. This resolves the earlier edge case noted in prior reviews.
🧹 Nitpick comments (3)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (3)
81-81: Differentiate validation messages for clarityField-level @notempty and the custom @AssertTrue share the same message, but enforce different things (list non-empty vs. at least one fully specified period). Suggest clearer, distinct messages.
Apply:
- @NotEmpty(message = "Schedule Periods cannot be empty") @Valid private List<SchedulePeriod> schedulePeriods; + @NotEmpty(message = "Schedule Periods list must not be empty") @Valid private List<SchedulePeriod> schedulePeriods;
113-119: Make “non-empty period” error explicitThe predicate is good. Update the message to reflect that at least one period must have both Start Period and Power Limit.
- @AssertTrue(message = "Schedule Periods cannot be empty") public boolean isSchedulePeriodsValid() { + @AssertTrue(message = "At least one Schedule Period must specify both Start Period and Power Limit") public boolean isSchedulePeriodsValid() { if (CollectionUtils.isEmpty(schedulePeriods)) { return false; } return schedulePeriods.stream().filter(Objects::nonNull).anyMatch(SchedulePeriod::isNonEmpty); }
128-133: Review domain rule forpowerLimit: zero is allowed by OCPP spec
- In steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (lines 128–133) the field is annotated with
@Positive, which forbids zero.- OCPP 1.6 defines
ChargingSchedulePeriod.limitas a required decimal (no explicit “> 0” constraint) (scribd.com) and its JSON‐schema examples include"limit": 0(ocpp-spec.org).Optional change if you need to represent “stop charging” with a zero limit:
- @Positive private BigDecimal powerLimit; + @PositiveOrZero private BigDecimal powerLimit;
📜 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 (4)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java(3 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js
- steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp
- steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java
🧰 Additional context used
🧬 Code graph analysis (1)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (2)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (3)
Getter(36-49)Getter(51-58)Getter(60-65)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js (3)
startPeriodInSeconds(11-11)powerLimit(12-12)numberPhases(13-13)
⏰ 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). (20)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
🔇 Additional comments (3)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (3)
29-45: Imports: additions look correctNew imports align with the validation and collection utilities used below.
126-126: Good: centralized defaultDEFAULT_NUMBER_PHASES constant is clear and fixes past ambiguity around implicit defaults.
135-137: No direct field accesses found; getter usage confirmedAll downstream mappings and repository/task implementations invoke
getNumberPhases(), preserving the default value, and noschedulePeriodMaporgetSchedulePeriodMapreferences remain.
juherr
force-pushed
the
features/steve-1791
branch
from
7d424f9 to
6f92063
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js (1)
2-4: Reindex rows after deletion to keep list indices contiguous (prevents null holes on bind).Without reindexing, deleting middle rows yields non-contiguous indices (e.g., 0,1,3), which can post null entries and trip server-side processing. Reindex immediately after a delete.
Apply this diff:
-$('#periodsTable').on('click', '.removeRow', function() { - $(this).closest("tr").remove(); -}); +$('#periodsTable').on('click', '.removeRow', function(e) { + e.preventDefault(); + $(this).closest("tr").remove(); + reindexSchedulePeriods(); +});Add this helper in the same file (outside the shown range):
function reindexSchedulePeriods() { $('#periodsTable tbody tr').each(function (idx) { const $tr = $(this).attr('id', String(idx)); $tr.find('input').each(function () { const $in = $(this); const name = $in.attr('name'); // schedulePeriods[3].field if (name) $in.attr('name', name.replace(/schedulePeriods\[\d+]/, `schedulePeriods[${idx}]`)); const id = $in.attr('id'); // schedulePeriods3.field if (id) $in.attr('id', id.replace(/schedulePeriods\d+\./, `schedulePeriods${idx}.`)); }); }); }steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
100-116: Guard against uninitialized pipeline to avoid NPEs.
pipelineis nullable and used without a null check. Ifinit(...)hasn’t run yet when a message arrives, this will NPE.Apply:
var context = new CommunicationContext(session, chargeBoxId); context.setIncomingString(incomingString); - pipeline.accept(context); + var p = this.pipeline; + if (p == null) { + session.close(CloseStatus.SERVER_ERROR.withReason("Pipeline not initialized")); + return; + } + p.accept(context);Additionally, consider safe publication:
- private @Nullable IncomingPipeline pipeline; + private volatile @Nullable IncomingPipeline pipeline;
🧹 Nitpick comments (6)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/WebSocketLogger.java (1)
34-75: Harden against potential null session and reduce log volume
- A defensive session-id accessor avoids accidental NPEs if a null slips through.
- Consider logging payloads at
debugto prevent noisy logs in production.Apply within-body changes:
- log.info("[chargeBoxId={}, sessionId={}] Sending: {}", chargeBoxId, session.getId(), msg); + log.debug("[chargeBoxId={}, sessionId={}] Sending: {}", chargeBoxId, session != null ? session.getId() : "null", msg); - log.info("[chargeBoxId={}, sessionId={}] Received: {}", chargeBoxId, session.getId(), msg); + log.debug("[chargeBoxId={}, sessionId={}] Received: {}", chargeBoxId, session != null ? session.getId() : "null", msg);Support code to add (outside the changed ranges, near class end) if you prefer a helper:
private static String sid(WebSocketSession s) { return s != null ? s.getId() : "null"; }Then replace
session.getId()calls withsid(session).steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
70-75: Provide a templated constructor for NotFound (parity with AlreadyExists).
Enables richer messages without manual String.format at call sites.public static class NotFound extends SteveException { public NotFound(String message) { super(message); } + public NotFound(String template, Object arg1) { + super(format(template, arg1)); + } }steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (1)
416-418: Include the profile PK in the NotFound message.
Improves diagnostics and client logs.- var details = chargingProfileRepository - .getDetails(params.getChargingProfilePk()) - .orElseThrow(() -> new SteveException.NotFound("Charging Profile not found")); + var details = chargingProfileRepository + .getDetails(params.getChargingProfilePk()) + .orElseThrow(() -> new SteveException.NotFound( + "Charging Profile with pk=%s not found", params.getChargingProfilePk()));steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js (1)
11-13: Prefer numeric inputs and safer element ids.
- Use type="number" with min/step to improve UX/validation.
- Avoid dots in element ids (fragile in CSS/jQuery selectors). Keep name=... as-is for Spring binding.
Apply this diff:
- const startPeriodInSeconds = `<td><input id="schedulePeriods${id}.startPeriodInSeconds" name="schedulePeriods[${id}].startPeriodInSeconds" type="text" value=""/></td>`; - const powerLimit = `<td><input id="schedulePeriods${id}.powerLimit" name="schedulePeriods[${id}].powerLimit" type="text" value=""/></td>`; - const numberPhases = `<td><input id="schedulePeriods${id}.numberPhases" name="schedulePeriods[${id}].numberPhases" type="text" value="" placeholder="if empty, 3 will be assumed"/></td>`; + const startPeriodInSeconds = `<td><input id="schedulePeriods_${id}_startPeriodInSeconds" name="schedulePeriods[${id}].startPeriodInSeconds" type="number" min="0" step="1" value=""/></td>`; + const powerLimit = `<td><input id="schedulePeriods_${id}_powerLimit" name="schedulePeriods[${id}].powerLimit" type="number" min="0" step="0.001" value=""/></td>`; + const numberPhases = `<td><input id="schedulePeriods_${id}_numberPhases" name="schedulePeriods[${id}].numberPhases" type="number" min="1" max="3" step="1" value="" placeholder="if empty, 3 will be assumed"/></td>`;steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (2)
118-121: Avoid duplicate attribute lookups in handlePongMessage.Minor nit: cache
chargeBoxIdonce for consistency and clarity.- private void handlePongMessage(WebSocketSession session) { - webSocketLogger.receivedPong(getChargeBoxId(session), session); - ocppServerRepository.updateChargeboxHeartbeat(getChargeBoxId(session), Instant.now()); - } + private void handlePongMessage(WebSocketSession session) { + var chargeBoxId = getChargeBoxId(session); + webSocketLogger.receivedPong(chargeBoxId, session); + ocppServerRepository.updateChargeboxHeartbeat(chargeBoxId, Instant.now()); + }
175-178: onError logging switch to instance logger — LGTM.Consider ensuring logger failures never propagate from here, but current usage is fine.
📜 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 (18)
steve-api/src/main/java/de/rwth/idsg/steve/config/ApiDocsConfiguration.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/ChargingProfileRepository.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java(3 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/config/OcppConfiguration.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java(5 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/PingTask.java(1 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/WebSocketLogger.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java(3 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java(3 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java(3 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java(2 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java(2 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ChargingProfilesController.java(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp(2 hunks)steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/data-man/00-charging-profile.jsp
- steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java
🧰 Additional context used
🧬 Code graph analysis (4)
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ChargingProfilesController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-76)
steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-76)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java (2)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
RequiredArgsConstructor(55-216)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java (2)
RequiredArgsConstructor(41-116)RequiredArgsConstructor(82-115)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/WebSocketLogger.java (3)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java (1)
Component(58-122)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java (1)
Component(59-127)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java (1)
Component(59-127)
⏰ 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). (20)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (19)
steve-api/src/main/java/de/rwth/idsg/steve/config/ApiDocsConfiguration.java (2)
78-86: Localvarusage is fineType inference here is clear (
StringandSecurityScheme). No issues.
72-73:springdoc.writer-with-order-by-keysis supported and won’t be overridden by application.properties
Project uses SpringDoc 2.8.10 (see pom.xml) and the official Springdoc properties list includesspringdoc.writer-with-order-by-keys(github.com, springdoc.org). In Spring Boot’s property resolution, Java system properties (System.setProperty) have higher precedence than values inapplication.properties(dev.to).steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/WebSocketLogger.java (2)
31-31: Bean-ifying the logger is correctSwitch to
@Component+ instance methods aligns with DI across call sites. Good move.
69-75: Error logging change is goodPassing
Throwableas the last arg is the correct SLF4J pattern; removingisErrorEnabled()is fine.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/config/OcppConfiguration.java (1)
74-76: No duplicate Sender bean
Senderis not annotated with@Component; only thesenderbean defined inOcppConfigurationis registered.steve-core/src/main/java/de/rwth/idsg/steve/repository/ChargingProfileRepository.java (1)
30-31: Document Optional<ChargingProfile.Details> contract in Javadoc
Add a Javadoc comment aboveOptional<ChargingProfile.Details> getDetails(int chargingProfilePk)in ChargingProfileRepository.java to state that it returns an empty Optional when the profile PK isn’t found. No changes are needed in the implementation or existing callers—they already unwrap the Optional appropriately.steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
70-75: Add NotFound: good addition aligned with Optional-based lookups.steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java (1)
21-21: Import SteveException: OK.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java (1)
39-43: Sender is already registered as a Spring bean. Thesender(WebSocketLogger)factory method in steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/config/OcppConfiguration.java defines and returns aSenderbean, so no additional registration is needed.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java (1)
65-77: Constructor and super call updated to inject WebSocketLogger: looks correct.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java (2)
31-31: LGTM: Logger is now injected.Constructor injection of WebSocketLogger and forwarding to super is correct and consistent with DI best practices.
65-77: All WebSocket endpoint constructors include WebSocketLogger
Verified that Ocpp12WebSocketEndpoint, Ocpp15WebSocketEndpoint, and Ocpp16WebSocketEndpoint each declare WebSocketLogger as their first constructor argument; no additional bean definitions require changes.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java (2)
31-31: LGTM: Logger injection mirrors 1.5 endpoint.
64-76: DI consistency verified: All OCPP WebSocketEndpoint subclasses (Ocpp12, Ocpp15, Ocpp16) accept WebSocketLogger and forward it in their super(…) calls, and both Sender and PingTask are correctly constructor-injected with WebSocketLogger.steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (2)
256-271: LGTM: Optional return and deterministic period order.Returning Optional for getDetails and ordering periods by START_PERIOD_IN_SECONDS improves API clarity and reproducibility.
388-402: No changes required: validation already enforces non-null schedule periods and startPeriodInSeconds
The DTO applies @notempty to schedulePeriods (and @NotNull on each element) and @NotNull on SchedulePeriod.startPeriodInSeconds, so the stream and comparator cannot receive nulls.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (3)
58-58: Injecting WebSocketLogger instead of statics — good change.Instance-based logging enables easier testing and per-endpoint configuration.
157-157: onClose logging switch to instance logger — LGTM.
133-133: All PingTask instantiations are using the 3-argument constructor. No remaining 2-arg usages found.
| var details = chargingProfileRepository | ||
| .getDetails(params.getChargingProfilePk()) | ||
| .orElseThrow(() -> new SteveException.NotFound("Charging Profile not found")); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HTTP mapping for NotFound → 404.
If no @ControllerAdvice translates SteveException.NotFound to 404, UI calls will surface as 500.
🏁 Script executed:
#!/bin/bash
# Expect: an @ExceptionHandler for SteveException.NotFound (preferably in UI/web module).
rg -nP '@ExceptionHandler\(\s*SteveException\.NotFound' -C3 --type=java
# Alternative mapping via ResponseStatusExceptionResolver-style annotations:
rg -nP 'class\s+.*ControllerAdvice|@ResponseStatus' -C2 --type=javaLength of output: 5140
🏁 Script executed:
#!/bin/bash
# Locate SteveException source file
STEVE_FILE=$(rg -l 'class SteveException' --type=java)
echo "SteveException file: $STEVE_FILE"
# Print first 200 lines to inspect nested NotFound class
sed -n '1,200p' "$STEVE_FILE"
# Locate all NotFoundException class declarations to check inheritance
rg -nP 'class\s+NotFoundException' -n --type=javaLength of output: 3093
Map SteveException.NotFound to HTTP 404
SteveException.NotFound isn’t handled by any ControllerAdvice and falls through to the generic 500 handler. Add an @ExceptionHandler(SteveException.NotFound) with @ResponseStatus(HttpStatus.NOT_FOUND) in ApiControllerAdvice (and GlobalControllerAdvice for UI) or annotate the exception class itself.
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java
around lines 416-418, SteveException.NotFound is thrown but not mapped to HTTP
404 and falls through to a 500 response; add handling so clients receive 404.
Fix by either annotating SteveException.NotFound with
@ResponseStatus(HttpStatus.NOT_FOUND) on the exception class, or add an
@ExceptionHandler(SteveException.NotFound) method in ApiControllerAdvice (and
GlobalControllerAdvice for UI) annotated with
@ResponseStatus(HttpStatus.NOT_FOUND) that returns an appropriate error
body/DTO; ensure the handler accepts the exception parameter and returns the
standardized error response used by the project.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java
Show resolved
Hide resolved
| private static final PingMessage PING_MESSAGE = new PingMessage(ByteBuffer.wrap("ping".getBytes(UTF_8))); | ||
|
|
||
| @Override | ||
| public void run() { | ||
| WebSocketLogger.sendingPing(chargeBoxId, session); | ||
| webSocketLogger.sendingPing(chargeBoxId, session); | ||
| try { | ||
| session.sendMessage(PING_MESSAGE); | ||
| } catch (IOException e) { | ||
| WebSocketLogger.pingError(chargeBoxId, session, e); | ||
| webSocketLogger.pingError(chargeBoxId, session, e); | ||
| // TODO: Do something about this | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid sharing a single PingMessage/ByteBuffer across threads.
PingMessage/ByteBuffer can have mutable position; use a duplicated read-only buffer per send.
- private static final PingMessage PING_MESSAGE = new PingMessage(ByteBuffer.wrap("ping".getBytes(UTF_8)));
+ private static final ByteBuffer PING_PAYLOAD = ByteBuffer.wrap("ping".getBytes(UTF_8)).asReadOnlyBuffer();
@@
- try {
- session.sendMessage(PING_MESSAGE);
+ try {
+ session.sendMessage(new PingMessage(PING_PAYLOAD.duplicate()));📝 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 PingMessage PING_MESSAGE = new PingMessage(ByteBuffer.wrap("ping".getBytes(UTF_8))); | |
| @Override | |
| public void run() { | |
| WebSocketLogger.sendingPing(chargeBoxId, session); | |
| webSocketLogger.sendingPing(chargeBoxId, session); | |
| try { | |
| session.sendMessage(PING_MESSAGE); | |
| } catch (IOException e) { | |
| WebSocketLogger.pingError(chargeBoxId, session, e); | |
| webSocketLogger.pingError(chargeBoxId, session, e); | |
| // TODO: Do something about this | |
| } | |
| // Replace shared PingMessage with a reusable read-only payload buffer | |
| private static final ByteBuffer PING_PAYLOAD = | |
| ByteBuffer.wrap("ping".getBytes(UTF_8)) | |
| .asReadOnlyBuffer(); | |
| @Override | |
| public void run() { | |
| webSocketLogger.sendingPing(chargeBoxId, session); | |
| try { | |
| // Duplicate the read-only buffer for each send to avoid shared mutable state | |
| session.sendMessage(new PingMessage(PING_PAYLOAD.duplicate())); | |
| } catch (IOException e) { | |
| webSocketLogger.pingError(chargeBoxId, session, e); | |
| // TODO: Do something about this | |
| } | |
| } |
🤖 Prompt for AI Agents
In steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/PingTask.java
around lines 42 to 52, don't reuse the static PING_MESSAGE (its ByteBuffer is
mutable and not thread-safe); instead keep a static byte[] or CharSequence for
the ping payload and create a fresh, read-only ByteBuffer for each send (e.g.
ByteBuffer.wrap(PING_BYTES).asReadOnlyBuffer() or
ByteBuffer.wrap(...).duplicate() before calling new PingMessage(...)) and call
session.sendMessage with that new PingMessage so each thread/send uses its own
buffer instance.
| form.setSchedulePeriods(periods.stream() | ||
| .map(ChargingProfileDetailsMapper::mapToFormPeriod) | ||
| .toList()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Stream.toList() immutability if callers may mutate schedulePeriods.
Stream.toList() returns an unmodifiable List; if later code appends/edits periods on the form, this will throw. Prefer a mutable list.
Apply this diff:
- form.setSchedulePeriods(periods.stream()
- .map(ChargingProfileDetailsMapper::mapToFormPeriod)
- .toList());
+ form.setSchedulePeriods(periods.stream()
+ .map(ChargingProfileDetailsMapper::mapToFormPeriod)
+ .collect(java.util.stream.Collectors.toCollection(java.util.ArrayList::new)));If you prefer imports, add:
import java.util.ArrayList;
import java.util.stream.Collectors;🤖 Prompt for AI Agents
In
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java
around lines 59-61, the code uses Stream.toList() which returns an unmodifiable
List; change the mapping to produce a mutable ArrayList (e.g. collect into a new
ArrayList via Collectors.toCollection(ArrayList::new) or wrap the result in new
ArrayList<>(...)) so callers can safely modify form.schedulePeriods, and add
imports for java.util.ArrayList and java.util.stream.Collectors if not already
present.
| var details = repository | ||
| .getDetails(chargingProfilePk) | ||
| .orElseThrow(() -> new SteveException.NotFound("Charging Profile not found")); | ||
| ; | ||
| var form = ChargingProfileDetailsMapper.mapToForm(details); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer informative message and drop stray semicolon
Include the PK in the NotFound message and remove the extra ; line.
- var details = repository
- .getDetails(chargingProfilePk)
- .orElseThrow(() -> new SteveException.NotFound("Charging Profile not found"));
- ;
+ var details = repository
+ .getDetails(chargingProfilePk)
+ .orElseThrow(() -> new SteveException.NotFound(
+ String.format("Charging Profile %d not found", chargingProfilePk)));Do we have a @ControllerAdvice mapping SteveException.NotFound to HTTP 404? If not, users will see 500. Search:
🏁 Script executed:
#!/bin/bash
rg -nP '@ExceptionHandler\(\s*SteveException\.NotFound' -C2Length of output: 61
🏁 Script executed:
#!/bin/bash
# Search for any @ControllerAdvice classes
rg -nP '@ControllerAdvice' -C2 .
# Search for ExceptionHandler methods targeting SteveException.NotFound
rg -nP '@ExceptionHandler\(.*SteveException\.NotFound' -C2 .Length of output: 650
🏁 Script executed:
#!/bin/bash
# Check for any exception handlers in GlobalControllerAdvice
rg -nP '@ExceptionHandler' -C3 steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/GlobalControllerAdvice.javaLength of output: 438
Include PK in NotFound message, remove stray semicolon, and map to 404
-
Update code to include the
chargingProfilePkin the exception message and drop the extra semicolon:- var details = repository - .getDetails(chargingProfilePk) - .orElseThrow(() -> new SteveException.NotFound("Charging Profile not found")); - ; + var details = repository + .getDetails(chargingProfilePk) + .orElseThrow(() -> new SteveException.NotFound( + String.format("Charging Profile %d not found", chargingProfilePk)));
-
Currently
SteveException.NotFoundbubbles to the generic@ExceptionHandler(Exception.class)inGlobalControllerAdvice, resulting in HTTP 500. Add a specific handler forNotFoundto return HTTP 404:@ExceptionHandler(SteveException.NotFound.class) @ResponseStatus(HttpStatus.NOT_FOUND) public ModelAndView handleNotFound(HttpServletRequest req, SteveException.NotFound ex) { log.warn("Resource not found: {}", req.getRequestURL(), ex); return errorModel("not-found", ex.getMessage()); }
🤖 Prompt for AI Agents
In
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ChargingProfilesController.java
around lines 129 to 133, remove the stray semicolon, change the NotFound
exception to include the chargingProfilePk in the message (e.g. "Charging
Profile not found: " + chargingProfilePk) and then map details to form as
before; additionally add a specific exception handler in GlobalControllerAdvice
for SteveException.NotFound that returns HTTP 404 (annotate with
@ExceptionHandler(SteveException.NotFound.class) and
@ResponseStatus(HttpStatus.NOT_FOUND)), logs a warning with the request URL and
exception, and returns the "not-found" error model with ex.getMessage().
| // Find the last table row | ||
| const last = $('#periodsTable tbody tr').last(); | ||
| // If there is no row (because table is empty), start with id 0. Otherwise, increment last row's id. | ||
| const id = last.length === 0 ? 0 : parseInt(last[0].id, 10) + 1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use tbody length for next index; avoid NaN/DOM id coupling.
Relying on last row’s DOM id can break if ids drift; using current row count is simpler and robust, especially with reindexing.
Apply this diff:
- // If there is no row (because table is empty), start with id 0. Otherwise, increment last row's id.
- const id = last.length === 0 ? 0 : parseInt(last[0].id, 10) + 1;
+ // Next index = current row count (rows are reindexed on delete)
+ const id = $('#periodsTable tbody tr').length;📝 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.
| // Find the last table row | |
| const last = $('#periodsTable tbody tr').last(); | |
| // If there is no row (because table is empty), start with id 0. Otherwise, increment last row's id. | |
| const id = last.length === 0 ? 0 : parseInt(last[0].id, 10) + 1; | |
| // Find the last table row | |
| const last = $('#periodsTable tbody tr').last(); | |
| // Next index = current row count (rows are reindexed on delete) | |
| const id = $('#periodsTable tbody tr').length; |
🤖 Prompt for AI Agents
In
steve-ui-jsp/src/main/resources/webapp/WEB-INF/views/snippets/schedulePeriodsTable.js
around lines 7 to 10, replace the logic that parses the last row's DOM id with
using the current tbody row count to compute the next index: compute const id =
$('#periodsTable tbody tr').length so an empty table yields 0 and each new row
uses the stable count-based index; remove reliance on last[0].id/parseInt to
avoid NaN and decouple from DOM id drift.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
128-134: Consider guarding pipeline exceptions to avoid dropping sessions.If
pipeline.accept(context)throws, the handler thread will bubble up to the container. Wrap and close with 1011 or log as needed.- pipeline.accept(context); + try { + pipeline.accept(context); + } catch (RuntimeException e) { + webSocketLogger.transportError(chargeBoxId, session, e); + try { + session.close(CloseStatus.SERVER_ERROR.withReason("Unhandled error while processing message")); + } catch (Exception ignore) { } + }steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java (1)
59-63: Bug: Instantiating abstract SteveException will not compile.
SteveExceptionis now abstract;new SteveException(...)is invalid. This path represents an invariant breach and should be a 5xx. UseIllegalStateException(or introduce a dedicated internal error subtype) instead.- var body = reservationsService - .getReservation(id) - .orElseThrow( - () -> new SteveException("Reservation not found after creation, this should never happen")); + var body = reservationsService + .getReservation(id) + .orElseThrow(() -> new IllegalStateException( + "Reservation not found after creation, this should never happen. id=" + id));
♻️ Duplicate comments (1)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
59-61: Resolved: close when chargeBoxId is missing on open.The null/empty guard with 1008 (policy violation) addresses the earlier risk.
Also applies to: 145-151
🧹 Nitpick comments (10)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
92-98: Make callback lists thread-safe or document single-writer semantics.If
registerConnectedCallback/registerDisconnectedCallbackmay be called while iterating on connect/disconnect, switch toCopyOnWriteArrayListto avoid CME.- private final List<Consumer<String>> connectedCallbackList; - private final List<Consumer<String>> disconnectedCallbackList; + private final List<Consumer<String>> connectedCallbackList; + private final List<Consumer<String>> disconnectedCallbackList; @@ - this.connectedCallbackList = new ArrayList<>(); + this.connectedCallbackList = new java.util.concurrent.CopyOnWriteArrayList<>(); @@ - this.disconnectedCallbackList = new ArrayList<>(); + this.disconnectedCallbackList = new java.util.concurrent.CopyOnWriteArrayList<>();Also applies to: 216-222
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (3)
65-67: Delegate to super(template, …) instead of pre-formatting.Using
super(format(...))bypasses the base “template + args” constructors and duplicates formatting logic. Call the formatting-aware super instead for consistency and to keep one formatting path.- public AlreadyExists(String template, Object arg1) { - super(format(template, arg1)); - } + public AlreadyExists(String template, Object arg1) { + super(template, arg1); + } @@ - public NotFound(String template, Object arg1) { - super(format(template, arg1)); - } + public NotFound(String template, Object arg1) { + super(template, arg1); + } @@ - public BadRequest(String template, Object arg1) { - super(format(template, arg1)); - } + public BadRequest(String template, Object arg1) { + super(template, arg1); + }Also applies to: 76-78, 86-88
70-89: Add cause-bearing overloads for NotFound/BadRequest.Parity with the base class helps preserve stack traces and makes wrapping easier.
public static class NotFound extends SteveException { public NotFound(String message) { super(message); } + public NotFound(String message, Throwable cause) { + super(message, cause); + } public NotFound(String template, Object arg1) { super(template, arg1); } + public NotFound(String template, Object arg1, Throwable cause) { + super(template, arg1, cause); + } } public static class BadRequest extends SteveException { public BadRequest(String message) { super(message); } + public BadRequest(String message, Throwable cause) { + super(message, cause); + } public BadRequest(String template, Object arg1) { super(template, arg1); } + public BadRequest(String template, Object arg1, Throwable cause) { + super(template, arg1, cause); + } }
63-69: Consider a message-only constructor for AlreadyExists.To match NotFound/BadRequest and reduce friction at call sites.
public static class AlreadyExists extends SteveException { - - public AlreadyExists(String template, Object arg1) { - super(template, arg1); - } + public AlreadyExists(String template, Object arg1) { + super(template, arg1); + } + public AlreadyExists(String message) { + super(message); + } }steve-api/src/main/java/de/rwth/idsg/steve/web/api/TasksRestController.java (1)
84-85: Use the template-aware ctor for NotFound (consistency).Keeps exception messages consistent and avoids string concatenation.
- throw new SteveException.NotFound("Task not found: " + taskId); + throw new SteveException.NotFound("Task not found: %d", taskId);steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java (1)
189-189: Use template ctor instead of string concatenation.
Align with SteveException guidance and keep messages consistent.- throw new SteveException.NotFound("Could not find ocppTag with id " + ocppTagPk); + throw new SteveException.NotFound("Could not find ocppTag with id %s", ocppTagPk);steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java (2)
146-147: Consider 409 (Conflict) for “active task/transaction” cases.
These are state conflicts rather than malformed requests. Optional follow-up: introduce SteveException.Conflict and map to 409 in ApiControllerAdvice, then update these throws.Also applies to: 159-160, 221-222, 238-239
146-147: Prefer template ctor over String.format when using a single placeholder.
Minor consistency nit; reduces verbosity.- var errMsg = String.format("Active task found on ChargeBox %s!", params.getChargeBoxId()); - throw new SteveException.BadRequest(errMsg); + throw new SteveException.BadRequest("Active task found on ChargeBox %s!", params.getChargeBoxId());Also applies to: 183-183
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ApiControllerAdvice.java (2)
77-80: Optional: add dedicated handlers for Conflict/Forbidden if introduced.
Prepares for more accurate HTTP semantics suggested in controllers.
94-100: Avoid leaking internal error messages on 500 responses.
Return a generic message for INTERNAL_SERVER_ERROR; log the details.Example tweak:
- return createResponse(url, HttpStatus.INTERNAL_SERVER_ERROR, exception.getMessage()); + return createResponse(url, HttpStatus.INTERNAL_SERVER_ERROR, "Unexpected server error");
📜 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 (15)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ApiControllerAdvice.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java(6 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java(1 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/TasksRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/TransactionsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/BadRequestException.java(0 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/NotFoundException.java(0 hunks)steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java(6 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java(1 hunks)
💤 Files with no reviewable changes (2)
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/BadRequestException.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/NotFoundException.java
🧰 Additional context used
🧬 Code graph analysis (5)
steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-90)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/TransactionsRestController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-90)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-90)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-90)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/TasksRestController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-90)
⏰ 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). (20)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (14)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java (1)
49-55: Fail-fast path looks good.Early return on deserialization error before switching on message type is correct and preserves existing behavior.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java (2)
86-115: Dispatch table is consistent with OCPP 1.5.Switch covers expected request types; default throws with a clear message.
57-79: Approve changes; DI wiring validated. WebSocketLogger is registered as a Spring @component, Sender is exposed via a @bean in OcppConfiguration matching its constructor, and there’s no leftover @PostConstruct pipeline initialization in the endpoints.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java (1)
86-115: Dispatch mapping looks correct.Covers standard v1.6 requests; default path is explicit.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java (2)
85-110: Dispatch coverage OK.Cases and transport settings look right for OCPP 1.2 JSON.
56-78: Approve constructor-based wiring. Verified that Ocpp12TypeStore is annotated @component and CentralSystemService12_SoapServer is annotated @service, so both beans are defined for injection.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (2)
71-98: Base-class constructor: solid centralization of pipeline wiring.Creating
IncomingPipelinehere reduces duplication across endpoints; the event callbacks are set up cleanly.
212-214: No action needed; CHARGEBOX_ID_KEY is used consistently by both producer (OcppWebSocketHandshakeHandler.java:96) and consumer (AbstractWebSocketEndpoint.java:213).steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java (1)
21-21: LGTM: tests updated to SteveException.NotFound.The switch to the new exception type aligns with the refactor; assertion for 404 remains correct.
Also applies to: 92-95
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java (1)
72-75: Message formatting: prefer template-aware ctor for consistency.Minor: Using the template-aware constructor keeps messaging uniform across the codebase (already done here). No action needed.
steve-api/src/main/java/de/rwth/idsg/steve/web/api/TransactionsRestController.java (1)
64-66: LGTM – controller advice correctly maps SteveException.BadRequest to HTTP 400 for unsupported CSV flag.steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java (1)
21-21: LGTM: migrated to SteveException import.
Consistent with project-wide exception unification.steve-api/src/main/java/de/rwth/idsg/steve/web/api/ApiControllerAdvice.java (2)
60-63: LGTM: moved NotFound handling to SteveException.NotFound.
Matches new exception model; 404 mapping intact.
77-80: LGTM: BadRequest now handled via SteveException.BadRequest.
Consistent 400 mapping maintained.
| .orElseThrow(() -> new SteveException.BadRequest( | ||
| String.format("ChargeBox %s not found!", params.getChargeBoxId()))); | ||
| transactionParams.setChargePointSelectList(Collections.singletonList(chargePoint)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return 404 (Not Found) for missing resources instead of 400 (Bad Request).
ChargeBox/Transaction absence is a lookup miss; map to NotFound so ApiControllerAdvice returns 404.
- .orElseThrow(() -> new SteveException.BadRequest(
- String.format("ChargeBox %s not found!", params.getChargeBoxId())));
+ .orElseThrow(() -> new SteveException.NotFound("ChargeBox %s not found!", params.getChargeBoxId()));
@@
- .orElseThrow(() -> new SteveException.BadRequest(
- String.format("ChargeBox %s not found!", params.getChargeBoxId())));
+ .orElseThrow(() -> new SteveException.NotFound("ChargeBox %s not found!", params.getChargeBoxId()));
@@
- .orElseThrow(() -> new SteveException.BadRequest(String.format(
+ .orElseThrow(() -> new SteveException.NotFound(String.format(
"No active transaction found for connector %s at ChargeBox %s!",
params.getConnectorId(), params.getChargeBoxId())));
@@
- .orElseThrow(
- () -> new SteveException.BadRequest(String.format("Transaction %s not found!", transactionId)))
+ .orElseThrow(
+ () -> new SteveException.NotFound("Transaction %s not found!", transactionId))Also applies to: 190-192, 198-201, 204-206, 227-229
🏁 Script executed:
#!/bin/bash
# Find BadRequest uses with "not found" to convert to NotFound
rg -nP --type=java -C2 'new\s+SteveException\.BadRequest\([^)]*not found' steve-api steve-coreLength of output: 830
🏁 Script executed:
#!/bin/bash
# List all occurrences of SteveException.BadRequest to identify "not found" cases
rg -n --type=java 'new\s+SteveException\.BadRequest' -C2 steve-api steve-coreLength of output: 8618
Map missing-resource lookups to HTTP 404 Not Found
Replace all .orElseThrow(() -> new SteveException.BadRequest(... "not found" ...)) in RemoteCommandsRestController with SteveException.NotFound, so ApiControllerAdvice returns 404 instead of 400.
Affected locations:
- lines 166–168 & 189–192: ChargeBox lookup
- lines 196–200: No active transaction lookup
- lines 203–207: Transaction lookup
- lines 225–229: ChargeBox lookup
@@
- .orElseThrow(() -> new SteveException.BadRequest(String.format("ChargeBox %s not found!", params.getChargeBoxId())));
+ .orElseThrow(() -> new SteveException.NotFound("ChargeBox %s not found!", params.getChargeBoxId())));
@@
- .orElseThrow(() -> new SteveException.BadRequest(String.format(
+ .orElseThrow(() -> new SteveException.NotFound(String.format(
"No active transaction found for connector %s at ChargeBox %s!",
params.getConnectorId(), params.getChargeBoxId())));
@@
- .orElseThrow(() -> new SteveException.BadRequest(String.format("Transaction %s not found!", transactionId)))
+ .orElseThrow(() -> new SteveException.NotFound("Transaction %s not found!", transactionId))Committable suggestion skipped: line range outside the PR's diff.
| if (!ocppTag.contentEquals(params.getOcppTag())) { | ||
| throw new BadRequestException("The transaction was authorised with another OCPP Tag!"); | ||
| throw new SteveException.BadRequest("The transaction was authorised with another OCPP Tag!"); | ||
| } |
There was a problem hiding this comment.
Make OCPP tag comparison null-safe.
Avoid NPE if params.getOcppTag() is null.
- if (!ocppTag.contentEquals(params.getOcppTag())) {
+ if (!java.util.Objects.equals(ocppTag, params.getOcppTag())) {Additionally add:
+import java.util.Objects;📝 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.
| if (!ocppTag.contentEquals(params.getOcppTag())) { | |
| throw new BadRequestException("The transaction was authorised with another OCPP Tag!"); | |
| throw new SteveException.BadRequest("The transaction was authorised with another OCPP Tag!"); | |
| } | |
| // Add alongside your other imports at the top of the file | |
| import java.util.Objects; | |
| … | |
| // In RemoteCommandsRestController.java, around line 207 | |
| if (!Objects.equals(ocppTag, params.getOcppTag())) { | |
| throw new SteveException.BadRequest("The transaction was authorised with another OCPP Tag!"); | |
| } |
🤖 Prompt for AI Agents
In
steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java
around lines 207 to 209, replace the direct contentEquals call with a null-safe
comparison to avoid NPEs (e.g. use java.util.Objects.equals(ocppTag,
params.getOcppTag())); update the if to if (!Objects.equals(ocppTag,
params.getOcppTag())) { throw new SteveException.BadRequest("The transaction was
authorised with another OCPP Tag!"); } and add the import java.util.Objects at
the top of the file.
| * @since 28.08.2014 | ||
| */ | ||
| public class SteveException extends RuntimeException { | ||
| public abstract class SteveException extends RuntimeException { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Abstract base + protected ctors are a breaking change; verify no remaining direct instantiations.
Making SteveException abstract and restricting constructors to protected is good, but any new SteveException(...) will now fail. I already spotted one in ReservationsRestController (see separate comment). Please scan the codebase for others and update callers to a concrete subtype.
Also applies to: 31-57
🏁 Script executed:
#!/bin/bash
# Find direct instantiations of SteveException (will not compile now)
rg -nP -C2 '\bnew\s+SteveException\s*\('
# Also list external subclasses which may rely on removed public constructors
rg -nP -C2 '\bclass\s+\w+\s+extends\s+SteveException\b'Length of output: 33177
Replace all direct new SteveException(…) calls with concrete subclasses
Direct instantiations of SteveException now fail due to its protected constructors. Update each occurrence across modules (UI, service, repository, OCPP websocket, core, API) to throw an appropriate subtype (e.g. SteveException.AlreadyExists, NotFound, BadRequest) or introduce a new subclass.
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java around line
27, direct instantiations of SteveException across the codebase now fail because
constructors are protected; replace each "new SteveException(...)" usage with a
concrete subclass (e.g. SteveException.AlreadyExists, NotFound, BadRequest) that
matches the semantic error, or add a new specific subclass in SteveException if
none exists; update import/usages in UI, service, repository, OCPP websocket,
core, and API modules to throw the chosen concrete type and adjust any catch
blocks to handle the new subclass where necessary.
| public Ocpp16WebSocketEndpoint( | ||
| WebSocketLogger webSocketLogger, | ||
| DelegatingTaskScheduler asyncTaskScheduler, | ||
| OcppServerRepository ocppServerRepository, | ||
| FutureResponseContextStore futureResponseContextStore, | ||
| ApplicationEventPublisher applicationEventPublisher, | ||
| CentralSystemService16_SoapServer server, | ||
| Ocpp16TypeStore typeStore, | ||
| SessionContextStore sessionStore, | ||
| Sender sender, | ||
| @Qualifier("ocppObjectMapper") ObjectMapper ocppMapper, | ||
| Sender sender) { | ||
| CentralSystemService16_SoapServer ocpp16Server, | ||
| Ocpp16TypeStore ocpp16TypeStore) { | ||
| super( | ||
| webSocketLogger, | ||
| asyncTaskScheduler, | ||
| ocppServerRepository, | ||
| futureResponseContextStore, | ||
| applicationEventPublisher, | ||
| sessionStore); | ||
| var serializer = new Serializer(ocppMapper); | ||
| var deserializer = new Deserializer(ocppMapper, futureResponseContextStore, typeStore); | ||
| this.pipeline = new IncomingPipeline(serializer, deserializer, sender, new Ocpp16CallHandler(server)); | ||
| } | ||
|
|
||
| @PostConstruct | ||
| public void init() { | ||
| super.init(pipeline); | ||
| sessionStore, | ||
| sender, | ||
| ocppMapper, | ||
| ocpp16TypeStore, | ||
| new Ocpp16CallHandler(ocpp16Server)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Matches new AbstractWebSocketEndpoint constructor; check DI consistency.
Wiring mirrors OCPP 1.5 changes; ensure the Ocpp16TypeStore bean and CentralSystemService16_SoapServer name match configuration.
🏁 Script executed:
#!/bin/bash
# Confirm ocpp16 beans exist
rg -nP 'class\s+Ocpp16TypeStore\b|@Bean\s*\(\)\s*Ocpp16TypeStore' -C2
rg -nP 'class\s+CentralSystemService16_SoapServer\b|@Bean.*CentralSystemService16_SoapServer' -C2Length of output: 1486
Register CentralSystemService16_SoapServer as a Spring bean
CentralSystemService16_SoapServer (steve-ocpp-soap/src/main/java/de/rwth/idsg/steve/ocpp/soap/CentralSystemService16_SoapServer.java) isn’t annotated or defined as a @bean—add @component or declare it in a @configuration so DI can provide it to Ocpp16WebSocketEndpoint.
🤖 Prompt for AI Agents
In
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java
around lines 57 to 79, the constructor requires
CentralSystemService16_SoapServer but that class isn't currently a Spring bean;
register it so DI can inject it by either annotating the
CentralSystemService16_SoapServer class with @Component (or @Service) and ensure
its package is component-scanned, or create a @Configuration class with a @Bean
method that returns an instance of CentralSystemService16_SoapServer (wiring its
dependencies there); after registering the bean, recompile and verify the
application context injects CentralSystemService16_SoapServer into
Ocpp16WebSocketEndpoint without errors.
| private final Consumer<CommunicationContext> handler; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against handler exceptions; always attempt a response/log on failure.
If handler.accept(context) throws, the client receives no reply and the connection might be left in an inconsistent state. Wrap handler execution to avoid propagating, and still try to serialize/send whatever the handler may have set.
- case OcppJsonCall ocppJsonCall -> {
- handler.accept(context);
- send(context);
- }
+ case OcppJsonCall ocppJsonCall -> {
+ try {
+ handler.accept(context);
+ } catch (RuntimeException e) {
+ log.error("Incoming handler failed for {}", context.getChargeBoxId(), e);
+ // Optionally, consider setting an internal error into the context here if your
+ // Serializer expects an outgoing message/error.
+ }
+ send(context);
+ }Also applies to: 57-60
🤖 Prompt for AI Agents
In
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java
around lines 43-44 (and similarly lines 57-60), the call to
handler.accept(context) is unguarded and may throw, leaving the client without a
reply and the connection in an inconsistent state; wrap the handler invocation
in a try-catch that catches Throwable/Exception, log the error with context
details, do not rethrow, and ensure execution continues to the
serialization/send path so you still attempt to serialize and send any response
the handler set (or send a sensible error/fallback response if none was set) and
perform any necessary cleanup/close in a finally or after the catch to keep
connection state consistent.
e8bbf94 to
6453add
Compare
juherr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ApiControllerAdvice.java (1)
60-66: Swapping to SteveException.NotFound/BadRequest is correct; add explicit InternalError handler and avoid leaking 500 messages.
Currently, 500s echo exception.getMessage(); prefer a generic message and a dedicated handler for SteveException.InternalError.Add an explicit InternalError handler (outside the shown ranges):
@ExceptionHandler(SteveException.InternalError.class) @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR) public ApiErrorResponse handleInternalError(HttpServletRequest req, SteveException.InternalError ex) { String url = req.getRequestURL().toString(); log.error("Request: {} raised following exception.", url, ex); return createResponse(url, HttpStatus.INTERNAL_SERVER_ERROR, "Internal server error"); }Also, sanitize the catch‑all:
- return createResponse(url, HttpStatus.INTERNAL_SERVER_ERROR, exception.getMessage()); + return createResponse(url, HttpStatus.INTERNAL_SERVER_ERROR, "Internal server error");Also applies to: 77-83
♻️ Duplicate comments (3)
steve/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (1)
132-132: Confirm InternalError overload sets the cause.Same constructor/cause concern as noted elsewhere; ensure stack traces are preserved.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (1)
145-148: Addressed: fail fast on missing chargeBoxId.
Matches prior review guidance; closes with 1008 and returns.steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
27-27: Abstract base + protected ctors: verify repo-wide breakages.Making SteveException abstract with protected ctors is a breaking change; ensure no remaining direct instantiations and no usages of removed top-level exceptions persist.
Run:
#!/usr/bin/env bash set -euo pipefail echo "Direct instantiations of SteveException (should be 0):" rg -nP -C2 '\bnew\s+SteveException\s*\(' echo echo "Legacy top-level exceptions (should be 0):" rg -nP -C2 '\b(new|catch\s*\()\s*(NotFoundException|BadRequestException)\b' echo echo "Subclasses of SteveException (review ctor calls after ctor visibility change):" rg -nP -C2 '\bclass\s+\w+\s+extends\s+SteveException\b'
🧹 Nitpick comments (22)
steve-core/src/main/java/de/rwth/idsg/steve/service/UsersService.java (1)
50-52: Improve InternalError message with the created ID for actionable diagnosticsInclude the created user ID and avoid “this should never happen” phrasing.
- .orElseThrow(() -> - new SteveException.InternalError("User not found after creation, this should never happen")); + .orElseThrow(() -> + new SteveException.InternalError(String.format( + "Inconsistent state: created user id=%d but getDetails() returned empty", id)));steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/InvocationContext.java (1)
38-43: Fail fast on null RequestType and enrich error messageAdd a null check for requestType and include context (chargeBoxId, task) in the error to aid debugging. Also tighten the wording (“Action not found …”).
var requestType = request().apply(task); var pair = typeStore().findActionResponse(requestType); - if (pair == null) { - throw new SteveException.InternalError("Action name is not found"); - } + if (requestType == null) { + throw new SteveException.InternalError(String.format( + "RequestType mapper returned null (task=%s, chargeBoxId=%s)", + task == null ? "null" : task.getClass().getName(), chargeBoxId)); + } + if (pair == null) { + throw new SteveException.InternalError(String.format( + "Action not found for requestType=%s (class=%s), chargeBoxId=%s", + requestType, requestType.getClass().getName(), chargeBoxId)); + }steve/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java (1)
277-277: Improve error message to include the enum value; consider null safety.Include the problematic value to aid debugging; also verify
form.getPeriodType()cannot be null before the switch.- default -> throw new SteveException.InternalError("Unknown enum type"); + default -> throw new SteveException.InternalError("Unknown enum type: " + form.getPeriodType());steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (2)
191-191: Include the offending enum in the error for faster diagnostics.Surface the actual heartbeat period value to aid debugging.
- default -> throw new SteveException.InternalError("Unknown enum type"); + default -> throw new SteveException.InternalError( + "Unknown heartbeat period: %s", form.getHeartbeatPeriod());
337-338: Add chargeBoxPk to the delete failure message.Helps correlate logs with inputs.
- throw new SteveException.InternalError("Failed to delete the charge point", e); + throw new SteveException.InternalError( + "Failed to delete the charge point with id '%d'", chargeBoxPk, e);steve/src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java (1)
90-92: Simplify removal with removeIf for clarity and performance.Replacing stream+remove with removeIf avoids extra lookups and is idiomatic for ConcurrentHashMap views.
- lookupTable.entrySet().stream().filter(filterPredicate).forEach(entry -> lookupTable.remove(entry.getKey())); + lookupTable.entrySet().removeIf(filterPredicate);steve/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
382-383: Unknown period type: consider BadRequest (input) vs InternalError (state).If period type originates from user input (web/API), BadRequest better reflects a client error. If it’s purely internal, InternalError is fine. Please confirm origin.
- default -> throw new SteveException.InternalError("Unknown enum type"); + default -> throw new SteveException.BadRequest("Unknown enum type: %s", form.getPeriodType());steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java (5)
81-82: Clarify error text for unknown message type."Unknown enum type" is vague. Include the numeric/type info to speed diagnostics.
- default -> throw new SteveException.InternalError("Unknown enum type"); + default -> throw new SteveException.InternalError( + "Unknown OCPP message type: typeNr=%s", messageTypeNr);
84-86: Avoid embedding full incoming payload in exception; risk of PII/log bloat.Consider truncating the payload or logging it at DEBUG separately, and keep the exception concise.
- throw new SteveException.InternalError( - "Deserialization of incoming string failed: %s", context.getIncomingString(), e); + throw new SteveException.InternalError("Deserialization of incoming string failed", e);
148-151: Same here: cap or omit raw message in exception.Large/PII-rich payloads should not be embedded in exception text.
- throw new SteveException.InternalError( - "A result message was received as response to a not-sent call. The message was: %s", - context.getIncomingString()); + throw new SteveException.InternalError( + "A result message was received as response to a not-sent call");
177-180: Same concern: avoid including full incoming payload in exception.Prefer concise message; log payload separately at DEBUG if needed.
- throw new SteveException.InternalError( - "An error message was received as response to a not-sent call. The message was: %s", - context.getIncomingString()); + throw new SteveException.InternalError( + "An error message was received as response to a not-sent call");
145-151: Consider logging session context for mismatched result/error.Adding chargeBoxId/sessionId to logs (not exception) eases triage of out-of-order or orphaned responses.
Would you like a follow-up patch to add guarded DEBUG logs with session.getId() and a short payload preview?
steve/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (3)
114-125: Include the unexpected enum value in the error messageHelps diagnosis when clients send an out-of-range value.
- default -> throw new SteveException.InternalError("Unknown enum type"); + default -> throw new SteveException.InternalError( + "Unknown enum value for 'expired': %s", form.getExpired());
250-252: Use ocppTagPk in update error message to avoid null/ambiguous idTagThe WHERE clause uses ocppTagPk; prefer that identifier in diagnostics.
- throw new SteveException.InternalError( - "Execution of updateOcppTag for idTag '%s' FAILED.", u.getIdTag(), e); + throw new SteveException.InternalError( + "Execution of updateOcppTag for ocppTagPk %d FAILED.", u.getOcppTagPk(), e);
259-261: Fix message: refers to idTag but the parameter is ocppTagPkAlign the message with the method argument.
- throw new SteveException.InternalError("Execution of deleteOcppTag for idTag FAILED.", e); + throw new SteveException.InternalError( + "Execution of deleteOcppTag for ocppTagPk %d FAILED.", ocppTagPk, e);steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java (1)
61-63: Include the created id in the InternalError messageImproves traceability in logs.
- .orElseThrow(() -> new SteveException.InternalError( - "Reservation not found after creation, this should never happen")); + .orElseThrow(() -> new SteveException.InternalError( + "Reservation %d not found after creation, this should never happen", id));steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java (1)
57-63: Enrich error context with chargeBoxId and standardize loggingAdd chargeBoxId to both the thrown message (CALL) and the log line (non-CALL).
- var msg = "Failed to send outgoing " + call.getAction() + " CALL over WebSocket"; - throw new SteveException.InternalError(msg, e); + var msg = String.format( + "Failed to send outgoing %s CALL over WebSocket for chargeBoxId '%s'", + call.getAction(), chargeBoxId); + throw new SteveException.InternalError(msg, e); } else { - log.error("Failed to send outgoing message over WebSocket", e); + log.error("Failed to send outgoing message over WebSocket for chargeBoxId {}", + chargeBoxId, e); }steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
63-88: InternalError: constructor symmetry, serialVersionUID, and minor dedup.
- Expose 2-arg+cause ctor publicly for parity.
- Add serialVersionUID for nested type.
- Prefer calling super(template, …) to avoid re-formatting here.
public static class InternalError extends SteveException { - - public InternalError(String message) { - super(message); - } - - public InternalError(String message, Throwable cause) { - super(message, cause); - } - - public InternalError(String template, Object arg1) { - this(format(template, arg1)); - } - - public InternalError(String template, Object arg1, Throwable cause) { - this(format(template, arg1), cause); - } - - public InternalError(String template, Object arg1, Object arg2) { - this(format(template, arg1, arg2)); - } - - protected InternalError(String template, Object arg1, Object arg2, Throwable cause) { - this(format(template, arg1, arg2), cause); - } + private static final long serialVersionUID = 1L; + + public InternalError(String message) { super(message); } + public InternalError(String message, Throwable cause) { super(message, cause); } + public InternalError(String template, Object arg1) { super(template, arg1); } + public InternalError(String template, Object arg1, Throwable cause) { super(template, arg1, cause); } + public InternalError(String template, Object arg1, Object arg2) { super(template, arg1, arg2); } + public InternalError(String template, Object arg1, Object arg2, Throwable cause) { super(template, arg1, arg2, cause); } }steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (4)
267-267: Add supporting DB index for ordered fetchConsider a composite index on CHARGING_SCHEDULE_PERIOD(CHARGING_PROFILE_PK, START_PERIOD_IN_SECONDS) to back the ORDER BY and frequent lookups.
309-309: Enrich error context on add()Include identifying context (e.g., description/stackLevel) to aid diagnostics.
Apply:
- throw new SteveException.InternalError("Failed to add the charging profile", e); + throw new SteveException.InternalError( + String.format("Failed to add charging profile [desc=%s, stackLevel=%s]", + form.getDescription(), form.getStackLevel()), + e + );
382-384: Use a non-internal error for business-rule violation + safe formattingThis is a user/action conflict, not an internal error. If a domain/validation/conflict SteveException exists, use it; otherwise at least avoid ambiguous formatting.
Apply safe formatting:
- throw new SteveException.InternalError( - "Cannot modify this charging profile, since the following stations are still using it: %s", r); + throw new SteveException.InternalError( + String.format("Cannot modify this charging profile; still used by stations: %s", r) + );If available, prefer:
- SteveException.Conflict / SteveException.Validation with same message.
388-399: Validate/normalize schedule periods before insertConsider rejecting negative start periods and duplicate startPeriodInSeconds early to avoid DB errors and nondeterminism.
Example:
- var r = form.getSchedulePeriods().stream() + var r = form.getSchedulePeriods().stream() + .filter(p -> p.getStartPeriodInSeconds() >= 0) .sorted(Comparator.comparingInt(ChargingProfileForm.SchedulePeriod::getStartPeriodInSeconds))If duplicates are possible, enforce uniqueness upstream or via a DB UNIQUE constraint on (CHARGING_PROFILE_PK, START_PERIOD_IN_SECONDS).
📜 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 (38)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ApiControllerAdvice.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java(6 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/TasksRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/TransactionsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/BadRequestException.java(0 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/NotFoundException.java(0 hunks)steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java(2 hunks)steve-api/src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java(1 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/service/UsersService.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ChangeConfigurationParams.java(1 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java(6 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/InvocationContext.java(1 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreImpl.java(1 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp12/Ocpp12WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp15/Ocpp15WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java(2 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java(5 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java(1 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java(3 hunks)steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Serializer.java(2 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java(2 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/AddressRepositoryImpl.java(2 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java(4 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(5 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java(4 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java(1 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java(2 hunks)steve/src/main/java/de/rwth/idsg/steve/repository/impl/UserRepositoryImpl.java(4 hunks)steve/src/main/java/de/rwth/idsg/steve/service/MailServiceDefault.java(1 hunks)
💤 Files with no reviewable changes (2)
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/NotFoundException.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/exception/BadRequestException.java
🚧 Files skipped from review as they are similar to previous changes (7)
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/TransactionsRestController.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/RemoteCommandsRestController.java
- steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/TasksRestController.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/OcppTagsRestController.java
- steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ocpp16/Ocpp16WebSocketEndpoint.java
- steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java (1)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/data/CommunicationContext.java (2)
RequiredArgsConstructor(41-116)RequiredArgsConstructor(82-115)
⏰ 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). (20)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
🔇 Additional comments (38)
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/InvocationContext.java (2)
42-42: Good move to InternalErrorThrowing SteveException.InternalError here correctly classifies a server-side mapping/config error.
35-55: Ensure callers aren’t expecting the old exception type
No catch blocks for SteveException.InternalError or IllegalStateException were found—manually verify no upstream code assumes the previous exception.steve/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (1)
78-81: LGTM: InternalError on TZ mismatch is appropriate.Consistent with the new exception hierarchy; message already includes both offsets.
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java (1)
99-101: Unsupported task type as InternalError is fine; also guard null task.Switch on a null
taskwill NPE. Add a pre-check and return NotFound for missing tasks.// Add before the switch: if (task == null) { throw new SteveException.NotFound("Task not found: id=" + taskId); }steve/src/main/java/de/rwth/idsg/steve/service/MailServiceDefault.java (1)
64-65: LGTM: Wrap MessagingException as InternalError.Message is generic (no secrets) and preserves the cause.
steve/src/main/java/de/rwth/idsg/steve/repository/impl/UserRepositoryImpl.java (5)
104-105: LGTM: Internal DB failure mapped to InternalError.Correctly ensures transaction rollback and consistent error semantics.
119-120: LGTM: Update failure mapped to InternalError.Consistent with repository-wide error handling.
132-133: LGTM: Delete failure mapped to InternalError.Keeps behavior uniform across CRUD paths.
234-235: LGTM: No-returning record treated as InternalError.Appropriate invariant check for an insert-returning flow.
238-239: LGTM: Insert failure mapped to InternalError.Consistent with the new exception hierarchy.
steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (1)
307-309: No change needed: cause is preserved by correct InternalError overload
Verified thatInternalError(String template, Object arg1, Throwable cause)delegates tosuper(formattedMessage, cause), so the passed exception isn’t formatted into the message and the stack‐trace is retained.steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
244-245: LGTM: NotFound is the right classification here.Switching to
SteveException.NotFoundfor missing users aligns with the new exception taxonomy.Also applies to: 259-260
steve/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
349-351: LGTM: Internal error on unexpected null id.Appropriate safeguard; jOOQ should return a key, but this ensures a clear failure mode.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreImpl.java (1)
139-139: InternalError constructor with (String, Object, Throwable) confirmed; the cause will be attached correctly.steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ocpp/ChangeConfigurationParams.java (1)
77-78: LGTM: InternalError on illegal KeyType is consistent with project taxonomy.No further changes needed.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java (2)
159-160: LGTM: preserve cause for payload deserialization failures.Exception wrapping is correct.
208-209: LGTM: cause propagation retained for error-message deserialization.No changes needed.
steve/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (1)
233-235: LGTM: InternalError preserves context and cause on add failureMessage includes idTag and propagates the original exception.
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java (1)
74-75: LGTM: 404 mapped to SteveException.NotFoundConsistent with the new exception model; aligns with controller advice.
steve-api/src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java (2)
364-367: LGTM: test uses InternalError to simulate 5xx pathKeeps the contract while aligning with the new exception type.
416-418: LGTM: delete 5xx simulation updated to InternalErrorConsistent with controller error handling.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Sender.java (2)
49-52: LGTM: switch to injected WebSocketLoggerRemoves static coupling and eases testing.
39-43: DI wiring for Sender is correctly configured. OcppConfiguration declares a @bean for Sender(WebSocketLogger), and WebSocketLogger is annotated with @component.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Serializer.java (2)
67-67: Use of SteveException.InternalError on serialization failure looks correct.
Aligns with internal-failure semantics; upstream can uniformly treat it as 500-class.
84-85: Consistent InternalError on payload conversion failure.
Good replacement; matches project-wide migration to nested SteveException types.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/AbstractWebSocketEndpoint.java (7)
71-99: Constructor-based wiring of IncomingPipeline is solid.
Thread-safe reuse of ObjectMapper here is fine; event callbacks correctly registered.
124-129: Text handling + logging LGTM.
Empty payload guard and logging are appropriate.
136-140: PONG handling LGTM.
Heartbeat update on pong is appropriate.
180-181: Close logging LGTM.
Uses instance logger; consistent format.
200-201: Transport error logging LGTM.
Uniform error path via WebSocketLogger.
59-69: Approved: CHARGEBOX_ID_KEY is set in OcppWebSocketHandshakeHandler (line 96) and retrieved in AbstractWebSocketEndpoint (line 213), confirming consistent use.
150-159: LGTM: Ping scheduling wiring is correct.steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
31-57: Base constructors/readability: LGTM.Protected overloads + format helpers are clean and prevent ad‑hoc base instantiation.
steve/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (5)
41-44: Imports for Comparator/Optional — OKThese additions match the new usages below.
301-301: Fix returning PK target — good catchReturning CHARGING_PROFILE.CHARGING_PROFILE_PK here is correct for retrieving the profile PK.
256-270: Resolved: Optional return and DB ordering verified
ChargingProfileRepositoryinterface declaresOptional<ChargingProfile.Details> getDetails(...), the implementation returnsOptional.empty()when absent and orders schedule periods ascending, and all callers (e.g.ChargingProfilesController,ChargePointServiceClient) handle the empty case via.orElseThrow(...).
359-361: Constructor overload is unambiguous; cause is preserved
The call InternalError(String, Object, Throwable) matches the three-arg constructor, which formats the message then delegates to super(message, cause). The exception’s cause is not dropped—no change needed.Likely an incorrect or invalid review comment.
392-399: Stream.toList() is supported with Java 21
The compiler is configured with<release>${java.version}</release>andjava.versionis set to21in the parent POM, so the code is compiled under Java 21 (which includes Stream.toList() since Java 16). No change needed.Likely an incorrect or invalid review comment.
| throw new SteveException.InternalError( | ||
| "ChargePointMaxProfile can only be set at Charge Point ConnectorId 0"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use BadRequest for spec-violating inputs instead of InternalError.
These are client/request validation errors; surfacing them as 400 aligns API semantics and avoids 500s.
- throw new SteveException.InternalError(
+ throw new SteveException.BadRequest(
"ChargePointMaxProfile can only be set at Charge Point ConnectorId 0");
...
- throw new SteveException.InternalError("TxProfile should only be set at Charge Point ConnectorId > 0");
+ throw new SteveException.BadRequest("TxProfile should only be set at Charge Point ConnectorId > 0");Also applies to: 65-65
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTask.java
around lines 60-61 (and the similar occurrence at line 65), replace the thrown
SteveException.InternalError for spec-violating inputs with the client-side 400
variant by throwing SteveException.BadRequest (or the project's BadRequest
variant) using the same descriptive message; ensure both locations use
BadRequest so these validation failures surface as HTTP 400 instead of 500.
| .orElseThrow(() -> | ||
| new SteveException.InternalError("User not found after update, this should never happen")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use NotFound (4xx) instead of InternalError (5xx) when user is missing after update
A missing user after update is a resource/state issue (invalid userPk or concurrent delete), not an internal failure.
- .orElseThrow(() ->
- new SteveException.InternalError("User not found after update, this should never happen"));
+ .orElseThrow(() ->
+ new SteveException.NotFound(String.format(
+ "User with id %d not found for update", form.getUserPk())));📝 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.
| .orElseThrow(() -> | |
| new SteveException.InternalError("User not found after update, this should never happen")); | |
| .orElseThrow(() -> | |
| new SteveException.NotFound(String.format( | |
| "User with id %d not found for update", form.getUserPk()))); |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/service/UsersService.java around
lines 61-62, the code throws SteveException.InternalError when the user is
missing after an update; change this to throw the appropriate 4xx NotFound
variant (e.g., SteveException.NotFound) with a clear message like "User not
found after update" so the response reflects a client/resource error rather than
a server/internal error.
| public static class AlreadyExists extends SteveException { | ||
|
|
||
| public AlreadyExists(String template, Object arg1) { | ||
| super(format(template, arg1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AlreadyExists: add message/cause ctors and serialVersionUID; use super(template,…).
Only having (template, arg1) limits cause propagation and message-only use.
public static class AlreadyExists extends SteveException {
-
- public AlreadyExists(String template, Object arg1) {
- super(format(template, arg1));
- }
+ private static final long serialVersionUID = 1L;
+
+ public AlreadyExists(String message) { super(message); }
+ public AlreadyExists(String template, Object arg1) { super(template, arg1); }
+ public AlreadyExists(String message, Throwable cause) { super(message, cause); }
+ public AlreadyExists(String template, Object arg1, Throwable cause) { super(template, arg1, cause); }
}📝 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 static class AlreadyExists extends SteveException { | |
| public AlreadyExists(String template, Object arg1) { | |
| super(format(template, arg1)); | |
| } | |
| } | |
| public static class AlreadyExists extends SteveException { | |
| private static final long serialVersionUID = 1L; | |
| public AlreadyExists(String message) { | |
| super(message); | |
| } | |
| public AlreadyExists(String template, Object arg1) { | |
| super(template, arg1); | |
| } | |
| public AlreadyExists(String message, Throwable cause) { | |
| super(message, cause); | |
| } | |
| public AlreadyExists(String template, Object arg1, Throwable cause) { | |
| super(template, arg1, cause); | |
| } | |
| } |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java around lines
90-95, the AlreadyExists subclass only has a single (template, arg1) ctor and
lacks serialVersionUID; add a private static final long serialVersionUID and
implement additional constructors: (String template) and (String template,
Throwable cause) (and keep the existing one but change it to delegate to
super(template, arg1) rather than pre-formatting), ensuring each ctor calls the
appropriate super(...) overload so message-only and message+cause use-cases and
cause propagation are supported.
| public static class NotFound extends SteveException { | ||
|
|
||
| public NotFound(String message) { | ||
| super(message); | ||
| } | ||
|
|
||
| public NotFound(String template, Object arg1) { | ||
| this(format(template, arg1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NotFound: add cause-enabled ctors + serialVersionUID; dedup to super(template,…).
Without cause ctors, stack traces may be lost; add them and keep formatting centralized.
- public static class NotFound extends SteveException {
-
- public NotFound(String message) {
- super(message);
- }
-
- public NotFound(String template, Object arg1) {
- this(format(template, arg1));
- }
- }
+ public static class NotFound extends SteveException {
+ private static final long serialVersionUID = 1L;
+
+ public NotFound(String message) { super(message); }
+ public NotFound(String message, Throwable cause) { super(message, cause); }
+ public NotFound(String template, Object arg1) { super(template, arg1); }
+ public NotFound(String template, Object arg1, Throwable cause) { super(template, arg1, cause); }
+ }📝 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 static class NotFound extends SteveException { | |
| public NotFound(String message) { | |
| super(message); | |
| } | |
| public NotFound(String template, Object arg1) { | |
| this(format(template, arg1)); | |
| } | |
| } | |
| public static class NotFound extends SteveException { | |
| private static final long serialVersionUID = 1L; | |
| public NotFound(String message) { | |
| super(message); | |
| } | |
| public NotFound(String message, Throwable cause) { | |
| super(message, cause); | |
| } | |
| public NotFound(String template, Object arg1) { | |
| super(template, arg1); | |
| } | |
| public NotFound(String template, Object arg1, Throwable cause) { | |
| super(template, arg1, cause); | |
| } | |
| } |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java around lines
97-106, the NotFound subclass should add a serialVersionUID and cause-aware
constructors, and delegate formatting to the superclass to avoid duplication:
add a private static final long serialVersionUID, replace the template-based
ctor that calls this(format(...)) with a call to super(template, arg1) (or
super(template, args...) if varargs exist), and add overloads accepting (String
message, Throwable cause) and (String template, Throwable cause, Object... args)
that pass the cause up to super so stack traces are preserved. Ensure signatures
match SteveException's existing constructors and remove the local format call to
centralize formatting in the base class.
| public static class BadRequest extends SteveException { | ||
| public BadRequest(String message) { | ||
| super(message); | ||
| } | ||
|
|
||
| public BadRequest(String template, Object arg1) { | ||
| this(format(template, arg1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
BadRequest: add cause-enabled ctors + serialVersionUID; dedup to super(template,…).
Mirror NotFound to preserve causal chains and consistency.
- public static class BadRequest extends SteveException {
- public BadRequest(String message) {
- super(message);
- }
-
- public BadRequest(String template, Object arg1) {
- this(format(template, arg1));
- }
- }
+ public static class BadRequest extends SteveException {
+ private static final long serialVersionUID = 1L;
+
+ public BadRequest(String message) { super(message); }
+ public BadRequest(String message, Throwable cause) { super(message, cause); }
+ public BadRequest(String template, Object arg1) { super(template, arg1); }
+ public BadRequest(String template, Object arg1, Throwable cause) { super(template, arg1, cause); }
+ }📝 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 static class BadRequest extends SteveException { | |
| public BadRequest(String message) { | |
| super(message); | |
| } | |
| public BadRequest(String template, Object arg1) { | |
| this(format(template, arg1)); | |
| } | |
| } | |
| public static class BadRequest extends SteveException { | |
| private static final long serialVersionUID = 1L; | |
| public BadRequest(String message) { | |
| super(message); | |
| } | |
| public BadRequest(String message, Throwable cause) { | |
| super(message, cause); | |
| } | |
| public BadRequest(String template, Object arg1) { | |
| super(template, arg1); | |
| } | |
| public BadRequest(String template, Object arg1, Throwable cause) { | |
| super(template, arg1, cause); | |
| } | |
| } |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java around lines
108 to 116, the BadRequest subclass is missing a serialVersionUID, lacks
constructors that accept a cause, and its templated constructor delegates to
this(format(...)) instead of calling the corresponding super(template, ...);
update BadRequest to match the NotFound pattern by adding a private static final
long serialVersionUID, adding constructors public BadRequest(String message,
Throwable cause) and public BadRequest(String template, Object arg1, Throwable
cause) (and a varargs templated overload if NotFound has one), and change the
existing templated ctor to delegate directly to super(template, arg1) (i.e.,
call the superclass template-formatting constructor) so causal chains are
preserved and code is consistent.
| throw new SteveException.InternalError("Result not found for chargeBoxId '" + chargeBoxId + "'"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer NotFound over InternalError when result is absent.
Missing entry for a user-supplied chargeBoxId is not an internal failure.
- throw new SteveException.InternalError("Result not found for chargeBoxId '" + chargeBoxId + "'");
+ throw new SteveException.NotFound("Result not found for chargeBoxId '" + chargeBoxId + "'");📝 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.
| throw new SteveException.InternalError("Result not found for chargeBoxId '" + chargeBoxId + "'"); | |
| } | |
| throw new SteveException.NotFound("Result not found for chargeBoxId '" + chargeBoxId + "'"); | |
| } |
🤖 Prompt for AI Agents
In
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/TasksController.java
around lines 124 to 125, the code throws SteveException.InternalError when a
result for a user-supplied chargeBoxId is missing; change this to throw
SteveException.NotFound with an appropriate message (e.g., "Result not found for
chargeBoxId '...'" ) so the absence of a resource is represented as a 404-level
error rather than an internal server error; ensure any callers or exception
handlers already map NotFound to the correct HTTP 404 response.
| } catch (DataAccessException e) { | ||
| throw new SteveException("Failed to insert the address"); | ||
| throw new SteveException.InternalError("Failed to insert the address"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve root cause on insert failure and guard against null RETURNING row
Wrap and propagate the original DataAccessException; also handle a potential null from fetchOne() to avoid NPEs.
- } catch (DataAccessException e) {
- throw new SteveException.InternalError("Failed to insert the address");
- }
+ } catch (DataAccessException e) {
+ throw new SteveException.InternalError("Failed to insert the address", e);
+ }Optionally, make the RETURNING handling explicit:
- .returning(ADDRESS.ADDRESS_PK)
- .fetchOne()
- .getAddressPk();
+ .returning(ADDRESS.ADDRESS_PK)
+ .fetchOne();
+ if (rec == null) {
+ throw new SteveException.InternalError("Failed to insert the address: no RETURNING row");
+ }
+ return rec.getAddressPk();📝 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.
| } catch (DataAccessException e) { | |
| throw new SteveException("Failed to insert the address"); | |
| throw new SteveException.InternalError("Failed to insert the address"); | |
| } | |
| } | |
| } catch (DataAccessException e) { | |
| throw new SteveException.InternalError("Failed to insert the address", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
steve/src/main/java/de/rwth/idsg/steve/repository/impl/AddressRepositoryImpl.java
around lines 100 to 103, the catch currently swallows the DataAccessException
and assumes fetchOne() returned a non-null row; change it to preserve the
original exception as the cause when throwing SteveException.InternalError (e.g.
pass 'e' into the new exception) and add a null-check for the result of
fetchOne() so you handle a null RETURNING row explicitly (throw a clear
SteveException or return an Optional/appropriate error) instead of risking an
NPE.
| if (count != 1) { | ||
| throw new SteveException("Failed to update the address"); | ||
| throw new SteveException.InternalError("Failed to update the address"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Differentiate “not found” vs “unexpected row count” on update
If count == 0, the PK doesn’t exist (404). If count > 1, that’s an internal integrity issue (500).
- if (count != 1) {
- throw new SteveException.InternalError("Failed to update the address");
- }
+ if (count == 0) {
+ throw new SteveException.NotFound("Address with id %d not found", ad.getAddressPk());
+ } else if (count != 1) {
+ throw new SteveException.InternalError(
+ "Updated %d rows for address id %d, expected 1", count, ad.getAddressPk());
+ }📝 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.
| if (count != 1) { | |
| throw new SteveException("Failed to update the address"); | |
| throw new SteveException.InternalError("Failed to update the address"); | |
| } | |
| if (count == 0) { | |
| throw new SteveException.NotFound( | |
| "Address with id %d not found", ad.getAddressPk()); | |
| } else if (count != 1) { | |
| throw new SteveException.InternalError( | |
| "Updated %d rows for address id %d, expected 1", | |
| count, ad.getAddressPk()); | |
| } |
🤖 Prompt for AI Agents
In
steve/src/main/java/de/rwth/idsg/steve/repository/impl/AddressRepositoryImpl.java
around lines 115-117, differentiate the update result: if count == 0, throw a
404-style exception (e.g. SteveException.NotFound with a message like "Address
not found for id ..."); if count > 1, throw the existing internal error
(SteveException.InternalError) with a clear message about the unexpected row
count (e.g. "Unexpected row count >1 when updating address"). Ensure the
exception types and messages make the distinction explicit and include the
address id when available.
| CommunicationTask r = lookupTable.get(taskId); | ||
| if (r == null) { | ||
| throw new SteveException("There is no task with taskId '%s'", taskId); | ||
| throw new SteveException.InternalError("There is no task with taskId '%s'", taskId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use NotFound for missing task rather than InternalError.
A non-existent taskId is a “not found” outcome, not an internal failure. Prefer SteveException.NotFound for clearer semantics and consistent API mapping.
- throw new SteveException.InternalError("There is no task with taskId '%s'", taskId);
+ throw new SteveException.NotFound("There is no task with taskId '%s'", taskId);📝 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.
| throw new SteveException.InternalError("There is no task with taskId '%s'", taskId); | |
| throw new SteveException.NotFound("There is no task with taskId '%s'", taskId); |
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java
around line 67, the code throws SteveException.InternalError for a missing
taskId; change this to throw SteveException.NotFound with the same message and
taskId so the API semantics correctly signal a missing resource, and add or
adjust the import if necessary to reference SteveException.NotFound.
|
|
||
| if (transaction == null) { | ||
| throw new SteveException("There is no transaction with id '%s'", transactionPk); | ||
| throw new SteveException.InternalError("There is no transaction with id '%s'", transactionPk); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return NotFound for absent transaction.
Absence of a transaction id should be classified as NotFound to align with repository semantics and potential 404 mapping at higher layers.
- throw new SteveException.InternalError("There is no transaction with id '%s'", transactionPk);
+ throw new SteveException.NotFound("There is no transaction with id '%s'", transactionPk);📝 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.
| throw new SteveException.InternalError("There is no transaction with id '%s'", transactionPk); | |
| @@ steve/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java:147 | |
| throw new SteveException.NotFound("There is no transaction with id '%s'", transactionPk); |
🤖 Prompt for AI Agents
In
steve/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl
around line 147, the code throws a SteveException.InternalError when a
transaction id is absent; change this to throw the appropriate
SteveException.NotFound (or the project's NotFound variant) with the same
formatted message including transactionPk so the absence is classified as
NotFound and correctly maps to a 404 at higher layers.
1 participant
Backport steve-community#1791
Summary by CodeRabbit
New Features
Bug Fixes
Refactor