refactor: reverse dependency between jooq and core module#74
Conversation
WalkthroughLarge refactor migrating from jOOQ record types to plain DTOs, builders, and Optionals; introduces a dedicated jooq module with mappers and JooqConfiguration, removes DataSource/DSLContext beans from core config, updates repositories/services/controllers/tests/forms, and adapts exception/message constructors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as REST Controller
participant Service
participant Repo
participant DTO
Client->>API: POST /chargepoints (body)
API->>Service: createChargePoint(body)
Service->>Repo: insertChargePoint(details)
Repo-->>Service: createdChargePointPk
Service->>API: build Location with createdChargePointPk
API-->>Client: 201 Created (Location -> /chargepoints/{pk})
sequenceDiagram
autonumber
participant Caller
participant AuthSvc as AuthTagServiceLocal
participant TagRepo as OcppTagRepository
participant Tag as OcppTagActivity (DTO)
Caller->>AuthSvc: authorize(idTag)
AuthSvc->>TagRepo: getRecord(idTag)
TagRepo-->>AuthSvc: Optional<OcppTagActivity>
alt missing
AuthSvc-->>Caller: IdTagInfo(status=INVALID)
else present
AuthSvc->>Tag: isBlocked()/isExpired()/hasReachedLimit()
Tag-->>AuthSvc: booleans
AuthSvc->>Tag: mapToAuthorizationData(now)
Tag-->>AuthSvc: AuthorizationData
AuthSvc-->>Caller: AuthorizationData
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (1)
151-159: Handle emptySCHEMA_VERSIONsafely; current code can NPE.
fetchOne()may returnnullon a fresh DB, makingrecord.value2()throw. UseorderBy…limit(1)+fetchOptional()and map safely.- var record = ctx.select(SCHEMA_VERSION.VERSION, SCHEMA_VERSION.INSTALLED_ON) - .from(SCHEMA_VERSION) - .where(SCHEMA_VERSION.INSTALLED_RANK.eq( - select(max(SCHEMA_VERSION.INSTALLED_RANK)).from(SCHEMA_VERSION))) - .fetchOne(); - - var ts = DateTimeUtils.humanize(record.value2()); - return DbVersion.builder().version(record.value1()).updateTimestamp(ts).build(); + var maybe = ctx.select(SCHEMA_VERSION.VERSION, SCHEMA_VERSION.INSTALLED_ON) + .from(SCHEMA_VERSION) + .orderBy(SCHEMA_VERSION.INSTALLED_RANK.desc()) + .limit(1) + .fetchOptional(); + + return maybe + .map(r -> DbVersion.builder() + .version(r.get(SCHEMA_VERSION.VERSION)) + .updateTimestamp(DateTimeUtils.humanize(r.get(SCHEMA_VERSION.INSTALLED_ON))) + .build()) + // If you prefer to fail fast instead of sentinel values, throw here instead. + .orElse(DbVersion.builder().version("unknown").updateTimestamp("-").build());steve-api/src/main/java/de/rwth/idsg/steve/web/api/ChargePointsRestController.java (1)
77-81: Fix Location header to point to the created ChargePoint resource.
buildAndExpand(body.getChargeBoxPk())expands with chargeBoxPk, not the new chargePointPk. Use thechargepointPkreturned by the service.Apply:
- var location = ServletUriComponentsBuilder.fromCurrentRequest() - .path("/{id}") - .buildAndExpand(body.getChargeBoxPk()) + var location = ServletUriComponentsBuilder.fromCurrentRequest() + .path("/{id}") + .buildAndExpand(chargepointPk) .toUri();steve/src/test/java/de/rwth/idsg/steve/OperationalTestSoapOCPP16.java (1)
573-581: Test bug: contradicting assertion then dereference.
assertThat(sampledValues).isEmpty()followed bysampledValues.get(0)will fail. The list has one element by construction.Apply:
- assertThat(sampledValues).isEmpty(); - var thisValueFound = false; - // and check, if it can be found in the DB - for (var values : details.getValues()) { - if (values.getValue().equals(sampledValues.get(0).getValue())) { + assertThat(sampledValues).isNotEmpty(); + var sample = sampledValues.get(0); + var thisValueFound = false; + // and check, if it can be found in the DB + for (var values : details.getValues()) { + if (values.getValue().equals(sample.getValue())) {steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
214-241: Avoid ordinal field access from union table; select explicit fieldsIndex-based
t1.field(2..9, ...)is brittle against schema/order changes and hard to read.Replace with explicit fields:
- var dateTimeField = t1.field(2, LocalDateTime.class); - var values = ctx - .select( - dateTimeField, - t1.field(3, String.class), - t1.field(4, String.class), - t1.field(5, String.class), - t1.field(6, String.class), - t1.field(7, String.class), - t1.field(8, String.class), - t1.field(9, String.class)) + var ts = t1.field(CONNECTOR_METER_VALUE.VALUE_TIMESTAMP); + var val = t1.field(CONNECTOR_METER_VALUE.VALUE); + var ctxf = t1.field(CONNECTOR_METER_VALUE.READING_CONTEXT); + var fmt = t1.field(CONNECTOR_METER_VALUE.FORMAT); + var mea = t1.field(CONNECTOR_METER_VALUE.MEASURAND); + var loc = t1.field(CONNECTOR_METER_VALUE.LOCATION); + var unit = t1.field(CONNECTOR_METER_VALUE.UNIT); + var ph = t1.field(CONNECTOR_METER_VALUE.PHASE); + var values = ctx + .select(ts, val, ctxf, fmt, mea, loc, unit, ph) .from(t1) - .orderBy(dateTimeField) + .orderBy(ts) .fetch() .map(r -> TransactionDetails.MeterValues.builder() - .valueTimestamp(toInstant(r.value1())) - .value(r.value2()) - .readingContext(r.value3()) - .format(r.value4()) - .measurand(r.value5()) - .location(r.value6()) - .unit(r.value7()) - .phase(r.value8()) + .valueTimestamp(toInstant(r.value1())) // ts + .value(r.value2()) // val + .readingContext(r.value3()) // ctxf + .format(r.value4()) // fmt + .measurand(r.value5()) // mea + .location(r.value6()) // loc + .unit(r.value7()) // unit + .phase(r.value8()) // ph .build())steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
199-206: Potential NPE when resolving connectorPk
fetchOne()may return null;.value1()would NPE (e.g., inconsistent DB state or invalid transactionId).Harden resolution:
- var connectorPk = ctx.select(TRANSACTION_START.CONNECTOR_PK) + Integer connectorPk = ctx.select(TRANSACTION_START.CONNECTOR_PK) .from(TRANSACTION_START) .where(TRANSACTION_START.TRANSACTION_PK.equal(transactionId)) - .fetchOne() - .value1(); + .fetchOptional(TRANSACTION_START.CONNECTOR_PK) + .orElseThrow(() -> new SteveException.InternalError( + "No connectorPk for transactionId=" + transactionId));steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
183-186: Missing cache invalidation on delete by PK can leak stale API auth.Deleting by PK does not invalidate
userCache, so a previously cachedUserDetailsmay remain valid for up to 10 minutes.Apply this diff to invalidate by username:
- public void deleteUser(int webUserPk) { - webUserRepository.deleteUser(webUserPk); - } + public void deleteUser(int webUserPk) { + var existing = webUserRepository.loadUserByUserPk(webUserPk).orElse(null); + webUserRepository.deleteUser(webUserPk); + if (existing != null) { + userCache.invalidate(existing.getLogin()); + } + }
♻️ Duplicate comments (1)
steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
269-275: Unify API authority mapping with UI login mapping.See earlier comment; this is the concrete spot to change. Diff included there.
🧹 Nitpick comments (60)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (2)
124-146: Avoid ordinal accessors; bind by Field to reduce brittleness.Using
value1()…value9()couples positions to mapping. Fetch, thenget(field)by name for safer maintenance.- var gs = ctx.select( + var row = ctx.select( numChargeBoxes, numOcppTags, numUsers, numReservations, numTransactions, heartbeatsToday, heartbeatsYesterday, heartbeatsEarlier, numWebUsers) .fetchOne(); return Statistics.builder() - .numChargeBoxes(gs.value1()) - .numOcppTags(gs.value2()) - .numUsers(gs.value3()) - .numReservations(gs.value4()) - .numTransactions(gs.value5()) - .heartbeatToday(gs.value6()) - .heartbeatYesterday(gs.value7()) - .heartbeatEarlier(gs.value8()) - .numWebUsers(gs.value9()) + .numChargeBoxes(row.get(numChargeBoxes)) + .numOcppTags(row.get(numOcppTags)) + .numUsers(row.get(numUsers)) + .numReservations(row.get(numReservations)) + .numTransactions(row.get(numTransactions)) + .heartbeatToday(row.get(heartbeatsToday)) + .heartbeatYesterday(row.get(heartbeatsYesterday)) + .heartbeatEarlier(row.get(heartbeatsEarlier)) + .numWebUsers(row.get(numWebUsers)) .build();
70-75:varusage is supported (Java 21); consider more descriptive variable names
Confirmed pom.xml sets<java.version>to 21 for maven-compiler-plugin source/target. Minor nit:javaandsqlare terse—considerjavaOffsetSeconds/sqlOffsetSeconds.steve-jooq/pom.xml (2)
35-39: Scope MySQL JDBC driver dependencies as runtime (and mark optional if consumers supply their own driver).${mysql.jdbc.version}is already defined in the parentpom.xml. Update all threemysql-connector-jentries insteve-jooq/pom.xml(lines 37–38, 113–114, 194–195) to include:<scope>runtime</scope> <!-- <optional>true</optional> -->
40-44: Pin HikariCP version via Maven property
Update the dependency to reference a${hikari.version}property and add under<properties>:<properties> + <hikari.version>7.0.1</hikari.version> </properties> <dependency> <groupId>com.zaxxer</groupId> <artifactId>HikariCP</artifactId> - <version>7.0.1</version> + <version>${hikari.version}</version> </dependency>HikariCP 7.x requires Java 11+, and the project is already set to Java 21, so compatibility is satisfied. (github.com, libraries.io)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (3)
60-63: var adoption is fine; minor DRY + naming nits
- Optional: factor out recipients/features extraction (used here and in getMailSettings) to avoid duplication.
- Optional: consider using
emails(lowercase m) for consistency with common naming.- Optional:
final varfor locals that don’t change.Suggested tweak within this block:
- var eMails = splitByComma(r.getMailRecipients()); - var features = splitFeatures(r.getNotificationFeatures()); + var eMails = parseRecipients(r); + var features = parseEnabledFeatures(r);Add these helpers (outside this range):
private static List<String> parseRecipients(SettingsRecord r) { return splitByComma(r.getMailRecipients()); } private List<NotificationFeature> parseEnabledFeatures(SettingsRecord r) { return splitFeatures(r.getNotificationFeatures()); }
82-85: Duplicate parsing logic — consider reusing helpersMirror the same refactor here to keep both methods in sync.
- var eMails = splitByComma(r.getMailRecipients()); - var features = splitFeatures(r.getNotificationFeatures()); + var eMails = parseRecipients(r); + var features = parseEnabledFeatures(r);
112-113: Explicitly serialize NotificationFeature by name
In SettingsRepositoryImpl.update (lines 112–113),joinByComma(form.getEnabledFeatures())relies on the enum’s defaulttoString()(which currently equalsname()). Map viaform.getEnabledFeatures().stream().map(NotificationFeature::name).collect(toList())(then join) to make persistence intent explicit and guard against futuretoString()overrides.steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java (2)
85-90: Defensive enum/value validation beforefromValue(...)If any of these strings are null/invalid,
fromValue(...)will throw. Either guarantee non-null/valid upstream or add a small pre-check to fail fast with a clear message.Also applies to: 93-96
98-101: Minor: streamline null handling forrecurrencyKindApply for brevity:
- .withRecurrencyKind( - details.getRecurrencyKind() == null - ? null - : RecurrencyKindType.fromValue(details.getRecurrencyKind())) + .withRecurrencyKind( + java.util.Optional.ofNullable(details.getRecurrencyKind()) + .map(RecurrencyKindType::fromValue) + .orElse(null))steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (1)
55-79: Validate enum inputs and confirm JDK level for.toList()
fromValue(...)on multiple enums will throw on unexpected values; ensure upstream guarantees or add null/whitelist checks..toList()requires Java 16+. Confirm baseline or switch toCollectors.toList().steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargePointMapper.java (2)
42-43: Avoid potential NPE on autounboxing Boolean→booleanIf
InsertConnectorStatusAfterTransactionMsgcan be null in DB, autounboxing will NPE. Safe-default to false.Apply:
- .insertConnectorStatusAfterTransactionMsg(r.getInsertConnectorStatusAfterTransactionMsg()) + .insertConnectorStatusAfterTransactionMsg( + java.lang.Boolean.TRUE.equals(r.getInsertConnectorStatusAfterTransactionMsg()))
22-23: Layering: jOOQ module depends on web DTOAddressIf feasible, consider mapping to core-level fields/DTO to keep infra (jOOQ) free of web-layer types.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java (3)
149-163: Guard against missing/multiple CONNECTOR rows when insertingUsing a subselect for CONNECTOR_PK defers “not found”/“not unique” failures to the DB and yields opaque errors. Prefer resolving to a single pk explicitly and erroring early with a clear message.
Apply either approach:
Option A: prefetch and validate (clear errors, 2 round-trips)
- var connectorPkQuery = DSL.select(CONNECTOR.CONNECTOR_PK) - .from(CONNECTOR) - .where(CONNECTOR.CHARGE_BOX_ID.equal(params.getChargeBoxId())) - .and(CONNECTOR.CONNECTOR_ID.equal(params.getConnectorId())); - - var reservationId = ctx.insertInto(RESERVATION) - .set(RESERVATION.CONNECTOR_PK, connectorPkQuery) + var connectorPk = ctx.select(CONNECTOR.CONNECTOR_PK) + .from(CONNECTOR) + .where(CONNECTOR.CHARGE_BOX_ID.eq(params.getChargeBoxId())) + .and(CONNECTOR.CONNECTOR_ID.eq(params.getConnectorId())) + .fetchOne(CONNECTOR.CONNECTOR_PK); + if (connectorPk == null) { + throw new SteveException.UserError("Connector not found for chargeBoxId=" + + params.getChargeBoxId() + ", connectorId=" + params.getConnectorId()); + } + var reservationId = ctx.insertInto(RESERVATION) + .set(RESERVATION.CONNECTOR_PK, connectorPk) .set(RESERVATION.ID_TAG, params.getIdTag())Option B: retain subselect but assert uniqueness beforehand
+ var exists = ctx.fetchExists( + DSL.selectOne().from(CONNECTOR) + .where(CONNECTOR.CHARGE_BOX_ID.eq(params.getChargeBoxId())) + .and(CONNECTOR.CONNECTOR_ID.eq(params.getConnectorId()))); + if (!exists) { + throw new SteveException.UserError("Connector not found for chargeBoxId=" + + params.getChargeBoxId() + ", connectorId=" + params.getConnectorId()); + }If a unique index on (CHARGE_BOX_ID, CONNECTOR_ID) already exists, keep the subselect but catching DataAccessException to translate into SteveException with context would still help.
132-142: Use database time instead of JVM time in predicatesComparing against LocalDateTime.now() can drift if DB and app timezones differ. Prefer DSL.currentLocalDateTime() so the comparison is evaluated server-side.
- .and(RESERVATION.EXPIRY_DATETIME.greaterThan(LocalDateTime.now())) + .and(RESERVATION.EXPIRY_DATETIME.greaterThan(DSL.currentLocalDateTime()))and in processType:
- case ACTIVE -> selectQuery.addConditions(RESERVATION.EXPIRY_DATETIME.greaterThan(LocalDateTime.now())); + case ACTIVE -> selectQuery.addConditions(RESERVATION.EXPIRY_DATETIME.greaterThan(DSL.currentLocalDateTime()));Also applies to: 268-274
87-130: Avoid raw SelectQuery and the suppressionYou can drop @SuppressWarnings("unchecked") by building a Condition and composing a typed select, which also reads cleaner.
- SelectQuery selectQuery = ctx.selectQuery(); - selectQuery.addFrom(RESERVATION); - ... - return selectQuery.fetch().map(new ReservationMapper()); + var conditions = DSL.trueCondition(); + if (form.isChargeBoxIdSet()) conditions = conditions.and(CHARGE_BOX.CHARGE_BOX_ID.eq(form.getChargeBoxId())); + if (form.isOcppIdTagSet()) conditions = conditions.and(RESERVATION.ID_TAG.eq(form.getOcppIdTag())); + if (form.isUserIdSet()) conditions = conditions.and(RESERVATION.ID_TAG.in(ocppTagByUserIdQuery(ctx, form.getUserId()))); + if (form.isStatusSet()) conditions = conditions.and(RESERVATION.STATUS.eq(form.getStatus().name())); + processType(ctx.selectQuery().addConditions(conditions), form); // or inline equivalent + return ctx.select(RESERVATION.RESERVATION_PK, RESERVATION.TRANSACTION_PK, OCPP_TAG.OCPP_TAG_PK, + CHARGE_BOX.CHARGE_BOX_PK, OCPP_TAG.ID_TAG, CHARGE_BOX.CHARGE_BOX_ID, + RESERVATION.START_DATETIME, RESERVATION.EXPIRY_DATETIME, + RESERVATION.STATUS, CONNECTOR.CONNECTOR_ID) + .from(RESERVATION) + .join(OCPP_TAG).on(OCPP_TAG.ID_TAG.eq(RESERVATION.ID_TAG)) + .join(CONNECTOR).on(CONNECTOR.CONNECTOR_PK.eq(RESERVATION.CONNECTOR_PK)) + .join(CHARGE_BOX).on(CONNECTOR.CHARGE_BOX_ID.eq(CHARGE_BOX.CHARGE_BOX_ID)) + .where(conditions) + .orderBy(RESERVATION.EXPIRY_DATETIME.asc()) + .fetch(new ReservationMapper());steve-core/src/main/java/de/rwth/idsg/steve/web/dto/WebUserAuthority.java (3)
34-50: Make jsonValue deterministic and simplerjsonValue is built from a HashSet stream; ordering is non-deterministic, yielding unstable JSON strings. Sort before joining.
- this.jsonValue = this.values.stream() - .map(v -> "\"" + v + "\"") - .reduce((a, b) -> a + ", " + b) - .map(s -> "[" + s + "]") - .orElseThrow(() -> new IllegalArgumentException("Failed to create JSON value")); + this.jsonValue = this.values.stream() + .sorted() + .map(v -> "\"" + v + "\"") + .collect(java.util.stream.Collectors.joining(", ", "[", "]"));Minor: the constructor message says “JSON values” but this argument represents authorities. Consider “Authority values must not be null or empty.”
57-69: Preserve parse error cause and null-guard inputfromJsonValue currently drops the original exception and NPEs on null input.
- public static WebUserAuthority fromJsonValue(ObjectMapper mapper, String v) { + public static WebUserAuthority fromJsonValue(ObjectMapper mapper, String v) { + if (v == null || v.isBlank()) { + throw new IllegalArgumentException("Empty WebUserAuthority JSON"); + } try { - var values = new HashSet<>(Arrays.asList(mapper.readValue(v, String[].class))); + var values = new HashSet<>(Arrays.asList(mapper.readValue(v, String[].class))); for (WebUserAuthority c : WebUserAuthority.values()) { if (c.values.equals(values)) { return c; } } - throw new IllegalArgumentException(v); + throw new IllegalArgumentException("No enum matches authorities: " + values); } catch (JsonProcessingException e) { - throw new IllegalArgumentException(v); + throw new IllegalArgumentException("Invalid WebUserAuthority JSON: " + v, e); } }
80-87: Clarify matching semantics or normalize inputfromAuthority matches case-sensitively. If inputs may vary in case/whitespace, normalize once.
- public static WebUserAuthority fromAuthority(String authority) { + public static WebUserAuthority fromAuthority(String authority) { + if (authority == null) throw new IllegalArgumentException("authority is null"); + var a = authority.trim().toUpperCase(java.util.Locale.ROOT); - for (WebUserAuthority c : WebUserAuthority.values()) { - if (c.values.contains(authority)) { + for (WebUserAuthority c : WebUserAuthority.values()) { + if (c.values.contains(a)) { return c; } } throw new IllegalArgumentException("No matching authority for: " + authority); }If callers already normalize, feel free to ignore.
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/WebUserBaseForm.java (1)
43-45: Avoid NPE when authorities is unsetUI binds may leave authorities null; guard and return an empty set.
-import java.util.Set; +import java.util.Set; +import java.util.Collections; ... - public Set<String> getAuthoritiesAsStrings() { - return authorities.getValues(); - } + public Set<String> getAuthoritiesAsStrings() { + return authorities != null ? authorities.getValues() : Collections.emptySet(); + }steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/WebUser.java (1)
28-41: Defensive immutability for authoritiesauthorities is exposed as a Set that callers can mutate after construction. Return an unmodifiable copy to keep DTOs immutable.
-import lombok.Getter; +import lombok.Getter; +import java.util.Collections; ... -@Getter +@Getter @Builder public class WebUser { ... - private final Set<WebUserAuthority> authorities; + private final Set<WebUserAuthority> authorities; + + public Set<WebUserAuthority> getAuthorities() { + return authorities == null ? Set.of() : Collections.unmodifiableSet(authorities); + }If you prefer, convert to EnumSet.copyOf(...) at construction.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargePoint.java (1)
61-69: Optional: null-safety for Address assemblyIf any address parts can be null, ensure Address setters tolerate nulls, or add minimal normalization.
- address.setCountry(country); + if (country != null) { + address.setCountry(country); + }If Address already handles null, ignore.
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/WebUserMapper.java (2)
57-60: Unify authority parsing to a single path.Both mappers should use the same JSON→Set logic. Prefer
fromJSON(...)here for consistency and easier maintenance.- .authorities(WebUserAuthority.fromJsonValue( - objectMapper, - r.getAuthorities() == null ? null : r.getAuthorities().data())) + .authorities(fromJSON(r.getAuthorities(), objectMapper))
71-74: Improve error logging and return an immutable empty set on failure.Include the raw JSON (trimmed) to aid debugging and return
Collections.emptySet()instead of allocating a newHashSet.- } catch (IOException e) { - log.error("Failed to deserialize authorities from JSON", e); - return new HashSet<>(); + } catch (IOException e) { + log.error("Failed to deserialize authorities from JSON: {}", json == null ? "null" : json.data(), e); + return Collections.emptySet(); }steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/OcppTagActivity.java (1)
69-79: Consider emitting CONCURRENT_TX when the active-tx limit is reached.Spec users often rely on CONCURRENT_TX to signal max concurrent sessions. You already have the predicate; wire it in.
public ocpp.cp._2015._10.AuthorizationStatus decideStatusForAuthData(Instant date) { if (isBlocked()) { return ocpp.cp._2015._10.AuthorizationStatus.BLOCKED; } else if (isExpired(date)) { return ocpp.cp._2015._10.AuthorizationStatus.EXPIRED; - // } else if (hasReachedLimitOfActiveTransactions()) { - // return ocpp.cp._2015._10.AuthorizationStatus.CONCURRENT_TX; + } else if (hasReachedLimitOfActiveTransactions()) { + return ocpp.cp._2015._10.AuthorizationStatus.CONCURRENT_TX; } else { return ocpp.cp._2015._10.AuthorizationStatus.ACCEPTED; } }steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java (1)
91-103: Handle empty registration-status filter explicitly.
IN ()yields no rows. If empty means “no filter,” add a condition similar tochargeBoxIdCondition.- return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS) + var registrationStatusCondition = CollectionUtils.isEmpty(inStatusFilter) + ? DSL.trueCondition() + : CHARGE_BOX.REGISTRATION_STATUS.in(inStatusFilter); + + return ctx.select(CHARGE_BOX.CHARGE_BOX_ID, CHARGE_BOX.ENDPOINT_ADDRESS) .from(CHARGE_BOX) .where(CHARGE_BOX.OCPP_PROTOCOL.equal(protocol.getCompositeValue())) .and(CHARGE_BOX.ENDPOINT_ADDRESS.isNotNull()) - .and(CHARGE_BOX.REGISTRATION_STATUS.in(inStatusFilter)) + .and(registrationStatusCondition) .and(chargeBoxIdCondition) .fetch() .map(r -> new ChargePointSelect(protocol, r.value1(), r.value2()));steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
25-27: Annotate optional fields for clarity.Consider
@NullableonrecurrencyKind,validFrom,validTo,durationInSeconds,startSchedule,minChargingRateto document nullability throughout the stack.steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java (1)
131-136: Clarify mixed-strategy fallback with logging.Using current startValue with next tx timestamp is non-obvious. Add a brief comment and a debug log to aid traceability.
steve-api/src/test/java/de/rwth/idsg/steve/web/api/ChargePointsRestControllerTest.java (1)
110-113: Builder-based helper decouples the test from jOOQ; consider populating a more realistic Details stub.To reduce future test churn (NPEs when new assertions land), consider setting minimal non-null fields commonly read by the controller (e.g., registrationStatus, insertConnectorStatusAfterTransactionMsg).
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (1)
254-269: Map with fetchOptional to avoid the explicit null check (minor).You can streamline getDetails by lifting the Optional to the query level.
Apply:
- public Optional<ChargingProfile.Details> getDetails(int chargingProfilePk) { - var profile = ctx.selectFrom(CHARGING_PROFILE) - .where(CHARGING_PROFILE.CHARGING_PROFILE_PK.eq(chargingProfilePk)) - .fetchOne(); - - if (profile == null) { - return Optional.empty(); - } - - var periods = ctx.selectFrom(CHARGING_SCHEDULE_PERIOD) - .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(chargingProfilePk)) - .orderBy(CHARGING_SCHEDULE_PERIOD.START_PERIOD_IN_SECONDS.asc()) - .fetch(); - - return Optional.of(ChargingProfileMapper.fromRecord(profile, periods)); - } + public Optional<ChargingProfile.Details> getDetails(int chargingProfilePk) { + return ctx.selectFrom(CHARGING_PROFILE) + .where(CHARGING_PROFILE.CHARGING_PROFILE_PK.eq(chargingProfilePk)) + .fetchOptional() + .map(profile -> { + var periods = ctx.selectFrom(CHARGING_SCHEDULE_PERIOD) + .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(chargingProfilePk)) + .orderBy(CHARGING_SCHEDULE_PERIOD.START_PERIOD_IN_SECONDS.asc()) + .fetch(); + return ChargingProfileMapper.fromRecord(profile, periods); + }); + }steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargePointForm.java (1)
41-56: fromDetails: add a null guard to fail fast (and keep constraint alignment).registrationStatus and insertConnectorStatusAfterTransactionMsg are mandatory on the form. A simple precondition avoids surprising NPEs.
Apply:
- public static ChargePointForm fromDetails(de.rwth.idsg.steve.repository.dto.ChargePoint.Details details) { - var form = new ChargePointForm(); + public static ChargePointForm fromDetails(de.rwth.idsg.steve.repository.dto.ChargePoint.Details details) { + java.util.Objects.requireNonNull(details, "details"); + var form = new ChargePointForm(); form.chargeBoxPk = details.getChargeBoxPk(); form.chargeBoxId = details.getChargeBoxId(); form.note = details.getNote(); form.description = details.getDescription(); form.locationLatitude = details.getLocationLatitude(); form.locationLongitude = details.getLocationLongitude(); form.insertConnectorStatusAfterTransactionMsg = details.isInsertConnectorStatusAfterTransactionMsg(); form.adminAddress = details.getAdminAddress(); form.registrationStatus = details.getRegistrationStatus(); form.address = details.getAddress(); return form; }steve-core/src/main/java/de/rwth/idsg/steve/web/dto/OcppTagForm.java (1)
45-54: Rename factory away from “Record” and add null-guardPost-decoupling this no longer maps from a jOOQ record. Rename to reflect the DTO and fail fast on null inputs.
- public static OcppTagForm fromRecord(de.rwth.idsg.steve.repository.dto.OcppTagActivity tag) { + public static OcppTagForm fromActivity(de.rwth.idsg.steve.repository.dto.OcppTagActivity tag) { + Objects.requireNonNull(tag, "tag"); var form = new OcppTagForm(); form.ocppTagPk = tag.getOcppTagPk(); form.idTag = tag.getIdTag(); form.parentIdTag = tag.getParentIdTag(); form.expiryDate = tag.getExpiryDate(); form.maxActiveTransactionCount = tag.getMaxActiveTransactionCount(); form.note = tag.getNote(); return form; }If you apply this rename, update call sites (e.g., OcppTagsController) accordingly:
- var form = OcppTagForm.fromRecord(tag); + var form = OcppTagForm.fromActivity(tag);steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/UserMapper.java (1)
27-29: Fail fast for null record and defensively handle ocppTagEntriesGuard against null
UserRecordand avoid NPEs/accidental mutation by defensively copyingocppTagEntries.Apply:
@@ -import java.util.List; -import java.util.Optional; +import java.util.List; +import java.util.Optional; +import java.util.Objects; @@ public static User.Details fromRecord( UserRecord r, Optional<Address> address, List<User.OcppTagEntry> ocppTagEntries) { - var builder = User.Details.builder() + Objects.requireNonNull(r, "UserRecord must not be null"); + var builder = User.Details.builder() .userPk(r.getUserPk()) @@ - .note(r.getNote()) - .ocppTagEntries(ocppTagEntries); + .note(r.getNote()) + .ocppTagEntries(ocppTagEntries == null ? List.of() : List.copyOf(ocppTagEntries));Also applies to: 33-45
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/AddressMapper.java (1)
30-38: Restore explicit null handling for input recordIf callers pass a null record (e.g., fetchOne() returned null), this now NPEs. Fail fast with a clear message.
Check call sites map nulls to Optional before calling this mapper.
Apply:
public static Address fromRecord(AddressRecord r) { - var a = new Address(); + java.util.Objects.requireNonNull(r, "AddressRecord must not be null"); + var a = new Address(); a.setAddressPk(r.getAddressPk()); a.setStreet(r.getStreet()); a.setHouseNumber(r.getHouseNumber()); a.setZipCode(r.getZipCode()); a.setCity(r.getCity()); a.setCountry(CountryCode.getByCode(r.getCountry())); return a; }Add import:
// at top with others import java.util.Objects;steve-core/src/main/java/de/rwth/idsg/steve/service/OcppTagsService.java (1)
59-61: Annotate nullable returnMethod returns null via orElse(null) but lacks @nullable, which can mislead callers.
- public OcppTagActivity getRecord(int ocppTagPk) { + public @org.jspecify.annotations.Nullable OcppTagActivity getRecord(int ocppTagPk) { return ocppTagRepository.getRecord(ocppTagPk).orElse(null); }steve-core/src/main/java/de/rwth/idsg/steve/repository/AddressRepository.java (1)
31-36: Annotate interface method parameter as @nullable
The implementation already guards againstnull(returnsOptional.empty()), so the interface should explicitly allownullinputs by annotating the parameter.File: steve-core/src/main/java/de/rwth/idsg/steve/repository/AddressRepository.java
- Optional<Address> get(Integer addressPk); + Optional<Address> get(@org.jspecify.annotations.Nullable Integer addressPk);No callers of
get(null)ordelete(null)were found in the codebase.steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/OcppTagMapper.java (1)
28-29: Nit: consider naming consistency.
If the DTO type is OcppTagActivity, renaming the class to OcppTagActivityMapper may improve discoverability alongside Address/ChargePoint/ChargingProfile mappers.steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ChargePointsController.java (2)
110-118: Guard against null/invalid protocol when building the registration status list.
If getDetails ever returns a DTO with an unknown protocol string, OcppProtocol.fromCompositeValue could throw and break the page. Prefer a safe fallback rather than propagating an IAE from the controller.Apply:
- model.addAttribute("registrationStatusList", getRegistrationStatusList(cp)); + model.addAttribute("registrationStatusList", getRegistrationStatusList(cp));(and adjust the helper below per next comment)
123-134: Graceful fallback for unknown OCPP versions.
Avoid throwing in the controller; default to upToOcpp15RegistrationStatusList and optionally log.- private static List<String> getRegistrationStatusList(ChargePoint.Details chargePoint) { - if (chargePoint.getOcppProtocol() == null) { - return upToOcpp15RegistrationStatusList; - } - - var protocol = OcppProtocol.fromCompositeValue(chargePoint.getOcppProtocol()); - return switch (protocol.getVersion()) { - case V_12, V_15 -> upToOcpp15RegistrationStatusList; - case V_16 -> ocpp16RegistrationStatusList; - default -> throw new IllegalArgumentException("Unknown OCPP version: " + protocol.getVersion()); - }; - } + private static List<String> getRegistrationStatusList(ChargePoint.Details chargePoint) { + var value = chargePoint.getOcppProtocol(); + if (value == null || value.isBlank()) { + return upToOcpp15RegistrationStatusList; + } + try { + var protocol = OcppProtocol.fromCompositeValue(value); + return switch (protocol.getVersion()) { + case V_12, V_15 -> upToOcpp15RegistrationStatusList; + case V_16 -> ocpp16RegistrationStatusList; + }; + } catch (IllegalArgumentException e) { + // Unknown/unsupported protocol; keep UI functional with a sensible default. + return upToOcpp15RegistrationStatusList; + } + }steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (1)
35-51: Handle null periods defensively and use toList() when on Java 16+.
Avoid NPE if repository passes null; toList() improves readability if the baseline allows it.- .periods(periods.stream().map(ChargingProfileMapper::fromRecord).collect(Collectors.toList())) + .periods((periods == null ? List.<ChargingSchedulePeriodRecord>of() : periods) + .stream() + .map(ChargingProfileMapper::fromRecord) + .toList())steve/src/test/java/de/rwth/idsg/steve/utils/__DatabasePreparer__.java (2)
160-165: Avoid NPE on missing charge box ID in tests.
pkMap.get(chargeboxID) may return null and cause unboxing NPE. Throw a descriptive error instead.- int pk = pkMap.get(chargeboxID); - return impl.getDetails(pk).orElseThrow(); + Integer pk = pkMap.get(chargeboxID); + if (pk == null) { + throw new IllegalStateException("ChargeBoxId not found: " + chargeboxID); + } + return impl.getDetails(pk).orElseThrow();
72-84: Resource lifecycle: consider a shutdown hook for the test DSL/DataSource.
To avoid leaking connections across test suites, expose a close() to shut down the DataSource/DSLContext when done.private static final JooqConfiguration jooqConfiguration = new JooqConfiguration(); private static DSLContext dslContext; public static void prepare(SteveConfiguration config) { @@ runOperation(ctx -> { truncateTables(ctx); insertChargeBox(ctx); insertOcppIdTag(ctx); }); } + + public static void close() { + try { + if (dslContext != null) { + var ds = dslContext.configuration().dataSource(); + dslContext.close(); + if (ds instanceof AutoCloseable ac) { + ac.close(); + } + } + } catch (Exception ignore) { + // best-effort cleanup for tests + } + }steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/TransactionMapper.java (1)
43-48: Minor: avoid duplicate column reads when formatting timestamps.
Cache start/stop LDT in locals before passing to toInstant and humanize. Micro-optimization/readability only.- .startTimestamp(toInstant(r.get(TRANSACTION.START_TIMESTAMP))) - .startTimestampFormatted(DateTimeUtils.humanize(r.get(TRANSACTION.START_TIMESTAMP))) + .startTimestamp(toInstant(r.get(TRANSACTION.START_TIMESTAMP))) + .startTimestampFormatted(DateTimeUtils.humanize(r.get(TRANSACTION.START_TIMESTAMP))) .startValue(r.get(TRANSACTION.START_VALUE)) - .stopTimestamp(toInstant(r.get(TRANSACTION.STOP_TIMESTAMP))) - .stopTimestampFormatted(DateTimeUtils.humanize(r.get(TRANSACTION.STOP_TIMESTAMP))) + .stopTimestamp(toInstant(r.get(TRANSACTION.STOP_TIMESTAMP))) + .stopTimestampFormatted(DateTimeUtils.humanize(r.get(TRANSACTION.STOP_TIMESTAMP)))(If preferred, introduce locals startTs = r.get(...), stopTs = r.get(...).)
steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (3)
50-55: Avoid Optional.orElse(null) to reduce nullable flowUse Optional’s control flow to keep the method null-free and self-documenting.
Example:
- var tag = ocppTagRepository.getRecord(idTag).orElse(null); - if (tag == null) { + var tagOpt = ocppTagRepository.getRecord(idTag); + if (tagOpt.isEmpty()) { log.error("The user with idTag '{}' is INVALID (not present in DB).", idTag); return new IdTagInfo().withStatus(AuthorizationStatus.INVALID); } + var tag = tagOpt.get();
64-65: Stabilize time and logging consistency
- Compute Instant once to avoid tiny race between checks.
- Use the same idTag placeholder in logs for consistency.
- if (tag.isExpired(Instant.now())) { + var now = Instant.now(); + if (tag.isExpired(now)) { ... - log.debug("The user with idTag '{}' is ACCEPTED.", tag.getIdTag()); + log.debug("The user with idTag '{}' is ACCEPTED.", idTag);Also applies to: 81-86
91-104: Rename param and align default-expiry with configured offset handling
- Rename parameter to tag for clarity.
- Compute fallback expiry via DateTimeUtils to keep zone handling consistent with DB conversions.
- private @Nullable OffsetDateTime getExpiryDateOrDefault(OcppTagActivity record) { - if (record.getExpiryDate() != null) { - return toOffsetDateTime(record.getExpiryDate()); + private @Nullable OffsetDateTime getExpiryDateOrDefault(OcppTagActivity tag) { + if (tag.getExpiryDate() != null) { + return toOffsetDateTime(tag.getExpiryDate()); } ... - } else { - return OffsetDateTime.now().plusHours(hoursToExpire); - } + } else { + // Use the same conversion path as real DB timestamps + return toOffsetDateTime(Instant.now().plus(hoursToExpire, java.time.temporal.ChronoUnit.HOURS)); + }steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/config/JooqConfiguration.java (3)
96-101: Make the DataSource transaction-aware for SpringWrap the DataSource to ensure jOOQ participates in Spring-managed transactions (without relying solely on jOOQ-local transactions).
- var conf = new DefaultConfiguration() - .set(SQLDialect.MYSQL) - .set(new DataSourceConnectionProvider(dataSource)) - .set(settings); + var conf = new DefaultConfiguration() + .set(SQLDialect.MYSQL) + .set(new DataSourceConnectionProvider( + new org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy(dataSource))) + .set(settings);Optionally add SpringTransactionProvider if you use Spring’s @transactional around jOOQ code paths.
50-70: Expose pool sizing via configConsider wiring maximumPoolSize/minimumIdle from SteveConfiguration for operability.
63-65: Use valid connectionTimeZone values and modern sslMode for SSL
- Ensure
dbTimeZoneIdpassed toPropertyKey.connectionTimeZoneis one of: "LOCAL", "SERVER", or any Java ZoneId/offset string (e.g., "UTC", "Europe/Berlin", "+02:00") (dev.mysql.com)- Replace legacy
PropertyKey.useSSL=truewith thesslModeproperty (e.g., REQUIRED, VERIFY_CA, VERIFY_IDENTITY) and configure truststore/keystore settings as needed (dev.mysql.com)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (2)
138-142: Short-circuit on empty idTagListAvoid emitting a useless SQL “IN ()/1=0” by returning early.
public List<OcppTagActivity> getRecords(List<String> idTagList) { + if (idTagList == null || idTagList.isEmpty()) { + return List.of(); + } return ctx.selectFrom(OCPP_TAG_ACTIVITY) .where(OCPP_TAG_ACTIVITY.ID_TAG.in(idTagList)) .fetch(OcppTagMapper::fromRecord); }
209-213: Minor: streamline batch creationYou can use toList() directly.
- var batch = - idTagList.stream().map(s -> ctx.newRecord(OCPP_TAG).setIdTag(s)).collect(Collectors.toList()); + var batch = idTagList.stream() + .map(s -> ctx.newRecord(OCPP_TAG).setIdTag(s)) + .toList();steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/UserRepositoryImpl.java (1)
162-166: Search by name: preserve word boundaryInsert a space between first and last name to improve matching quality.
- var joinedField = DSL.concat(USER.FIRST_NAME, USER.LAST_NAME); + var joinedField = DSL.concat(USER.FIRST_NAME, DSL.inline(" "), USER.LAST_NAME);steve-core/src/main/java/de/rwth/idsg/steve/service/TransactionStopService.java (1)
48-53: Log missing transaction detailsReturn is fine, but add a debug/warn for traceability.
- if (thisTxDetails == null) { - return; - } + if (thisTxDetails == null) { + // No-op: transaction not found (may have been deleted or already finalized) + return; + }steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
429-435: Optional: Short-circuit if connector missing
getConnectorPkFromConnectorwill NPE onfetchOne()when connector is missing. ConsiderfetchOptional(...).orElseThrow(...)for consistency with other guards.steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/User.java (1)
47-62: Consider type-safety for sex and email naming consistency
sexasStringinvites invalid values; consider using a small enum likeUserSexat the DTO layer (mappers can still convert).steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (1)
193-200: Serialize authorities deterministically (optional)For stable diffs and caching, consider sorting before serialization.
- private JSON toJSON(Set<String> authorities) { + private JSON toJSON(Set<String> authorities) { try { - return JSON.valueOf(objectMapper.writeValueAsString(authorities)); + var sorted = authorities == null ? List.<String>of() + : authorities.stream().sorted().toList(); + return JSON.valueOf(objectMapper.writeValueAsString(sorted)); } catch (JsonProcessingException e) { log.error("Failed to serialize authorities to JSON", e); return JSON.valueOf("[]"); } }steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (3)
246-253: Form translation is fine; consider a direct enum-based helper.Current mapping goes enum -> name ->
fromAuthorities(Set<String>). If an overload acceptingSet<WebUserAuthority>exists, use it to avoid string churn.
193-195: Prefer primitive for counts to avoid null checks.
getUserCountWithAuthorityreturningIntegerforces null handling here. Returningintsimplifies the API and callers.
315-334: Remove unused JSON (de)serializer methods
fromJsonandtoJsonin WebUsersService.java are never referenced and can be safely deleted to reduce surface area.steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (2)
42-43: Return primitive for counts.
Integer getUserCountWithAuthority(String authority)->intavoids nullable returns and simplifies callers (seeWebUsersService.hasUserWithAuthority).Apply:
- Integer getUserCountWithAuthority(String authority); + int getUserCountWithAuthority(String authority);
56-57: Repository should not depend on web-layer forms.
List<WebUserOverview> getOverview(WebUserQueryForm form)ties repository toweb.dto. Introduce a repository-level query/filter DTO and map from web form in the service layer.Suggested outline:
- New:
repository.dto.WebUserQuery { String usernameLike; Boolean enabled; Set<WebUserAuthority> authorities; ... }- Change repository to accept
WebUserQuery.- Map
WebUserQueryForm->WebUserQuerywithinWebUsersService.
| @Getter | ||
| @RequiredArgsConstructor | ||
| @Builder | ||
| public static final class Details { | ||
| private final ChargingProfileRecord profile; | ||
| private final List<ChargingSchedulePeriodRecord> periods; | ||
| // from ChargingProfileRecord | ||
| private final int chargingProfilePk; | ||
| private final int stackLevel; | ||
| private final String chargingProfilePurpose; | ||
| private final String chargingProfileKind; | ||
| private final String recurrencyKind; | ||
| private final Instant validFrom; | ||
| private final Instant validTo; | ||
| private final Integer durationInSeconds; | ||
| private final Instant startSchedule; | ||
| private final String chargingRateUnit; | ||
| private final BigDecimal minChargingRate; | ||
| // from ChargingSchedulePeriodRecord | ||
| private final List<ChargingSchedulePeriod> periods; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include description and note in Details to prevent data loss in edit flows.
ChargingProfileForm.fromDetails(...) can’t populate description/note because Details doesn’t expose them. Editing other fields risks overwriting these columns with nulls on update.
@Getter
@Builder
public static final class Details {
- // from ChargingProfileRecord
+ // from ChargingProfileRecord
private final int chargingProfilePk;
private final int stackLevel;
+ private final String description;
+ private final String note;
private final String chargingProfilePurpose;
private final String chargingProfileKind;
private final String recurrencyKind;
private final Instant validFrom;
private final Instant validTo;
private final Integer durationInSeconds;
private final Instant startSchedule;
private final String chargingRateUnit;
private final BigDecimal minChargingRate;
// from ChargingSchedulePeriodRecord
private final List<ChargingSchedulePeriod> periods;
}Follow-up: update the mapper and ChargingProfileForm.fromDetails(...) to copy these two fields.
📝 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.
| @Getter | |
| @RequiredArgsConstructor | |
| @Builder | |
| public static final class Details { | |
| private final ChargingProfileRecord profile; | |
| private final List<ChargingSchedulePeriodRecord> periods; | |
| // from ChargingProfileRecord | |
| private final int chargingProfilePk; | |
| private final int stackLevel; | |
| private final String chargingProfilePurpose; | |
| private final String chargingProfileKind; | |
| private final String recurrencyKind; | |
| private final Instant validFrom; | |
| private final Instant validTo; | |
| private final Integer durationInSeconds; | |
| private final Instant startSchedule; | |
| private final String chargingRateUnit; | |
| private final BigDecimal minChargingRate; | |
| // from ChargingSchedulePeriodRecord | |
| private final List<ChargingSchedulePeriod> periods; | |
| } | |
| @Getter | |
| @Builder | |
| public static final class Details { | |
| // from ChargingProfileRecord | |
| private final int chargingProfilePk; | |
| private final int stackLevel; | |
| private final String description; | |
| private final String note; | |
| private final String chargingProfilePurpose; | |
| private final String chargingProfileKind; | |
| private final String recurrencyKind; | |
| private final Instant validFrom; | |
| private final Instant validTo; | |
| private final Integer durationInSeconds; | |
| private final Instant startSchedule; | |
| private final String chargingRateUnit; | |
| private final BigDecimal minChargingRate; | |
| // from ChargingSchedulePeriodRecord | |
| private final List<ChargingSchedulePeriod> periods; | |
| } |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java
around lines 59-76, the Details DTO lacks description and note so edit flows can
drop those values; add final String description and final String note to the
Details class (with @Getter/@Builder) and update the mapper that constructs
Details from DB records to populate these two fields from ChargingProfileRecord,
then update ChargingProfileForm.fromDetails(...) to copy description and note
into the form so edits do not overwrite existing values with null.
| public TransactionDetails.@Nullable MeterValues findLastMeterValue() { | ||
| var v = values.stream() | ||
| .filter(TransactionStopServiceHelper::isEnergyValue) | ||
| .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp)) | ||
| .orElse(null); | ||
|
|
||
| // if the list of values is empty, we fall to this case, as well. |
There was a problem hiding this comment.
Avoid NPE when selecting last meter value (null timestamps).
Comparator.comparing will throw if any valueTimestamp is null. Filter nulls (or use nullsLast) before max; also guarantees non-null stopTimestamp in path 1.
Apply:
- var v = values.stream()
- .filter(TransactionStopServiceHelper::isEnergyValue)
- .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp))
- .orElse(null);
+ var v = values.stream()
+ .filter(TransactionStopServiceHelper::isEnergyValue)
+ .filter(mv -> mv.getValueTimestamp() != null)
+ .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp))
+ .orElse(null);📝 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 TransactionDetails.@Nullable MeterValues findLastMeterValue() { | |
| var v = values.stream() | |
| .filter(TransactionStopServiceHelper::isEnergyValue) | |
| .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp)) | |
| .orElse(null); | |
| // if the list of values is empty, we fall to this case, as well. | |
| public TransactionDetails.@Nullable MeterValues findLastMeterValue() { | |
| var v = values.stream() | |
| .filter(TransactionStopServiceHelper::isEnergyValue) | |
| .filter(mv -> mv.getValueTimestamp() != null) | |
| .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp)) | |
| .orElse(null); | |
| // if the list of values is empty, we fall to this case, as well. | |
| } |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java
around lines 68 to 74, the Comparator.comparing call can throw an NPE if any
MeterValues.valueTimestamp is null; modify the stream to either filter out
entries with null valueTimestamp before calling max (e.g. add .filter(mv ->
mv.getValueTimestamp() != null) after the isEnergyValue filter) or use
Comparator.comparing(MeterValues::getValueTimestamp, Comparator.nullsLast(...))
so null timestamps are ordered last; ensure the chosen approach still guarantees
the non-null stopTimestamp path remains valid and return the max (or null) as
before.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java
Show resolved
Hide resolved
| if (nextTransactionStart != null) { | ||
| // some charging stations do not reset the meter value counter after each transaction and | ||
| // continue counting. in such cases, use the value of subsequent transaction's start value | ||
| if (Integer.parseInt(nextTransactionStart.getStartValue()) | ||
| > Integer.parseInt(transaction.getStartValue())) { | ||
| return TerminationValues.builder() | ||
| .stopValue(nextTransactionStart.getStartValue()) | ||
| .stopTimestamp(nextTransactionStart.getStartTimestamp()) | ||
| .build(); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use wide, safe numeric comparison for start values.
Integer.parseInt risks overflow for large meter counters and is unnecessary. Parse as long once and compare.
Apply:
- if (Integer.parseInt(nextTransactionStart.getStartValue())
- > Integer.parseInt(transaction.getStartValue())) {
+ long nextStart = Long.parseLong(nextTransactionStart.getStartValue());
+ long currentStart = Long.parseLong(transaction.getStartValue());
+ if (nextStart > currentStart) {📝 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 (nextTransactionStart != null) { | |
| // some charging stations do not reset the meter value counter after each transaction and | |
| // continue counting. in such cases, use the value of subsequent transaction's start value | |
| if (Integer.parseInt(nextTransactionStart.getStartValue()) | |
| > Integer.parseInt(transaction.getStartValue())) { | |
| return TerminationValues.builder() | |
| .stopValue(nextTransactionStart.getStartValue()) | |
| .stopTimestamp(nextTransactionStart.getStartTimestamp()) | |
| .build(); | |
| } else { | |
| if (nextTransactionStart != null) { | |
| // some charging stations do not reset the meter value counter after each transaction and | |
| // continue counting. in such cases, use the value of subsequent transaction's start value | |
| long nextStart = Long.parseLong(nextTransactionStart.getStartValue()); | |
| long currentStart = Long.parseLong(transaction.getStartValue()); | |
| if (nextStart > currentStart) { | |
| return TerminationValues.builder() | |
| .stopValue(nextTransactionStart.getStartValue()) | |
| .stopTimestamp(nextTransactionStart.getStartTimestamp()) | |
| .build(); | |
| } else { |
🤖 Prompt for AI Agents
In
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java
around lines 121 to 130, the code currently uses Integer.parseInt for start
value comparison which can overflow; change to parse each startValue to a long
once (use Long.parseLong), store into local long variables, and compare those
longs (using >) to decide whether to build the TerminationValues from
nextTransactionStart; ensure you replace both Integer.parseInt calls with the
long parsing and variable usage (optionally wrap parsing in a try/catch if
inputs might be malformed).
steve-core/src/main/java/de/rwth/idsg/steve/repository/TransactionRepository.java
Show resolved
Hide resolved
| var addressPk = selectAddressId(userPk); | ||
| addressRepository.delete(addressPk); | ||
| deleteInternal(tx, userPk); |
There was a problem hiding this comment.
Potential FK violation: deleting address before user
If USER.ADDRESS_PK references ADDRESS, deleting the address first can fail. Either delete the user first or nullify USER.ADDRESS_PK before address deletion; also guard null addressPk.
- var addressPk = selectAddressId(userPk);
- addressRepository.delete(addressPk);
- deleteInternal(tx, userPk);
+ var addressPk = selectAddressId(userPk);
+ // Delete the user first (removes FK), then the address if present
+ deleteInternal(tx, userPk);
+ if (addressPk != null) {
+ addressRepository.delete(addressPk);
+ }📝 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.
| var addressPk = selectAddressId(userPk); | |
| addressRepository.delete(addressPk); | |
| deleteInternal(tx, userPk); | |
| var addressPk = selectAddressId(userPk); | |
| // Delete the user first (removes FK), then the address if present | |
| deleteInternal(tx, userPk); | |
| if (addressPk != null) { | |
| addressRepository.delete(addressPk); | |
| } |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/UserRepositoryImpl.java
around lines 126-128, the code deletes the ADDRESS row before removing the USER
or nullifying the USER.ADDRESS_PK which can violate the foreign key; change the
sequence to guard nulls and avoid FK violations by first read addressPk, if
addressPk is null just call deleteInternal(tx, userPk), otherwise either (A)
call deleteInternal(tx, userPk) first and then
addressRepository.delete(addressPk) within the same transaction, or (B) update
the USER row to set ADDRESS_PK = null within tx, flush/apply that change, then
delete the ADDRESS; ensure you check for null addressPk before calling
addressRepository.delete and keep all operations inside the provided
transaction.
| public void createUser(WebUser user) { | ||
| ctx.insertInto(WEB_USER) | ||
| .set(WEB_USER.USERNAME, user.getUsername()) | ||
| .set(WEB_USER.USERNAME, user.getLogin()) | ||
| .set(WEB_USER.PASSWORD, user.getPassword()) | ||
| .set(WEB_USER.API_PASSWORD, user.getApiPassword()) | ||
| .set(WEB_USER.ENABLED, user.getEnabled()) | ||
| .set(WEB_USER.AUTHORITIES, user.getAuthorities()) | ||
| .set(WEB_USER.ENABLED, user.isEnabled()) | ||
| .set( | ||
| WEB_USER.AUTHORITIES, | ||
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | ||
| .execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null authorities when creating users
user.getAuthorities() may be null; .stream() would NPE. Default to empty set before serializing.
- .set(
- WEB_USER.AUTHORITIES,
- toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))
+ .set(
+ WEB_USER.AUTHORITIES,
+ toJSON(user.getAuthorities() == null
+ ? Set.of()
+ : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))📝 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 void createUser(WebUser user) { | |
| ctx.insertInto(WEB_USER) | |
| .set(WEB_USER.USERNAME, user.getUsername()) | |
| .set(WEB_USER.USERNAME, user.getLogin()) | |
| .set(WEB_USER.PASSWORD, user.getPassword()) | |
| .set(WEB_USER.API_PASSWORD, user.getApiPassword()) | |
| .set(WEB_USER.ENABLED, user.getEnabled()) | |
| .set(WEB_USER.AUTHORITIES, user.getAuthorities()) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .execute(); | |
| public void createUser(WebUser user) { | |
| ctx.insertInto(WEB_USER) | |
| .set(WEB_USER.USERNAME, user.getLogin()) | |
| .set(WEB_USER.PASSWORD, user.getPassword()) | |
| .set(WEB_USER.API_PASSWORD, user.getApiPassword()) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities() == null | |
| ? Set.of() | |
| : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .execute(); | |
| } |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
around lines 62 to 71, guard against a possible NPE from user.getAuthorities()
being null by defaulting to an empty set before streaming/serializing; replace
the direct user.getAuthorities().stream() call with a null-safe expression
(e.g., use
Optional.ofNullable(user.getAuthorities()).orElse(Collections.emptySet()) or
Objects.requireNonNullElse(user.getAuthorities(), Collections.emptySet())) and
then map Enum::name and collect to a set before passing to toJSON so
serialization never receives null.
| public void updateUser(WebUser user) { | ||
| // To change the password use one of the changePassword methods | ||
| ctx.update(WEB_USER) | ||
| .set(WEB_USER.ENABLED, user.getEnabled()) | ||
| .set(WEB_USER.AUTHORITIES, user.getAuthorities()) | ||
| .where(WEB_USER.USERNAME.eq(user.getUsername())) | ||
| .set(WEB_USER.ENABLED, user.isEnabled()) | ||
| .set( | ||
| WEB_USER.AUTHORITIES, | ||
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | ||
| .where(WEB_USER.USERNAME.eq(user.getLogin())) | ||
| .execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same null-guard for update by username
Apply the same defaulting pattern here.
- .set(
- WEB_USER.AUTHORITIES,
- toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))
+ .set(
+ WEB_USER.AUTHORITIES,
+ toJSON(user.getAuthorities() == null
+ ? Set.of()
+ : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))📝 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 void updateUser(WebUser user) { | |
| // To change the password use one of the changePassword methods | |
| ctx.update(WEB_USER) | |
| .set(WEB_USER.ENABLED, user.getEnabled()) | |
| .set(WEB_USER.AUTHORITIES, user.getAuthorities()) | |
| .where(WEB_USER.USERNAME.eq(user.getUsername())) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .where(WEB_USER.USERNAME.eq(user.getLogin())) | |
| .execute(); | |
| public void updateUser(WebUser user) { | |
| // To change the password use one of the changePassword methods | |
| ctx.update(WEB_USER) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities() == null | |
| ? Set.of() | |
| : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .where(WEB_USER.USERNAME.eq(user.getLogin())) | |
| .execute(); |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
around lines 75 to 83, the updateUser method uses user.getAuthorities() directly
which can be null; mirror the same null-guard/defaulting pattern used elsewhere
by defaulting authorities to an empty set before mapping to names and
serializing. Replace the direct user.getAuthorities().stream() call with a
guarded expression (e.g. use
Optional.ofNullable(user.getAuthorities()).orElse(Collections.emptySet()) or a
ternary) then map Enum::name, collect to a set, pass that to toJSON, and keep
the rest of the update unchanged.
| .set(WEB_USER.USERNAME, user.getLogin()) | ||
| .set(WEB_USER.ENABLED, user.isEnabled()) | ||
| .set( | ||
| WEB_USER.AUTHORITIES, | ||
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | ||
| .where(WEB_USER.WEB_USER_PK.eq(user.getWebUserPk())) | ||
| .execute(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same null-guard for update by PK
Apply the same handling here.
- .set(
- WEB_USER.AUTHORITIES,
- toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))
+ .set(
+ WEB_USER.AUTHORITIES,
+ toJSON(user.getAuthorities() == null
+ ? Set.of()
+ : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))📝 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.
| .set(WEB_USER.USERNAME, user.getLogin()) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .where(WEB_USER.WEB_USER_PK.eq(user.getWebUserPk())) | |
| .execute(); | |
| } | |
| .set(WEB_USER.USERNAME, user.getLogin()) | |
| .set(WEB_USER.ENABLED, user.isEnabled()) | |
| .set( | |
| WEB_USER.AUTHORITIES, | |
| toJSON(user.getAuthorities() == null | |
| ? Set.of() | |
| : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) | |
| .where(WEB_USER.WEB_USER_PK.eq(user.getWebUserPk())) | |
| .execute(); | |
| } |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
around lines 90-97, the update-by-PK code calls user.getAuthorities().stream()
without a null-guard; add the same null handling used elsewhere: ensure user and
user.getWebUserPk() are validated (return/throw if PK is null), and replace the
authorities stream with a null-safe expression (e.g., use Collections.emptySet()
when user.getAuthorities() is null) before converting to JSON so toJSON never
receives a null or causes an NPE.
| var form = ChargingProfileForm.fromDetails(details); | ||
|
|
There was a problem hiding this comment.
Risk: description/note may be dropped in the details form.
After switching to ChargingProfileForm.fromDetails(details), the form doesn’t get description/note (DTO lacks them), so updates can null them out.
Action:
- Extend
ChargingProfile.Detailsto includedescriptionandnote(see suggested diff in ChargingProfile.java). - Populate them in
ChargingProfileForm.fromDetails(...). - Ensure repository update preserves existing values if the form omits them (defensive server-side merge).
🤖 Prompt for AI Agents
In
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ChargingProfilesController.java
around lines 132-133, switching to ChargingProfileForm.fromDetails(details)
causes description/note to be dropped because ChargingProfile.Details and the
form factory don’t carry those fields; update ChargingProfile.Details to add
description and note (with getters/setters), update
ChargingProfileForm.fromDetails(...) to populate form.description and form.note
from details, and change the repository update path to perform a defensive
server-side merge that preserves existing description/note when the incoming
form omits them (i.e., read existing entity, copy non-null incoming fields onto
it, then save).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (2)
114-129: API now returns Optional — verify all callers unwrap safely.Ensure services/controllers don’t pass
Optional<TransactionDetails>into models or assume presence.#!/bin/bash # Find and review all call sites of getDetails(...) rg -nP --type=java -C2 '\bgetDetails\s*\(\s*int\s*\)'
170-176: Make the upper bound exclusive to avoid attributing the next transaction’s first meter value.
between(start, nextStart)is inclusive; switch to [>= start AND < nextStart].- } else { - timestampCondition = - CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.between(startTimestamp, nextTxRecord.getStartTimestamp()); - } + } else { + timestampCondition = + CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.ge(startTimestamp) + .and(CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.lt(nextTxRecord.getStartTimestamp())); + }steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
299-307: Same NPE risk on connectorPk in update flow — fix like insertMeterValuesfetchOne().value1() will NPE when no row exists. Mirror the defensive pattern you used above.
- var connectorPk = DSL.select(TRANSACTION_START.CONNECTOR_PK) - .from(TRANSACTION_START) - .where(TRANSACTION_START.TRANSACTION_PK.equal(p.getTransactionId())) - .fetchOne() - .value1(); + var connectorPk = DSL.select(TRANSACTION_START.CONNECTOR_PK) + .from(TRANSACTION_START) + .where(TRANSACTION_START.TRANSACTION_PK.equal(p.getTransactionId())) + .fetchOptional(TRANSACTION_START.CONNECTOR_PK) + .orElseThrow(() -> new SteveException.InternalError( + "No connectorPk for transactionId=" + p.getTransactionId()));steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
60-78: Past data-loss risk fixed; Details now carries description/note.This addresses the earlier edit-flow issue where these fields could be lost. Nicely done.
steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (1)
21-26: Fix layering: repository must not depend on service DTOs.Importing
de.rwth.idsg.steve.service.dto.WebUserOverviewfrom a repository interface inverts layers. MoveWebUserOverviewto a repository/core DTO package and update the import.Apply:
-import de.rwth.idsg.steve.service.dto.WebUserOverview; +import de.rwth.idsg.steve.repository.dto.WebUserOverview;#!/bin/bash # Find repository-layer usages of service DTOs rg -nP 'de\.rwth\.idsg\.steve\.service\.dto\.WebUserOverview|service\.dto' -C2 steve-core/src/main/java/de/rwth/idsg/steve/repositoryAlso applies to: 56-57
steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (3)
159-166: Unify authority expansion across UI and API.Here you flatten via
getValues(); API usesname(). Keep a single expansion to avoid divergent permissions.- .authorities(webUser.getAuthorities().stream() - .map(WebUserAuthority::getValues) - .flatMap(Set::stream) - .toArray(String[]::new)) + .authorities(expandAuthorities(webUser.getAuthorities()))Add once in this class:
private static String[] expandAuthorities(Set<WebUserAuthority> auths) { return auths.stream() .map(WebUserAuthority::getValues) .flatMap(java.util.Collection::stream) .toArray(String[]::new); }Then update the API variant similarly (see Lines 276-278).
262-279: Make API authority expansion consistent with UI.Replace
.map(WebUserAuthority::name)with the same expansion used in the UI builder.- .authorities(webUser.getAuthorities().stream() - .map(WebUserAuthority::name) - .toArray(String[]::new)) + .authorities(expandAuthorities(webUser.getAuthorities()))
282-295: Harden authority parsing fromUserDetails.
valueOf(uppercased)will fail for inputs likeROLE_ADMIN. Use a normalizing factory (e.g.,fromAuthority) and handle unknowns explicitly.- var authorities = user.getAuthorities().stream() - .map(GrantedAuthority::getAuthority) - .map(String::toUpperCase) - .map(WebUserAuthority::valueOf) - .collect(Collectors.toSet()); + var authorities = user.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .map(WebUserAuthority::fromAuthority) // normalize "ROLE_" and case + .flatMap(java.util.Optional::stream) + .collect(Collectors.toSet());If
fromAuthorityis not available, add it on the enum.
🧹 Nitpick comments (25)
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java (1)
97-100: Use template-based NotFound constructor; verify 404 mapping and JSP expectations.
Prefer the formatting-friendly ctor to avoid string concatenation, consistent with SteveException guidance. Also, confirm NotFound is mapped to HTTP 404 by your global handler and that transactionDetails.jsp expects a concrete details object (not Optional).Apply this diff:
- .orElseThrow(() -> new SteveException.NotFound("Transaction with pk " + transactionPk + " not found.")); + .orElseThrow(() -> new SteveException.NotFound("Transaction with pk %d not found.", transactionPk));steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (4)
197-205: Avoid unnecessary duplicate-elimination work in UNION; prefer UNION ALL + anti-dup predicate.As written, both branches always execute and a dedup step runs. Filter the timestamp branch to exclude rows that already match by transaction_pk, then use
unionAll(...).var timestampQuery = ctx.selectFrom(CONNECTOR_METER_VALUE) .where(CONNECTOR_METER_VALUE.CONNECTOR_PK.eq(ctx.select(CONNECTOR.CONNECTOR_PK) .from(CONNECTOR) .where(CONNECTOR.CHARGE_BOX_ID.equal(chargeBoxId)) .and(CONNECTOR.CONNECTOR_ID.equal(connectorId)))) .and(timestampCondition) + // avoid duplicates already returned by transactionQuery + .and(CONNECTOR_METER_VALUE.TRANSACTION_PK.isNull() + .or(CONNECTOR_METER_VALUE.TRANSACTION_PK.ne(transactionPk))) .and(unitCondition) .getQuery(); -var t1 = transactionQuery.union(timestampQuery).asTable("t1"); +var t1 = transactionQuery.unionAll(timestampQuery).asTable("t1");Also applies to: 212-213
223-236: Reduce fragility: use field-based getters instead of positionalvalueN()access.Index-based access couples mapping to select order. Use
r.get(field)for stability.- .fetch() - .map(r -> TransactionDetails.MeterValues.builder() - .valueTimestamp(toInstant(r.value1())) - .value(r.value2()) - .readingContext(r.value3()) - .format(r.value4()) - .measurand(r.value5()) - .location(r.value6()) - .unit(r.value7()) - .phase(r.value8()) - .build()) + .fetch() + .map(r -> TransactionDetails.MeterValues.builder() + .valueTimestamp(toInstant(r.get(ts))) + .value(r.get(val)) + .readingContext(r.get(ctxf)) + .format(r.get(fmt)) + .measurand(r.get(mea)) + .location(r.get(loc)) + .unit(r.get(unit)) + .phase(r.get(ph)) + .build())
286-318: Eliminate raw types for SelectQuery helpers.Use generics to avoid unchecked warnings and keep type-safety.
-private SelectQuery addConditions(SelectQuery selectQuery, TransactionQueryForm form) { +private <R extends Record> SelectQuery<R> addConditions(SelectQuery<R> selectQuery, TransactionQueryForm form) { if (form.isTransactionPkSet()) { selectQuery.addConditions(TRANSACTION.TRANSACTION_PK.eq(form.getTransactionPk())); } ... return selectQuery; }
319-342: Match the above: generic signature forprocessType.Keeps helper signatures consistent and type-safe.
-private void processType(SelectQuery selectQuery, TransactionQueryForm form) { +private <R extends Record> void processType(SelectQuery<R> selectQuery, TransactionQueryForm form) { switch (form.getPeriodType()) { case TODAY -> ...steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (3)
56-87: Reduce duplication when building IdTagInfoThe four branches repeat parent/expiry wiring. Consider a small helper to keep this DRY and consistent.
Apply within this range:
- return new IdTagInfo() - .withStatus(AuthorizationStatus.BLOCKED) - .withParentIdTag(tag.getParentIdTag()) - .withExpiryDate(getExpiryDateOrDefault(tag)); + return buildIdTagInfo(AuthorizationStatus.BLOCKED, tag); ... - return new IdTagInfo() - .withStatus(AuthorizationStatus.EXPIRED) - .withParentIdTag(tag.getParentIdTag()) - .withExpiryDate(getExpiryDateOrDefault(tag)); + return buildIdTagInfo(AuthorizationStatus.EXPIRED, tag); ... - return new IdTagInfo() - .withStatus(AuthorizationStatus.CONCURRENT_TX) - .withParentIdTag(tag.getParentIdTag()) - .withExpiryDate(getExpiryDateOrDefault(tag)); + return buildIdTagInfo(AuthorizationStatus.CONCURRENT_TX, tag); ... - return new IdTagInfo() - .withStatus(AuthorizationStatus.ACCEPTED) - .withParentIdTag(tag.getParentIdTag()) - .withExpiryDate(getExpiryDateOrDefault(tag)); + return buildIdTagInfo(AuthorizationStatus.ACCEPTED, tag);Add outside this range:
private static IdTagInfo buildIdTagInfo(AuthorizationStatus status, OcppTagActivity tag) { return new IdTagInfo() .withStatus(status) .withParentIdTag(tag.getParentIdTag()) .withExpiryDate(getExpiryDateOrDefault(tag)); }I can push a follow-up commit with this refactor if you want.
82-82: Consistent logging placeholderUse the already-available idTag parameter for consistency.
- log.debug("The user with idTag '{}' is ACCEPTED.", tag.getIdTag()); + log.debug("The user with idTag '{}' is ACCEPTED.", idTag);
92-95: Rename parameter from record -> tag for clarityMinor naming nit to reflect the DTO.
- private @Nullable OffsetDateTime getExpiryDateOrDefault(OcppTagActivity record) { - if (record.getExpiryDate() != null) { - return toOffsetDateTime(record.getExpiryDate()); + private @Nullable OffsetDateTime getExpiryDateOrDefault(OcppTagActivity tag) { + if (tag.getExpiryDate() != null) { + return toOffsetDateTime(tag.getExpiryDate());Optionally, consider
OffsetDateTime.now(ZoneOffset.UTC)for deterministic timezone handling if clients expect UTC.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
429-436: Harden connectorPk retrieval helperfetchOne().value1() can NPE if the connector row is missing (race or inconsistent data). The callers usually insertIgnore first, but making the helper safe reduces blast radius.
- private int getConnectorPkFromConnector(DSLContext ctx, String chargeBoxIdentity, int connectorId) { - return ctx.select(CONNECTOR.CONNECTOR_PK) - .from(CONNECTOR) - .where(CONNECTOR.CHARGE_BOX_ID.equal(chargeBoxIdentity)) - .and(CONNECTOR.CONNECTOR_ID.equal(connectorId)) - .fetchOne() - .value1(); - } + private int getConnectorPkFromConnector(DSLContext ctx, String chargeBoxIdentity, int connectorId) { + return ctx.select(CONNECTOR.CONNECTOR_PK) + .from(CONNECTOR) + .where(CONNECTOR.CHARGE_BOX_ID.equal(chargeBoxIdentity)) + .and(CONNECTOR.CONNECTOR_ID.equal(connectorId)) + .fetchOptional(CONNECTOR.CONNECTOR_PK) + .orElseThrow(() -> new SteveException.InternalError( + "No connectorPk for %s/%d".formatted(chargeBoxIdentity, connectorId))); + }steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (1)
36-41: Clarify Optional semantics in Javadoc.Document when Optional.empty() is returned (e.g., fresh DB / no schema history), so callers know not to assume presence.
Apply:
/** - * Returns database version of SteVe and last database update timestamp + * Returns the database version of SteVe and the last database update timestamp. + * May return Optional.empty() if no schema history exists (e.g., fresh database). * */ - Optional<DbVersion> getDBVersion(); + Optional<DbVersion> getDBVersion();steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (3)
71-76: Avoid index-based getValue and handle potential nulls.Use a typed fetch to reduce fragility and guard against rare nulls.
Apply:
- var sql = ctx.select(timestampDiff(DatePart.SECOND, utcTimestamp(), DSL.currentTimestamp())) - .fetchOne() - .getValue(0, Long.class); + final var sql = ctx.select(timestampDiff(DatePart.SECOND, utcTimestamp(), DSL.currentTimestamp())) + .fetchOneInto(Long.class); - if (sql != java) { + if (sql == null || sql.longValue() != java) { throw new SteveException.InternalError( "MySQL and Java are not using the same time zone. " + "Java offset in seconds (%s) != MySQL offset in seconds (%s)", - java, sql); + java, sql); }Optional: rename vars for readability.
- var java = DateTimeUtils.getOffsetFromUtcInSeconds(); + final var java = DateTimeUtils.getOffsetFromUtcInSeconds();
125-136: Prefer fetchSingle() (or assert non-null) for a single-row projection.This query should always return one row; using fetchSingle() makes that explicit and prevents silent nulls.
Apply:
- var row = ctx.select( + var row = ctx.select( numChargeBoxes, numOcppTags, numUsers, numReservations, numTransactions, heartbeatsToday, heartbeatsYesterday, heartbeatsEarlier, numWebUsers) - .fetchOne(); + .fetchSingle();If your jOOQ version lacks fetchSingle(), keep fetchOne() but add a null check with a clear exception.
151-165: Use fetchOptional + ORDER BY/LIMIT for simpler, idiomatic jOOQ.Removes the max subquery and eliminates explicit null checks.
Apply:
- public Optional<DbVersion> getDBVersion() { - var schemaVersion = ctx.select(SCHEMA_VERSION.VERSION, SCHEMA_VERSION.INSTALLED_ON) - .from(SCHEMA_VERSION) - .where(SCHEMA_VERSION.INSTALLED_RANK.eq( - select(max(SCHEMA_VERSION.INSTALLED_RANK)).from(SCHEMA_VERSION))) - .fetchOne(); - if (schemaVersion == null) { - return Optional.empty(); - } - var ts = DateTimeUtils.humanize(schemaVersion.value2()); - return Optional.of(DbVersion.builder() - .version(schemaVersion.value1()) - .updateTimestamp(ts) - .build()); - } + public Optional<DbVersion> getDBVersion() { + return ctx.select(SCHEMA_VERSION.VERSION, SCHEMA_VERSION.INSTALLED_ON) + .from(SCHEMA_VERSION) + .orderBy(SCHEMA_VERSION.INSTALLED_RANK.desc()) + .limit(1) + .fetchOptional() + .map(r -> DbVersion.builder() + .version(r.value1()) + .updateTimestamp(DateTimeUtils.humanize(r.value2())) + .build()); + }steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (4)
56-58: Trim/dedupe recipients to avoid blank addressesGuard against extra spaces and duplicates coming from UI/config.
- private static List<String> parseRecipients(SettingsRecord r) { - return splitByComma(r.getMailRecipients()); - } + private static List<String> parseRecipients(SettingsRecord r) { + return splitByComma(r.getMailRecipients()).stream() + .map(String::trim) + .filter(s -> !s.isEmpty()) + .distinct() + .collect(Collectors.toList()); + }
60-62: Make parseEnabledFeatures static (match parseRecipients) and keep null-safety at the sourceSmall consistency tweak; also keeps method side-effect free.
- private List<NotificationFeature> parseEnabledFeatures(SettingsRecord r) { + private static List<NotificationFeature> parseEnabledFeatures(SettingsRecord r) { return splitFeatures(r.getNotificationFeatures()); }Additionally update the helper to static (outside the changed hunk):
private static List<NotificationFeature> splitFeatures(String str) { return splitByComma(str).stream() .map(NotificationFeature::fromName) .collect(Collectors.toList()); }
70-71: Minor: local var naming or inlineConsider using "emails" (lower camel) or inline into the builder to reduce locals.
Also applies to: 92-93
136-137: Enforce single-row invariant for SETTINGS.APP_IDTo avoid surprises if multiple rows appear:
- Add a unique index on SETTINGS.APP_ID.
- Ensure bootstrap migration inserts the row if missing.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (2)
76-78: Make periods immutable at the DTO boundary.Guard against accidental mutation by exposing an unmodifiable view.
Apply in-place and supporting change:
- private final List<ChargingSchedulePeriod> periods; + @Getter(lombok.AccessLevel.NONE) + private final List<ChargingSchedulePeriod> periods; + + public List<ChargingSchedulePeriod> getPeriods() { + return List.copyOf(periods); + }And add the missing import once in this file:
+import lombok.AccessLevel;
80-85: Optional: Document OCPP constraints on fields.Consider brief Javadoc on numberPhases (1..3) and powerLimit scale (<=1 fractional digit) to make the contract explicit at the DTO level.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java (3)
74-83: Null/empty safety for schedule periods.If details.getPeriods() is null or empty, request building may NPE or violate OCPP constraints. Fail fast with a clear message.
- var schedulePeriods = details.getPeriods().stream() + var periods = java.util.Objects.requireNonNull(details.getPeriods(), "Charging schedule periods must be present"); + if (periods.isEmpty()) { + throw new IllegalArgumentException("Charging schedule must contain at least one period"); + } + var schedulePeriods = periods.stream() .map(k -> { ChargingSchedulePeriod p = new ChargingSchedulePeriod(); p.setStartPeriod(k.getStartPeriodInSeconds()); p.setLimit(k.getPowerLimit()); p.setNumberPhases(k.getNumberPhases()); return p; }) .toList();
85-89: Guard required enum mapping.fromValue(..) will throw if chargingRateUnit is null/invalid. Add a precondition so failures are explicit.
- var schedule = new ChargingSchedule() - .withDuration(details.getDurationInSeconds()) - .withStartSchedule(toOffsetDateTime(details.getStartSchedule())) - .withChargingRateUnit(ChargingRateUnitType.fromValue(details.getChargingRateUnit())) + var schedule = new ChargingSchedule() + .withDuration(details.getDurationInSeconds()) + .withStartSchedule(toOffsetDateTime(details.getStartSchedule())) + .withChargingRateUnit(ChargingRateUnitType.fromValue( + java.util.Objects.requireNonNull(details.getChargingRateUnit(), "chargingRateUnit"))) .withMinChargingRate(details.getMinChargingRate())
91-96: Remove commented-out fields.OCPP 1.6 ChargingProfile doesn’t support description/note; keep the code clean by dropping commented lines.
- // .withDescription(details.getDescription()) - // .withNote(details.getNote())steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (1)
35-53: Handle null periods and standardize stream terminal operation.
- Be defensive if periods can be null.
- Use Stream.toList() for consistency with the rest of the codebase (Java 16+), unless you target older bytecode.
- public static ChargingProfile.Details fromRecord( - ChargingProfileRecord profile, List<ChargingSchedulePeriodRecord> periods) { + public static ChargingProfile.Details fromRecord( + ChargingProfileRecord profile, List<ChargingSchedulePeriodRecord> periods) { + var safePeriods = periods == null ? List.<ChargingSchedulePeriodRecord>of() : periods; return ChargingProfile.Details.builder() .chargingProfilePk(profile.getChargingProfilePk()) .stackLevel(profile.getStackLevel()) .description(profile.getDescription()) .note(profile.getNote()) .chargingProfilePurpose(profile.getChargingProfilePurpose()) .chargingProfileKind(profile.getChargingProfileKind()) .recurrencyKind(profile.getRecurrencyKind()) .validFrom(toInstant(profile.getValidFrom())) .validTo(toInstant(profile.getValidTo())) .durationInSeconds(profile.getDurationInSeconds()) .startSchedule(toInstant(profile.getStartSchedule())) .chargingRateUnit(profile.getChargingRateUnit()) .minChargingRate(profile.getMinChargingRate()) - .periods(periods.stream().map(ChargingProfileMapper::fromRecord).collect(Collectors.toList())) + .periods(safePeriods.stream().map(ChargingProfileMapper::fromRecord).toList()) .build(); }steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (1)
44-49: Document null semantics for API password.
changeApiPassword(Integer userPk, String newPassword)appears to acceptnull(to clear). Specify this contract here.+ /** + * Sets the API password for the given user. + * @param userPk primary key of the user (required) + * @param newPassword encoded password; pass {@code null} to clear/unset the API password + */ void changeApiPassword(Integer userPk, String newPassword);steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
92-98: Prefer EnumSet for single-enum set.Use
EnumSet.of(WebUserAuthority.ADMIN)instead of wrappingSet.ofin aLinkedHashSet.- .authorities(new LinkedHashSet<>(Set.of(WebUserAuthority.ADMIN))) + .authorities(java.util.EnumSet.of(WebUserAuthority.ADMIN))
📜 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 (20)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ChargePointsRestController.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java(11 hunks)steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/config/JooqConfiguration.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(4 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java(18 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java(6 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java(9 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java(6 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/ChargePointsRestController.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/config/JooqConfiguration.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java
- steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java
- steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java
🧰 Additional context used
🧬 Code graph analysis (6)
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (2)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
ChargingProfile(33-87)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java (1)
NoArgsConstructor(37-73)
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
SteveException(27-117)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (2)
Getter(48-151)Getter(133-150)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (1)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
RequiredArgsConstructor(314-318)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (2)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (2)
RequiredArgsConstructor(314-318)Slf4j(69-492)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/AddressRepositoryImpl.java (1)
Slf4j(40-125)
steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/User.java (1)
User(34-82)
⏰ 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). (19)
- GitHub Check: license-check
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- 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-22.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
🔇 Additional comments (31)
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java (1)
21-21: LGTM: Added SteveException import is correct and used below.steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (1)
50-52: Optional/DTO switch looks goodUsing Optional from the repository and short-circuiting INVALID is clear and safe.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (2)
117-118: Good move to database timeReplacing LocalDateTime.now() with DSL.currentLocalDateTime() aligns timestamps to DB clock and avoids app/DB drift.
Also applies to: 126-127
198-207: Robust connectorPk lookupUsing fetchOptional(...).orElseThrow(...) prevents hidden NPEs and yields actionable errors.
steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (1)
24-25: LGTM on Optional import.Consistent with the Optional-based repository API.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java (2)
137-147: Stats mapping looks correct.Using row.get(field) avoids positional coupling and is robust to column order.
57-63: Lombok annotation processing already enabled
The parent POM’s maven-compiler-plugin includes Lombok in its<annotationProcessorPaths>, so the generated constructor will be processed—no changes needed here.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (3)
27-27: Constructor injection + final DSLContext: solid improvementImmutability, easier testing, and clearer dependencies. No further action needed.
Also applies to: 47-47, 64-64
68-68: No guard needed—migration B1_0_5__stevedb.sql inserts a default settings row
The baseline migration (B1_0_5__stevedb.sql) includesINSERT INTO `settings` (…) VALUES ('U3RlY2tkb3NlblZlcndhbHR1bmc=', …);so on a fresh install
fetchOne()will never return null. You may still add a null‐check ingetInternal()for extra safety, but it isn’t required.
27-27: Prevent jOOQ record leakage
Tests insteve/src/test/java/de/rwth/idsg/steve/utils/__DatabasePreparer__.javaimportjooq.steve.db.tables.records.OcppTagActivityRecordandTransactionRecord, pulling jOOQ types into thestevemodule. Replace these with domain DTOs or factory methods exposed by thesteve-jooqmodule.⛔ Skipped due to learnings
Learnt from: juherr PR: juherr/evolve#62 File: steve/README.md:119-121 Timestamp: 2025-08-29T07:17:42.970Z Learning: SteVe project uses jOOQ which requires live database connection during Maven build process to run Flyway migrations and generate Java code from database schema in the generate-sources phase.steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
25-25: Import addition is correct.BigDecimal is required for minChargingRate/powerLimit. LGTM.
steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java (2)
64-67: Success path looks good.Switch to details.getChargingProfilePk() aligns with the new DTO. Repository call remains unchanged. LGTM.
101-102: toOffsetDateTime is null-safe. The LocalDateTime and Instant overloads in DateTimeUtils explicitly return null for null inputs, so mapping validFrom/validTo cannot NPE.steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (2)
35-53: Mapping completeness: OK (incl. description/note).All relevant fields are mapped from the record to the core DTO. LGTM.
55-58: Period mapping is straightforward and correct.Field-to-field copy matches DTO. LGTM.
steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (4)
30-35: DTO/Optional migration looks good.Switching to
WebUserDTOs andOptional<WebUser>improves decoupling and null-safety.Also applies to: 52-55
42-42: Primitive count return is preferable.Returning
intavoids boxed nulls. LGTM.
50-50: Useful addition.
boolean userExists(String username)is a helpful pre-check for flows. LGTM.
36-39: Keep both deleteUser overloads
BothdeleteUser(String)anddeleteUser(int)are called inWebUsersService(lines 120 and 185), so neither is dead API.steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (12)
105-107: Create flow: DTO migration LGTM.
113-115: Update flow: DTO migration LGTM.
130-144: Password change context refresh LGTM.
171-178: API cache sentinel pattern LGTM.
184-189: Good: invalidate cache on delete by PK.
197-199: Count-based check LGTM.
231-232: Overview passthrough LGTM.
235-240: Details-by-PK flow LGTM (clear NotFound).
243-248: Details-by-username flow LGTM (clear NotFound).
319-326: JSON array parsing signature change LGTM.
327-338: Stable, de-duplicated authority JSON LGTM.
353-354: Minor style improvement LGTM.
| * | ||
| */ | ||
| DbVersion getDBVersion(); | ||
| Optional<DbVersion> getDBVersion(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking API: verify all call sites and implementors handle Optional.
Search for invocations that still expect DbVersion and adjust to Optional usage to avoid NPEs or compile errors.
Run:
🏁 Script executed:
#!/bin/bash
# Calls
rg -nP '\bgetDBVersion\s*\(' -C2
# Implementations
rg -nP 'class\s+\w+\s+implements\s+GenericRepository\b' -C2Length of output: 2487
Unwrap Optional in AboutSettingsController#getAbout
Change
model.addAttribute("db", genericRepository.getDBVersion());to
model.addAttribute("db", genericRepository.getDBVersion().orElse(null));🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java
around line 40 and specifically in AboutSettingsController#getAbout, the
Optional<DbVersion> returned by genericRepository.getDBVersion() is being passed
directly to the model; change the call site to unwrap the Optional by using
orElse(null) so the model receives either the DbVersion or null (e.g., replace
model.addAttribute("db", genericRepository.getDBVersion()) with
model.addAttribute("db", genericRepository.getDBVersion().orElse(null))).
steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java
Show resolved
Hide resolved
| @NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
| public final class ChargingProfileMapper { | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enforce the new module boundary with tests.
Add ArchUnit rules to ensure steve-core never depends on jooq.* types, and mapping stays in steve-jooq.
Example ArchUnit (place in tests of steve-core):
import com.tngtech.archunit.junit.AnalyzeClasses;
import com.tngtech.archunit.junit.ArchTest;
import com.tngtech.archunit.lang.syntax.ArchRuleDefinition;
@AnalyzeClasses(packages = "de.rwth.idsg.steve")
public class ModuleDependenciesTest {
@ArchTest
static final var CORE_SHOULD_NOT_DEPEND_ON_JOOQ =
ArchRuleDefinition.noClasses()
.that().resideInAPackage("..steve-core..")
.should().dependOnClassesThat().resideInAPackage("..jooq..");
}🏁 Script executed:
#!/bin/bash
# Quick guardrails: ensure core has no lingering jOOQ imports and no Details.getProfile() usage
rg -n 'jooq\.steve\.db|tables\.records' steve-core/ -S
rg -nP '\bgetProfile\s*\('
# Ensure only jooq module references record types (expect hits here)
rg -n 'jooq\.steve\.db|tables\.records' -g '!steve-jooq/**' -SLength of output: 2120
Enforce module boundary: ban jOOQ imports outside steve-jooq (incl. tests)
- Relocate or refactor
steve/src/test/java/de/rwth/idsg/steve/utils/__DatabasePreparer__.javaso it no longer importsjooq.steve.db.*ortables.records—test helpers belong in steve-jooq. - Add ArchUnit tests in steve-core (and the root steve module) to assert no classes in
..steve-core..or..steve..depend onjooq.steve.dborjooq.steve.db.tables.records. - Drop or scope any grep/regex guards for
getProfile()to only catch mapping‐related calls, since application config calls are legitimate.
| private static TransactionStopEventActor toJooq(de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) { | ||
| if (actor == null) { | ||
| return null; | ||
| } | ||
| return TransactionStopEventActor.valueOf(actor.name()); | ||
| } |
There was a problem hiding this comment.
valueOf() may throw on enum drift; add tolerant mapping
If the DTO enum gets new values or casing differs, valueOf() throws IllegalArgumentException and breaks inserts.
- private static TransactionStopEventActor toJooq(de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) {
- if (actor == null) {
- return null;
- }
- return TransactionStopEventActor.valueOf(actor.name());
- }
+ private static TransactionStopEventActor toJooq(
+ de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) {
+ if (actor == null) {
+ return null;
+ }
+ for (var b : TransactionStopEventActor.values()) {
+ if (b.name().equalsIgnoreCase(actor.name())) {
+ return b;
+ }
+ }
+ // Fallback: allow null rather than failing the write.
+ return null;
+ }📝 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 TransactionStopEventActor toJooq(de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) { | |
| if (actor == null) { | |
| return null; | |
| } | |
| return TransactionStopEventActor.valueOf(actor.name()); | |
| } | |
| private static TransactionStopEventActor toJooq( | |
| de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) { | |
| if (actor == null) { | |
| return null; | |
| } | |
| for (var b : TransactionStopEventActor.values()) { | |
| if (b.name().equalsIgnoreCase(actor.name())) { | |
| return b; | |
| } | |
| } | |
| // Fallback: allow null rather than failing the write. | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java
around lines 486-491, the use of TransactionStopEventActor.valueOf(actor.name())
is brittle and will throw IllegalArgumentException if the DTO enum gains new
values or casing differs; replace this with a tolerant mapping: perform a
case-insensitive lookup across TransactionStopEventActor.values() and return the
matching enum, or if none matches return null (or a defined fallback), and
ensure any unexpected exceptions are caught and handled so inserts don’t break.
juherr
force-pushed
the
feature/jooq-module-dep
branch
from
6836d53 to
c1ceafe
Compare
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-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (1)
36-41: Document Optional.empty() in getDBVersion Javadoc
- In steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (lines 36–41), update the method Javadoc to specify when the returned Optional is empty (e.g. no version row exists or the database isn’t initialized).
- Call-site audit (rg search) shows no direct Optional.get() usage; callers (e.g. AboutSettingsController) safely use orElse(null).
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreImpl.java (1)
146-153: Thread-safety: read access should also acquire the per-id lockArrayDeque isn’t thread-safe. getSize() reads endpointDeque without the chargeBoxId lock, racing with add/remove. Guard the read.
public int getSize(String chargeBoxId) { - Deque<SessionContext> endpointDeque = lookupTable.get(chargeBoxId); - if (endpointDeque == null) { - return 0; - } else { - return endpointDeque.size(); - } + Lock l = locks.get(chargeBoxId); + l.lock(); + try { + Deque<SessionContext> endpointDeque = lookupTable.get(chargeBoxId); + return endpointDeque == null ? 0 : endpointDeque.size(); + } finally { + l.unlock(); + } }
♻️ Duplicate comments (12)
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/AboutSettingsController.java (1)
74-74: Unwrapping Optional before adding to the model is correctPassing the underlying value (or null) fixes the previous type mismatch in views.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
59-78: Details now include description/note — resolves edit-flow data loss.This addresses the earlier concern about overwriting these fields during edits.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java (2)
68-74: Null-safe max: filter out null timestamps before Comparator.comparingCurrent Comparator will NPE if any valueTimestamp is null. Also ensures non-null stopTimestamp in path 1.
- var v = values.stream() - .filter(TransactionStopServiceHelper::isEnergyValue) - .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp)) - .orElse(null); + var v = values.stream() + .filter(TransactionStopServiceHelper::isEnergyValue) + .filter(mv -> mv.getValueTimestamp() != null) + .max(Comparator.comparing(TransactionDetails.MeterValues::getValueTimestamp)) + .orElse(null);
121-131: Avoid overflow: compare meter counters as longInteger.parseInt can overflow; use long once and compare.
- if (Integer.parseInt(nextTransactionStart.getStartValue()) - > Integer.parseInt(transaction.getStartValue())) { + long nextStart = Long.parseLong(nextTransactionStart.getStartValue()); + long currentStart = Long.parseLong(transaction.getStartValue()); + if (nextStart > currentStart) {steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
171-174: Exclude next transaction’s start instant from windowbetween(start, nextStart) is inclusive; use [start, nextStart) to avoid attributing next tx’s first value to current.
- timestampCondition = - CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.between(startTimestamp, nextTxRecord.getStartTimestamp()); + timestampCondition = CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.ge(startTimestamp) + .and(CONNECTOR_METER_VALUE.VALUE_TIMESTAMP.lt(nextTxRecord.getStartTimestamp()));#!/bin/bash # Verify no remaining inclusive upper-bounds on VALUE_TIMESTAMP around tx boundaries rg -nP 'VALUE_TIMESTAMP\.between\(' -C2steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (2)
299-307: Null record risk on fetchOne().value1(); use Optional and guard the insertThis can NPE if the transaction row is missing. Align with the safer pattern used elsewhere.
Apply:
- var connectorPk = DSL.select(TRANSACTION_START.CONNECTOR_PK) - .from(TRANSACTION_START) - .where(TRANSACTION_START.TRANSACTION_PK.equal(p.getTransactionId())) - .fetchOne() - .value1(); - - insertConnectorStatus(ctx, connectorPk, toLocalDateTime(p.getStopTimestamp()), p.getStatusUpdate()); + var connectorPkOpt = DSL.select(TRANSACTION_START.CONNECTOR_PK) + .from(TRANSACTION_START) + .where(TRANSACTION_START.TRANSACTION_PK.equal(p.getTransactionId())) + .fetchOptional(TRANSACTION_START.CONNECTOR_PK); + + connectorPkOpt.ifPresent(pk -> + insertConnectorStatus(ctx, pk, toLocalDateTime(p.getStopTimestamp()), p.getStatusUpdate()));
487-492: valueOf() is brittle; mirror tolerant enum mapping used aboveIllegalArgumentException on enum drift will bounce the insert into the “failed” path unnecessarily.
Apply:
- private static TransactionStopEventActor toJooq(de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) { - if (actor == null) { - return null; - } - return TransactionStopEventActor.valueOf(actor.name()); - } + private static TransactionStopEventActor toJooq( + de.rwth.idsg.steve.repository.dto.TransactionStopEventActor actor) { + if (actor == null) { + return null; + } + for (var b : TransactionStopEventActor.values()) { + if (b.name().equalsIgnoreCase(actor.name())) { + return b; + } + } + // Fallback: return null rather than throwing to avoid breaking the write path. + return null; + }steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (1)
22-22: Fix layering breach: repository must not import service DTOs.Import
WebUserOverviewfrom a repository/core-scoped DTO package, notservice.dto.-import de.rwth.idsg.steve.service.dto.WebUserOverview; +import de.rwth.idsg.steve.repository.dto.WebUserOverview;steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (1)
68-71: Null-safe authorities serialization (NPE risk today).
user.getAuthorities()may be null;.stream()would NPE in create/update/updateByPk.- toJSON(user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet()))) + toJSON((user.getAuthorities() == null) + ? java.util.Set.of() + : user.getAuthorities().stream().map(Enum::name).collect(Collectors.toSet())))Apply the same change in all three places shown in the selected ranges.
Also applies to: 79-83, 92-95
steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (3)
284-296: Robust authority parsing from GrantedAuthority strings.Avoid
valueOfbrittleness (e.g., “ROLE_ADMIN”). Prefer a normalizing factory.Option A (if
WebUserAuthority.fromAuthority(String)exists and returns the enum):- var authorities = user.getAuthorities().stream() - .map(GrantedAuthority::getAuthority) - .map(String::toUpperCase) - .map(WebUserAuthority::valueOf) - .collect(Collectors.toSet()); + var authorities = user.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .map(WebUserAuthority::fromAuthority) + .collect(Collectors.toSet());Option B (if it returns Optional):
+ var authorities = user.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .flatMap(WebUserAuthority::fromAuthority) + .collect(Collectors.toSet());Verification script to check available factories:
#!/bin/bash rg -nP 'enum\s+WebUserAuthority|fromAuthority\(' -g '!**/target/**' -C2
275-281: Align API authorities mapping with UI (avoid privilege drift).Flatten via
getValues()like the UI path.- return User.withUsername(webUser.getLogin()) + return User.withUsername(webUser.getLogin()) .password(apiPassword) - .disabled(!webUser.isEnabled()) - .authorities(webUser.getAuthorities().stream() - .map(WebUserAuthority::name) - .toArray(String[]::new)) + .disabled(!webUser.isEnabled()) + .authorities(webUser.getAuthorities().stream() + .map(WebUserAuthority::getValues) + .flatMap(java.util.Set::stream) + .toArray(String[]::new)) .build();
209-217: Null-safe authorities + cache invalidation on username rename.
Set.of(form.getAuthorities())throws on null; useEnumSet.- Invalidate old username if changed to prevent stale cache hits.
- var webUser = WebUser.builder() + var existing = webUserRepository.loadUserByUserPk(form.getWebUserPk()).orElse(null); + var auths = (form.getAuthorities() == null) + ? EnumSet.noneOf(WebUserAuthority.class) + : EnumSet.copyOf(java.util.Arrays.asList(form.getAuthorities())); + var webUser = WebUser.builder() .webUserPk(form.getWebUserPk()) .login(form.getWebUsername()) .enabled(form.getEnabled()) - .authorities(Set.of(form.getAuthorities())) + .authorities(auths) .build(); webUserRepository.updateUserByPk(webUser); userCache.invalidate(form.getWebUsername()); + if (existing != null && !existing.getLogin().equals(form.getWebUsername())) { + userCache.invalidate(existing.getLogin()); + }
🧹 Nitpick comments (29)
steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (1)
36-41: Naming nit: prefer getDbVersion for acronym casingMinor: method name could follow standard camel-case for acronyms (Db vs DB). Consider a follow-up rename if you plan further breaking changes.
steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/AboutSettingsController.java (1)
71-79: Optional: avoid adding a null-valued attributeTo keep the model cleaner and avoid subtle JSP differences between “present but null” vs “absent”, only add the attribute when present.
Apply:
- model.addAttribute("db", genericRepository.getDBVersion().orElse(null)); + genericRepository.getDBVersion().ifPresent(db -> model.addAttribute("db", db));Confirm the JSP does not rely on the attribute’s existence when absent (e.g.,
${db != null}logic). If it does, keep the current version.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (2)
139-143: Guard against empty idTagList to avoid generating a pointless query.
Minor optimization and makes behavior explicit.Apply:
public List<OcppTagActivity> getRecords(List<String> idTagList) { + if (idTagList == null || idTagList.isEmpty()) { + return List.of(); + } return ctx.selectFrom(OCPP_TAG_ACTIVITY) .where(OCPP_TAG_ACTIVITY.ID_TAG.in(idTagList)) .fetch(OcppTagMapper::fromRecord); }
185-188: Active tag filter: DB clock switch mirrors overview — good; watch OR predicate impact.
TheisNull().or(> now)predicate can limit index usage on EXPIRY_DATE. If this query is hot, consider a functional index or separating “no expiry” vs “future expiry” into UNION branches by dialect.steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (2)
50-75: Optional-based flow and domain methods (isBlocked/isExpired/hasReachedLimitOfActiveTransactions) look solid.
Control flow is unchanged functionally; code is clearer.Consider injecting a
Clock(or passingInstant nowfrom the caller) to make time-based decisions deterministic in tests and consistent with repository-side DB time usage.
87-99: Expiry resolution logic preserved; align time sources if possible.
You now useOffsetDateTime.now()here vs DB-side time in repository queries. If app and DB can drift, centralize time via aClockor passInstant nowthrough.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (3)
55-63: Harden parsing against null/invalid DB values.If
MAIL_RECIPIENTSorNOTIFICATION_FEATUREScontains null/empty/unknown tokens,fromName(...)may throw or yield nulls. Filter blanks and ignore unknowns.Apply:
@@ - private static List<String> parseRecipients(SettingsRecord r) { - return splitByComma(r.getMailRecipients()); - } + private static List<String> parseRecipients(SettingsRecord r) { + var raw = r.getMailRecipients(); + return raw == null ? List.of() : splitByComma(raw).stream() + .filter(s -> s != null && !s.isBlank()) + .toList(); + } @@ - private static List<NotificationFeature> parseEnabledFeatures(SettingsRecord r) { - return splitByComma(r.getNotificationFeatures()).stream() - .map(NotificationFeature::fromName) - .toList(); - } + private static List<NotificationFeature> parseEnabledFeatures(SettingsRecord r) { + var raw = r.getNotificationFeatures(); + return raw == null ? List.of() : splitByComma(raw).stream() + .filter(s -> s != null && !s.isBlank()) + .map(s -> { + try { return NotificationFeature.fromName(s); } + catch (IllegalArgumentException __) { return null; } + }) + .filter(java.util.Objects::nonNull) + .toList(); + }
69-86: Null-row safety for getInternal().If the settings row is missing,
rbecomes null and all getters NPE. Consider failing fast with a clear exception or auto-seeding.- var r = getInternal(); + var r = getInternal(); + if (r == null) { + throw new SteveException.InternalError("Settings row not found for APP_ID"); + }Apply similarly in
getMailSettings().
91-107: Nit: naming consistency.Prefer
emailsovereMailsfor consistency withgetForm().- var eMails = parseRecipients(r); + var emails = parseRecipients(r); @@ - .recipients(eMails) + .recipients(emails)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (3)
51-51: Prefer Stream.toList() and drop Collectors import for consistency.-import java.util.stream.Collectors; @@ - .periods(periods.stream().map(ChargingProfileMapper::fromRecord).collect(Collectors.toList())) + .periods(periods.stream().map(ChargingProfileMapper::fromRecord).toList())Remove the unused
Collectorsimport.Also applies to: 27-29
35-37: Make periods null-safe.Guard against a null
periodslist from callers.- public static ChargingProfile.Details fromRecord( - ChargingProfileRecord profile, List<ChargingSchedulePeriodRecord> periods) { + public static ChargingProfile.Details fromRecord( + ChargingProfileRecord profile, List<ChargingSchedulePeriodRecord> periods) { + var safePeriods = periods == null ? java.util.List.<ChargingSchedulePeriodRecord>of() : periods; return ChargingProfile.Details.builder() @@ - .periods(periods.stream().map(ChargingProfileMapper::fromRecord).toList()) + .periods(safePeriods.stream().map(ChargingProfileMapper::fromRecord).toList()) .build(); }Also applies to: 51-53
32-34: Utility class style (optional).You can replace
final class + private ctorwith Lombok’s@UtilityClass.-@NoArgsConstructor(access = AccessLevel.PRIVATE) -public final class ChargingProfileMapper { +@lombok.UtilityClass +public class ChargingProfileMapper {steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (5)
72-76: Confirmvarusage against project JDK; optional explicit type for clarity
- Please confirm the baseline is JDK ≥ 10 (project likely 17/21) so
varis fine.- Optional: if your style prefers explicit types for jOOQ fluency, replace
varwith the concreteSelectConditionStep<Record1<Integer>>.
85-86: Same note onvarin subselectEchoing the above: fine if JDK ≥ 10; otherwise consider explicit typing.
255-266: Minor robustness + potential single-roundtrip refactor
- Prefer using the PK from the fetched record inside the mapper lambda instead of the outer param (safer if the method ever evolves).
Apply within this hunk:
.fetchOptional() .map(profile -> { - var periods = ctx.selectFrom(CHARGING_SCHEDULE_PERIOD) - .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(chargingProfilePk)) + var pk = profile.getChargingProfilePk(); + var periods = ctx.selectFrom(CHARGING_SCHEDULE_PERIOD) + .where(CHARGING_SCHEDULE_PERIOD.CHARGING_PROFILE_PK.eq(pk)) .orderBy(CHARGING_SCHEDULE_PERIOD.START_PERIOD_IN_SECONDS.asc()) .fetch(); return ChargingProfileMapper.fromRecord(profile, periods); });
- Optional: collapse to one DB roundtrip using jOOQ MULTISET for periods if you later batch-fetch details.
355-355:String.formatted(...)requires JDK ≥ 15If your toolchain might be <15, switch to
String.format(...):- "Failed to update the charging profile with id '%s'".formatted(form.getChargingProfilePk()), e); + String.format("Failed to update the charging profile with id '%s'", form.getChargingProfilePk()), e);
378-380: Same JDK baseline note; consider de-duplicating station IDs in message
- Ensure JDK ≥ 15 for
.formatted(...); otherwise useString.format(...)as above.- Optional: the list
rmay contain duplicate chargeBoxIds (multiple connectors). Consider distinct for a cleaner message. Example (outside this hunk):var stations = ctx.selectDistinct(CONNECTOR.CHARGE_BOX_ID) .from(CONNECTOR_CHARGING_PROFILE) .join(CONNECTOR).on(CONNECTOR.CONNECTOR_PK.eq(CONNECTOR_CHARGING_PROFILE.CONNECTOR_PK)) .where(CONNECTOR_CHARGING_PROFILE.CHARGING_PROFILE_PK.eq(chargingProfilePk)) .fetch(CONNECTOR.CHARGE_BOX_ID);steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java (2)
131-136: Consider aligning timestamp with value sourceMixing transaction.startValue with nextTx.startTimestamp may be confusing; either keep both from nextTx or document the rationale.
25-25: OCPP UnitOfMeasure package divergence across modulesCore imports ocpp.cs._2012._06.UnitOfMeasure while jOOQ imports ocpp.cs._2015._10.UnitOfMeasure. Consider a single abstraction (local enum/constants) to avoid cross-version drift.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (3)
142-143: Minor: avoid shadowing fieldctxwith localvar ctxinside transactionsNot a bug, but it hurts readability. Consider
txortxCtx.Also applies to: 179-180, 198-199
406-408: Optional: prefer event/DB time over LocalDateTime.now() in audit-like noteFor consistency with other timestamps and reproducibility, consider using p.getEventTimestamp() (or DB time) when formatting the note.
- var note = "This unknown idTag was used in a transaction that started @ " + p.getStartTimestamp() - + ". It was reported @ " + LocalDateTime.now() + "."; + var note = "This unknown idTag was used in a transaction that started @ " + p.getStartTimestamp() + + ". It was reported @ " + p.getEventTimestamp() + ".";
440-456: Optional: guard against very large batchesFor huge meter value lists, collecting all records may spike memory. Consider chunking (e.g., in batches of 1–5k) before batchInsert.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreImpl.java (1)
166-168: “Copy” is shallow; consider deep copy or rename for clarityImmutableMap.copyOf retains mutable Deque values. Either deep-copy the Deques or rename to getAShardedView()/getSnapshotShallow() to avoid misleading callers.
steve-api/src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java (1)
456-458: Domain wording nitConsider “OCPP tag” instead of “user” for consistency with the endpoint.
- "A user with idTag '%s' already exists.".formatted(form.getIdTag()))); + "An OCPP tag with idTag '%s' already exists.".formatted(form.getIdTag())));steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java (1)
85-85: Consider truncating/sanitizing incoming payload in exception messagesEmbedding full incoming strings can leak PII and bloat logs. Recommend capping length (e.g., 2–4KB) and/or redacting sensitive fields before inclusion.
Also applies to: 149-151, 178-180
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
41-50: Optional: add static helpers to centralize common message patternsFactory methods (e.g., NotFound.forId("Reservation", id)) would standardize wording and reduce duplication across layers.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (2)
95-95: Validate PK on update-by-PK.Guard against null PK to avoid a silent no-op (or unintended “IS NULL” filter).
- .where(WEB_USER.WEB_USER_PK.eq(user.getWebUserPk())) + .where(WEB_USER.WEB_USER_PK.eq( + java.util.Objects.requireNonNull(user.getWebUserPk(), "webUserPk must not be null")))
186-188: Normalize role filters and fix typo.
- Normalize case to match stored enum names.
- Typo: “separated”.
- var split = form.getRoles().split(","); // Comma seperated String to StringArray - var roles = Arrays.stream(split).map(String::strip).toList(); + var split = form.getRoles().split(","); // Comma separated String to String array + var roles = Arrays.stream(split).map(String::strip).map(String::toUpperCase).toList();steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (1)
321-340: Remove unused methodsfromJsonandtoJson
These private helpers in WebUsersService have no references and can be safely removed.
📜 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 (28)
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ChargePointsRestController.java(2 hunks)steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java(1 hunks)steve-api/src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java(1 hunks)steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java(3 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java(2 hunks)steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java(11 hunks)steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/config/JooqConfiguration.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java(1 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java(9 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java(6 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java(18 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java(8 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java(5 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java(2 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java(9 hunks)steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java(6 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/pipeline/Deserializer.java(3 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/AboutSettingsController.java(1 hunks)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/config/JooqConfiguration.java
- steve-ui-jsp/src/main/java/de/rwth/idsg/steve/web/controller/ReservationsController.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/GenericRepositoryImpl.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargePointRepositoryImpl.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java
- steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java
- steve-core/src/main/java/de/rwth/idsg/steve/ocpp/task/SetChargingProfileTaskFromDB.java
- steve-api/src/main/java/de/rwth/idsg/steve/web/api/ChargePointsRestController.java
- steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java
🧰 Additional context used
🧬 Code graph analysis (5)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/web/dto/ChargingProfileForm.java (2)
Getter(48-151)Getter(133-150)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java (1)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/UpdateTransactionParams.java (1)
Getter(30-45)
steve-jooq/src/main/java/de/rwth/idsg/steve/jooq/mapper/ChargingProfileMapper.java (2)
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
ChargingProfile(33-87)steve-ui-jsp/src/main/java/de/rwth/idsg/steve/utils/mapper/ChargingProfileDetailsMapper.java (1)
NoArgsConstructor(37-73)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (2)
steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (1)
Slf4j(36-100)steve-core/src/main/java/de/rwth/idsg/steve/service/OcppTagsService.java (1)
Slf4j(45-160)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (2)
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (1)
RequiredArgsConstructor(314-318)steve/src/test/java/de/rwth/idsg/steve/issues/Issue1219.java (1)
RequiredArgsConstructor(57-214)
⏰ 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-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.5.21)
- 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, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- 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.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
🔇 Additional comments (35)
steve-core/src/main/java/de/rwth/idsg/steve/repository/GenericRepository.java (1)
24-25: Import aligns with API changeImporting Optional is correct and scoped to the interface; no further changes needed here.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java (9)
22-26: DTO + mapper adoption decouples core from jOOQ types — nice.
Public API now returns OcppTagActivity with mapping centralized in OcppTagMapper. This aligns with the PR goal to reverse the dependency.Also applies to: 37-45
75-75: Local var alias for parent table improves readability.
Usingvar parentTable = OCPP_TAG_ACTIVITY.as("parent");is clear and keeps the join tidy.
117-124: Use DB clock (DSL.currentLocalDateTime()) for expiry filtering — good; confirm time-base consistency.
Server-side time avoids app/DB skew in the query. Ensure app-level checks (e.g., AuthTagServiceLocal usingInstant.now()) won’t diverge from DB semantics if servers run different timezones.Would you like a small integration test that asserts parity between DB-driven and app-driven expiry checks around boundary values?
134-136: Repository now returns domain objects directly.
fetch(OcppTagMapper::fromRecord)is clean and keeps persistence concerns localized.
146-150: Optional-returning single-record fetch looks correct.
UsingfetchOptional(OcppTagMapper::fromRecord)is the right shape for the new API.
153-157: Same here — Optional by PK is consistent with the new API.
210-212: Stream.toList() is fine; confirm baseline JDK ≥ 16 (project likely on 17).If the baseline is JDK 11 in any downstream consumer, replace with
collect(Collectors.toList()).
231-236: Modernized messages viaString#formatted— OK; ensure baseline JDK ≥ 15.If supporting older runtimes, stick to
String.format(...).
134-157: No jOOQ record types leak into core
No imports oforg.jooq.*,jooq.steve.db.*, orOcppTagActivityRecordwere found insteve-core.steve-core/src/main/java/de/rwth/idsg/steve/service/AuthTagServiceLocal.java (2)
21-24: Swapping to OcppTagActivity DTO keeps service layer jOOQ-free.
Matches the dependency inversion goal.
77-82: HelperbuildIdTagInforeduces duplication — good extraction.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java (2)
46-47: Good move to constructor injection with Lombok.Immutable
DSLContext+@RequiredArgsConstructorimproves testability and avoids field injection pitfalls.
69-69: No action needed: Java baseline ≥ 16 confirmed. The project’sjava.versionproperty is set to 21 (and the Maven Compiler Plugin uses<release>${java.version}</release>), soStream.toList()is fully supported.steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/ChargingProfile.java (1)
80-86: DTO for schedule periods looks good.Minimal, immutable, and aligned with form mapping.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/ChargingProfileRepositoryImpl.java (1)
22-22: Mapper import looks goodBrings the repository closer to DTO mapping responsibilities without leaking jOOQ types.
steve-core/src/main/java/de/rwth/idsg/steve/repository/dto/TransactionDetails.java (1)
80-91: kWh→Wh conversion is now correctUnit is updated alongside value. Good.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java (1)
72-78: Mapper usage looks goodSwitch to TransactionMapper::fromRecord for fetch/fetchOptional is clean.
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java (8)
117-118: Good switch to DB-side timestamps; confirm DB timezone/source of truthUsing DSL.currentLocalDateTime() avoids app/DB clock skew. Ensure the DB timezone is what you expect in prod/backups/replicas.
Would you confirm the DB session/server timezone and that it matches your reporting/UX expectations? If needed, I can draft a short checklist.
Also applies to: 126-127
182-183: Nice: centralized, null-safe connectorPk retrievalgetConnectorPkFromConnector(...).fetchOptional(...).orElseThrow(...) eliminates silent nulls/NPEs and gives a clear error.
Also applies to: 434-437
201-207: Nice defensive fetchOptional on transaction->connectorPkThis mirrors the safer pattern; good guardrail.
218-237: Start flow refactor reads cleanerUsing var and returning a holder with existsAlready/transactionId clarifies control flow.
255-267: Passing numeric connectorPk simplifies API and avoids subselectsAlso keeps reservation/status paths consistent.
284-285: Map via helper is fine; ensure helper is tolerantGiven external DTOs can drift, make sure the helper won’t throw on unknown values (see comment below for toJooq()).
476-485: Tolerant mapping for TransactionStopFailedEventActor is solidCase-insensitive match with a safe fallback avoids breaking writes.
372-381: insertConnectorStatus call sites updatedAll internal uses now invoke the new
insertConnectorStatus(ctx, connectorPk, timestamp, statusUpdate)signature, and the public API still acceptsInsertConnectorStatusParams. No further changes required.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/SessionContextStoreImpl.java (2)
139-139: LGTM: preformatted InternalError messageUse of String.formatted() reads well and aligns with the exception API change.
139-139: Confirm language levelString.formatted requires Java 15+. Verify module/source/target compatibility.
steve-api/src/main/java/de/rwth/idsg/steve/web/api/ReservationsRestController.java (1)
74-74: LGTM: switched to preformatted NotFound messageMatches the updated SteveException API; behavior unchanged.
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java (1)
85-85: LGTM: consistent use of String.formatted for context-rich messagesKeeps semantics and causes intact while aligning with new exception constructors.
Also applies to: 149-151, 178-180
steve-core/src/main/java/de/rwth/idsg/steve/SteveException.java (1)
54-60: LGTM: message-first constructors with optional causeConstructor set is clear and aligns call sites across modules.
Also applies to: 69-71, 79-81
steve-core/src/main/java/de/rwth/idsg/steve/repository/WebUserRepository.java (1)
30-35: Approve code changes. Core/service layers import only DTOs (WebUserOverview) and contain no jOOQ Record leaks.steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java (1)
118-124: Count retrieval LGTM with null-safe fallback.steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java (2)
160-167: UI login authorities mapping LGTM (flattened). Keep API in sync.
93-99: Bootstrap admin creation LGTM.
| .thenThrow(new SteveException.AlreadyExists( | ||
| "A user with idTag '%s' already exists.".formatted(ocppTagPk))); | ||
|
|
There was a problem hiding this comment.
Wrong placeholder content: using ocppTagPk where idTag is expected
Message says idTag but formats with ocppTagPk (123). Use the actual idTag to keep user-facing text correct.
- .thenThrow(new SteveException.AlreadyExists(
- "A user with idTag '%s' already exists.".formatted(ocppTagPk)));
+ .thenThrow(new SteveException.AlreadyExists(
+ "A user with idTag '%s' already exists.".formatted(form.getIdTag())));📝 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.
| .thenThrow(new SteveException.AlreadyExists( | |
| "A user with idTag '%s' already exists.".formatted(ocppTagPk))); | |
| .thenThrow(new SteveException.AlreadyExists( | |
| "A user with idTag '%s' already exists.".formatted(form.getIdTag()))); |
🤖 Prompt for AI Agents
In
steve-api/src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java
around lines 456 to 458, the thrown SteveException.AlreadyExists message says "A
user with idTag '%s' already exists." but is formatted with ocppTagPk; update
the formatted argument to use the actual idTag variable (the idTag string used
in the test) instead of ocppTagPk so the user-facing text reflects the idTag
value.
| import de.rwth.idsg.steve.SteveException; | ||
| import de.rwth.idsg.steve.repository.WebUserRepository; | ||
| import de.rwth.idsg.steve.repository.dto.WebUser; | ||
| import de.rwth.idsg.steve.service.dto.WebUserOverview; |
There was a problem hiding this comment.
Fix layering: service should use repository DTO for overview.
Use de.rwth.idsg.steve.repository.dto.WebUserOverview.
-import de.rwth.idsg.steve.service.dto.WebUserOverview;
+import de.rwth.idsg.steve.repository.dto.WebUserOverview;📝 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.
| import de.rwth.idsg.steve.service.dto.WebUserOverview; | |
| import de.rwth.idsg.steve.repository.dto.WebUserOverview; |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/service/WebUsersService.java
around line 29, replace the service-layer DTO import and usages with the
repository-layer DTO: change the import from
de.rwth.idsg.steve.service.dto.WebUserOverview to
de.rwth.idsg.steve.repository.dto.WebUserOverview, update any method signatures,
return types, local variable types and mapper/stream operations in this class to
use the repository DTO, and remove or replace the old service DTO import; also
adjust related imports and update any calling code/tests to the new DTO type if
necessary.
| var eMails = joinByComma(form.getRecipients()); | ||
| var features = joinByComma(form.getEnabledFeatures()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Persist canonical feature names to ensure round-trip with fromName().
Avoid relying on toString(). Store name() (or a dedicated getter) to match fromName(...).
- var eMails = joinByComma(form.getRecipients());
- var features = joinByComma(form.getEnabledFeatures());
+ var eMails = joinByComma(form.getRecipients());
+ var features = joinByComma(
+ form.getEnabledFeatures().stream()
+ .map(f -> (f instanceof Enum) ? ((Enum<?>) f).name() : f.toString())
+ .toList()
+ );📝 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.
| var eMails = joinByComma(form.getRecipients()); | |
| var features = joinByComma(form.getEnabledFeatures()); | |
| var eMails = joinByComma(form.getRecipients()); | |
| var features = joinByComma( | |
| form.getEnabledFeatures().stream() | |
| .map(f -> (f instanceof Enum) ? ((Enum<?>) f).name() : f.toString()) | |
| .toList() | |
| ); |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/SettingsRepositoryImpl.java
around lines 121 to 123, the code currently serializes enabled feature enums via
toString() when building the comma list; instead map each feature to its
canonical identifier (feature.name() or the dedicated getter used by
fromName(...)) before calling joinByComma so the persisted string round-trips
with fromName(...). Replace the direct join of form.getEnabledFeatures() with a
stream/map to the canonical name and then join.
| .location(r.value6()) | ||
| .unit(r.value7()) | ||
| .phase(r.value8()) | ||
| .valueTimestamp(toInstant(r.get(ts))) |
There was a problem hiding this comment.
Null-safe timestamp mapping
If VALUE_TIMESTAMP can be null, toInstant(null) will NPE. Map null through unchanged; DTO allows @nullable.
- .valueTimestamp(toInstant(r.get(ts)))
+ .valueTimestamp(r.get(ts) == null ? null : toInstant(r.get(ts)))📝 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.
| .valueTimestamp(toInstant(r.get(ts))) | |
| .valueTimestamp(r.get(ts) == null ? null : toInstant(r.get(ts))) |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/TransactionRepositoryImpl.java
around line 231, the call .valueTimestamp(toInstant(r.get(ts))) will throw NPE
if the VALUE_TIMESTAMP column is null; change it to map null through unchanged
by checking r.get(ts) for null and passing null to valueTimestamp when null,
otherwise call toInstant on the non-null value, so the DTO receives a nullable
timestamp instead of causing an exception.
| import de.rwth.idsg.steve.jooq.mapper.WebUserMapper; | ||
| import de.rwth.idsg.steve.repository.WebUserRepository; | ||
| import de.rwth.idsg.steve.repository.dto.WebUser; | ||
| import de.rwth.idsg.steve.service.dto.WebUserOverview; |
There was a problem hiding this comment.
Break circular dependency: use repository DTO for overview.
steve-jooq must not depend on service. Swap to a repository/core DTO.
-import de.rwth.idsg.steve.service.dto.WebUserOverview;
+import de.rwth.idsg.steve.repository.dto.WebUserOverview;📝 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.
| import de.rwth.idsg.steve.service.dto.WebUserOverview; | |
| import de.rwth.idsg.steve.repository.dto.WebUserOverview; |
🤖 Prompt for AI Agents
In
steve-jooq/src/main/java/de/rwth/idsg/steve/repository/impl/WebUserRepositoryImpl.java
around line 26, replace the import of the service-layer DTO
(de.rwth.idsg.steve.service.dto.WebUserOverview) with the repository/core-layer
DTO (e.g. de.rwth.idsg.steve.repository.dto.WebUserOverview or
de.rwth.idsg.steve.core.dto.WebUserOverview) to break the circular dependency;
update any code in this class that references the service DTO to use the
repository/core DTO fields and constructors (or add an adapter/mapper in the
repository layer if shapes differ), remove the service module dependency from
this module's build config, and run compilation/tests to ensure the new DTO is
used consistently.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests