validate MeterValue timestamps against transaction time window and chronological order

[rishabhvaish]

The existing validateMeterValuesInternal checked that timestamps aren't in the future and don't exceed the stop timestamp, but it didn't verify that they fall within the transaction's time window or arrive in order.

I've added two checks:

1. Transaction time window — when called from validateStop, the method now receives startTimestamp and rejects any MeterValue whose timestamp is before the transaction started. This catches late-arriving meter data from a previous session or clock-drift replays.
2. Chronological ordering — timestamps in the MeterValue list must be non-decreasing. Out-of-order entries suggest clock issues or message replays and are now flagged.
   Both checks follow the existing pattern of returning a SteveException without throwing, so the caller decides how to handle it.

Tests cover: before-start rejection, at-start acceptance, out-of-order rejection, in-order acceptance, and equal-timestamp acceptance.

Partial fix for #1962.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Validate MeterValue timestamps against transaction window and order

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Validate MeterValue timestamps fall within transaction time window
• Enforce chronological ordering of MeterValue timestamps
• Add comprehensive test coverage for new validations
• Refactor timestamp extraction to support multiple validation checks

Diagram

flowchart LR
  MV["MeterValue List"]
  Extract["Extract Timestamps"]
  FutureCheck["Check Future Timestamps"]
  WindowCheck["Check Time Window<br/>start ≤ ts ≤ stop"]
  OrderCheck["Check Chronological<br/>Order"]
  Result["Validation Result"]
  
  MV --> Extract
  Extract --> FutureCheck
  FutureCheck --> WindowCheck
  WindowCheck --> OrderCheck
  OrderCheck --> Result

Loading

File Changes

1. src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java 🐞 Bug fix +25/-8

Add transaction window and chronological order validation

• Added startTimestamp parameter to validateMeterValuesInternal method to validate against
 transaction start time
• Refactored timestamp extraction to collect all timestamps in a list for multiple validation passes
• Added validation to reject MeterValues with timestamps before transaction start time
• Added chronological ordering check to ensure timestamps are non-decreasing
• Removed unused import AbstractInstant and added Comparator import

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java

---

2. src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java 🧪 Tests +53/-0

Add timestamp validation test coverage

• Added test for MeterValue timestamp before start time rejection
• Added test for MeterValue timestamp at start time acceptance
• Added test for out-of-order timestamps rejection
• Added test for in-order timestamps acceptance
• Added test for equal timestamps acceptance

src/test/java/de/rwth/idsg/steve/service/CentralSystemService16ServiceValidatorTest.java

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

1. Null MeterValue not handled ☑ 📘 Rule violation ✓ Correctness

Description

meterValues is streamed and dereferenced via MeterValue::getTimestamp without filtering out
possible null elements, which can cause an NPE instead of a clear validation error. Additionally,
null timestamps are silently dropped (unless all are null), allowing invalid external input to
bypass validation logic.

Code

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[R115-118]

+        List<DateTime> timestamps = meterValues.stream()
        .map(MeterValue::getTimestamp)
        .filter(java.util.Objects::nonNull)
-            .max(AbstractInstant::compareTo)
-            .orElse(null);
+            .toList();

Evidence

PR Compliance ID 5 requires externally supplied collections to be sanitized (treat null as empty and
filter null elements) and to fail fast with clear exceptions, rather than risking NPEs or implicitly
ignoring invalid items. The changed code dereferences elements without null-element filtering and
drops null timestamps via filter(Objects::nonNull) instead of rejecting them.

src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[115-118]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validateMeterValuesInternal` can throw an NPE because it calls `MeterValue::getTimestamp` on elements of an externally supplied list without filtering out null elements first. It also silently ignores null timestamps (unless *all* timestamps are null), which lets invalid external input bypass validation.
## Issue Context
Compliance requires sanitizing externally supplied collections (treat null as empty, filter null elements) and failing fast with clear/semantically correct validation errors.
## Fix Focus Areas
- src/main/java/de/rwth/idsg/steve/service/CentralSystemService16_ServiceValidator.java[115-123]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[goekay]

hey @rishabhvaish, thanks for your contribution.

the code makes, in worst-case scenario, 4 passes over the MeterValues. this can be inefficient with long-running transactions and therefore a long list of MeterValues entries. i think, this number can be reduced to 2 without sacrificing anything. or even 1 (but maybe in this case it will harm the readability and compactness). can you explore an improvement in this area?

[rishabhvaish]

@goekay — Good catch. Refactored validateMeterValuesInternal from 4 passes to 1.

Before: stream → list of timestamps, stream → max, stream → min, loop → order check
After: single for loop tracking earliest, latest, and outOfOrder in one pass. Also removes the intermediate List<DateTime> allocation and the unused Comparator import.

All existing tests are unchanged — they exercise the same public API and error messages.

[goekay]

@rishabhvaish i will revert this PR partially, because of #1992