validate StartTransaction.meterStart against previous meterStop (part…#1989
Open
rishabhvaish wants to merge 2 commits into
Open
validate StartTransaction.meterStart against previous meterStop (part…#1989rishabhvaish wants to merge 2 commits into
rishabhvaish wants to merge 2 commits into
Conversation
Review Summary by QodoValidate StartTransaction meter against previous transaction stop value
WalkthroughsDescription• Add meter regression validation to StartTransaction • Query last transaction stop value from repository • Compare meterStart against previous meterStop value • Log warning when meter value decreases between transactions Diagramflowchart LR
A["StartTransaction Request"] --> B["Query Last Stop Value"]
B --> C["Validate Meter Regression"]
C --> D{Meter Start >= Previous Stop?}
D -->|Yes| E["Transaction Allowed"]
D -->|No| F["Return Error Exception"]
File Changes1. src/main/java/de/rwth/idsg/steve/repository/OcppServerRepository.java
|
Code Review by Qodo
1. StartTransaction inserts despite validation
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+233
to
239
| String previousStopValue = ocppServerRepository.getLastTransactionStopValue( | ||
| chargeBoxIdentity, parameters.getConnectorId() | ||
| ); | ||
| var exception = serviceValidator.validateStart(parameters, previousStopValue); | ||
| if (exception != null) { | ||
| log.warn("StartTransaction validation failed", exception); | ||
| } |
There was a problem hiding this comment.
1. Starttransaction inserts despite validation 📎 Requirement gap ⛯ Reliability
Even when validateStart(...) returns an exception (including meter regression), the code only logs a warning and still calls insertTransaction, so the event is not flagged/recorded as suspect and side effects still occur. This violates the requirement to mark meter regression as suspect and to avoid side effects after validation failure.
Agent Prompt
## Issue description
`startTransaction(...)` continues with `insertTransaction(...)` even when `validateStart(...)` returns a non-null exception (including the new meter-regression exception). This means the event is not actually flagged/recorded as suspect, and DB writes occur after validation failure.
## Issue Context
The meter regression check currently returns a `SteveException`, but the caller only logs and proceeds. To meet compliance, the system must deterministically record/flag the StartTransaction as suspect (or otherwise prevent normal processing) and avoid normal state mutation after validation failure.
## Fix Focus Areas
- src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_Service.java[233-242]
- src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[69-71]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
already handled — dcc77ccb gates side effects on validation result so insertTransaction is skipped when validateStart returns an exception
Wrap Integer.parseInt(previousStopValue) in a try-catch to gracefully handle corrupted or non-numeric stop values from the database instead of throwing NumberFormatException at runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rishabh Vaish <rishabhvaish.904@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The meter-start value of a new StartTransaction wasn't validated against the previous transaction on the same connector. If a charger reports a meterStart lower than the last meterStop, the energy register has regressed — either from a meter reset, firmware bug, or data integrity issue. This should be flagged as suspect.
I've added a
getLastTransactionStopValue(chargeBoxId, connectorId)query to the repository layer and wired it into the existingCentralSystemService16_ServiceValidator.validateStart()method. The check runs after the existing validations (positive connectorId, non-negative meterStart, non-future timestamp) and logs a warning whenmeterStart < previousMeterStop. When there's no previous completed transaction (first session on a connector), the check is skipped.This addresses rule 5 ("Meter regression") from #1963.
Tests added for: regression detected, equal values allowed, no previous transaction.