.pr_agent_accepted_suggestions

PR 1865 (2025-10-29)

[incremental [*]] Make store holder thread-safe

---

✅ Make store holder thread-safe

**

• @author Sevket Goekay [email protected] @@ -32,7 +32,7 @@ @Component public class SessionContextStoreHolder {

• private final EnumMap<OcppVersion, SessionContextStore> storesPerVersion = new EnumMap<>(OcppVersion.class);

• private final ConcurrentHashMap<OcppVersion, SessionContextStore> storesPerVersion = new ConcurrentHashMap<>();

**Replace the non-thread-safe EnumMap in SessionContextStoreHolder with a java.util.concurrent.ConcurrentHashMap. This prevents race conditions when getOrCreate is called concurrently from multiple threads.**

[src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreHolder.java [32-46]](https://github.com/steve-community/steve/pull/1865/files#diff-7f2ea532644135afba5b5cf2e742659effaa151c398f1b3891320538d15a2d35R32-R46)

```diff
 @Component
 public class SessionContextStoreHolder {
 
-    private final EnumMap<OcppVersion, SessionContextStore> storesPerVersion = new EnumMap<>(OcppVersion.class);
+    private final java.util.concurrent.ConcurrentMap<OcppVersion, SessionContextStore> storesPerVersion =
+            new java.util.concurrent.ConcurrentHashMap<>();
 
     private final WsSessionSelectStrategy wsSessionSelectStrategy;
 
     public SessionContextStoreHolder(SteveProperties steveProperties) {
         wsSessionSelectStrategy = steveProperties.getOcpp().getWsSessionSelectStrategy();
     }
 
     public SessionContextStore getOrCreate(OcppVersion version) {
-        return storesPerVersion.computeIfAbsent(version, k -> new SessionContextStoreImpl(wsSessionSelectStrategy));
+        return storesPerVersion.computeIfAbsent(version, v -> new SessionContextStoreImpl(wsSessionSelectStrategy));
     }
 }

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical thread-safety issue where using a non-thread-safe EnumMap could lead to race conditions in a multi-threaded environment. Replacing it with a ConcurrentHashMap is the correct solution to ensure safe concurrent access and prevent data corruption.

---

[general] Handle multiple active transactions gracefully

---

✅ Handle multiple active transactions gracefully

Handle cases with multiple active transactions by returning the most recent one instead of throwing an IllegalStateException.

src/main/java/de/rwth/idsg/steve/service/TransactionService.java [83-98]

 public Transaction getActiveTransaction(String chargeBoxId, Integer connectorId) {
     TransactionQueryForm form = new TransactionQueryForm();
     form.setChargeBoxId(chargeBoxId);
     form.setConnectorId(connectorId);
     form.setType(TransactionQueryForm.QueryType.ACTIVE);
 
     List<Transaction> transactions = transactionRepository.getTransactions(form);
     if (transactions.isEmpty()) {
         return null;
-    } else if (transactions.size() == 1) {
+    } else if (transactions.size() > 1) {
+        log.warn("Found multiple active transactions for chargeBoxId '{}' and connectorId '{}'. Returning the most recent one.", chargeBoxId, connectorId);
+        return transactions.stream()
+                           .max(Comparator.comparing(Transaction::getStartTimestamp))
+                           .orElse(null); // Should not be null here, but for safety
+    } else {
         return transactions.get(0);
-    } else {
-        throw new IllegalStateException("There are multiple active transactions with the same charge box id and connector id");
     }
 }

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that multiple active transactions could exist and proposes a reasonable fallback strategy (returning the latest one), which improves the system's resilience to data inconsistencies.

---

[possible issue] Guard against empty query results

---

✅ Guard against empty query results

Add a check to ensure the transaction list is not empty before calling getFirst() in getTransaction to prevent a potential NoSuchElementException.

src/main/java/de/rwth/idsg/steve/service/TransactionService.java [74-81]

 public Transaction getTransaction(int transactionPk) {
     TransactionQueryForm form = new TransactionQueryForm();
     form.setTransactionPk(transactionPk);
     form.setReturnCSV(false);
     form.setType(TransactionQueryForm.QueryType.ALL);
 
-    return transactionRepository.getTransactions(form).getFirst();
+    List<Transaction> list = transactionRepository.getTransactions(form);
+    if (list == null || list.isEmpty()) {
+        return null;
+    }
+    return list.get(0);
 }

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that calling getFirst() on a potentially empty list can cause a NoSuchElementException, and proposes a valid fix to improve the method's robustness.

---

---