Skip to content

.pr_agent_auto_best_practices

Jump to bottom Edit New page
qodo-merge-bot edited this page Sep 18, 2026 · 10 revisions

Pattern 1: Validate request-derived identifiers and collections for null, blank, empty, or null-element values before using them in SQL predicates. Fail fast with a clear validation exception rather than relying on query-builder or database behavior.

Example code before:

public void update(String externalId) {
    db.update(TABLE)
        .set(TABLE.STATE, "DONE")
        .where(TABLE.EXTERNAL_ID.eq(externalId))
        .execute();
}

Example code after:

public void update(String externalId) {
    if (externalId == null || externalId.isBlank()) {
        throw new IllegalArgumentException("externalId must not be blank");
    }

    db.update(TABLE)
        .set(TABLE.STATE, "DONE")
        .where(TABLE.EXTERNAL_ID.eq(externalId))
        .execute();
}
Relevant past accepted suggestions:
Suggestion 1: [correctness] `idTags` Set not validated
`idTags` Set not validated `getParentIdTags(Set idTags)` uses `idTags` directly in a SQL predicate without validating non-null (and without guarding empty/null elements), which can lead to runtime failures instead of clear validation behavior. This violates the requirement to validate externally supplied/configured values before using them in operations/SQL predicates.

Issue description

getParentIdTags(Set<String> idTags) uses OCPP_TAG.ID_TAG.in(idTags) without validating that idTags is non-null (and ideally without null elements). This can produce a runtime exception rather than a clear validation outcome.

Issue Context

This method is now part of the repository interface and may be called from multiple code paths; defensive validation prevents unexpected NPEs and ambiguous behavior.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/repository/impl/OcppTagRepositoryImpl.java[226-233]
  • src/main/java/de/rwth/idsg/steve/repository/OcppTagRepository.java[49-51]
Suggestion 2: [reliability] `insertIgnoreConnector` lacks input validation
`insertIgnoreConnector` lacks input validation `insertIgnoreConnector()` uses externally supplied `chargeBoxIdentity` directly in SQL predicates without validating null/blank, which can lead to ambiguous queries or runtime failures instead of a clear validation error.

Issue description

insertIgnoreConnector() accepts chargeBoxIdentity (request/station-derived) and uses it to build SQL predicates without validating null/blank. This violates the input validation requirement and can cause unclear behavior or runtime errors.

Issue Context

The compliance requirement expects fail-fast validation (e.g., IllegalArgument/BadRequest-style) before dereferencing/using inputs in SQL predicate construction.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/repository/impl/OcppServerRepositoryImpl.java[434-455]
Suggestion 3: [reliability] `idTagFromTransaction` not validated
`idTagFromTransaction` not validated `idTagFromTransaction` (derived from external OCPP input) is used to build SQL predicates without null/blank validation, which can yield ambiguous query behavior (e.g., matching `NULL` or silently updating nothing). This violates the requirement to validate externally supplied inputs (including when used in SQL predicate construction) before use and fail fast with a clear error.

Issue description

idTagFromTransaction is used to construct SQL conditions without null/blank validation, which can cause ambiguous behavior (e.g., IS NULL-style predicates or silent no-op updates) instead of failing fast with a clear validation error.

Issue Context

idTagFromTransaction originates from transaction input (p.getIdTag()), i.e., external/request-derived data.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/repository/impl/ReservationRepositoryImpl.java[174-199]

Pattern 2: Use bounded waits in asynchronous and concurrent test helpers, and explicitly fail when a latch, poll, future, or executor does not complete before its deadline. Replace fixed sleeps and unchecked timeout results with condition-based waiting and diagnostic timeout messages.

Example code before:

start.await();
Thread.sleep(1000);
done.await(5, TimeUnit.SECONDS);
executor.shutdownNow();

Example code after:

if (!start.await(5, TimeUnit.SECONDS)) {
    throw new AssertionError("Workers did not receive the start signal");
}

await().atMost(Duration.ofSeconds(10))
       .until(this::expectedStateExists);

executor.shutdownNow();
if (!executor.awaitTermination(5, TimeUnit.SECONDS)) {
    throw new AssertionError("Executor did not terminate");
}
Relevant past accepted suggestions:
Suggestion 1: [reliability] `start.await()` lacks timeout
`start.await()` lacks timeout The new concurrent test helper blocks worker threads on `start.await()` with no timeout, which can cause tests to hang indefinitely if the start signal is never delivered due to an earlier failure. This violates the requirement for bounded waits with checked timeout outcomes in concurrent test helpers.

Issue description

The concurrent test helper runConcurrently uses an unbounded start.await() in worker threads. This can hang the test suite indefinitely if start.countDown() is not reached due to an earlier failure.

Issue Context

PR Compliance ID 2 requires bounded waits with explicit timeout checks for asynchronous/concurrent tests.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/repository/impl/Ocpp1ConnectorEvseBridgeIT.java[131-135]
Suggestion 2: [reliability] Executor termination not verified
Executor termination not verified The new concurrency test helper calls `shutdownNow()` but never waits for executor termination, so worker threads may still be running if a future times out or an operation blocks in JDBC. This makes test cleanup nondeterministic and can cause flakiness/interference in CI failures.

Issue description

runConcurrently shuts down the thread pool with shutdownNow() but does not awaitTermination(). On timeouts/failures, tasks may still be executing or blocked, and the helper returns without confirming cleanup.

Issue Context

This is a test-only helper, but nondeterministic cleanup can make failing CI runs noisier and can interfere with subsequent tests.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/repository/impl/Ocpp1ConnectorEvseBridgeIT.java[123-149]

What to change

After shutdownNow(), call awaitTermination with a bounded timeout (e.g., 5–10s). If termination fails, either:

  • call shutdownNow() again and fail the test with a clear message, or
  • log diagnostic info and proceed (less strict). Also consider executor.shutdown() first, then shutdownNow() if it doesn’t terminate in time.
Suggestion 3: [reliability] Fixed sleep causes flakes
Fixed sleep causes flakes The TLS certification IT uses a fixed 1-second sleep before asserting DB state, which can intermittently fail under slow CI/DB load because persistence timing is nondeterministic.

Issue description

A fixed Thread.sleep(1s) is used to wait for DB updates, which can be too short on CI and cause flaky failures.

Issue Context

The certificate signing + persistence is asynchronous relative to the test flow.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/certification/ocpp16/Ocpp16JsonCsmsCertification_TLS_IT.java[161-189]

Suggested fix

  • Replace Thread.sleep with polling + timeout (e.g., Awaitility or a simple loop with Thread.sleep(100ms) and a max duration) until the expected DB row/flag appears.
  • Assert with a clear timeout failure message showing current DB state.
Suggestion 4: [reliability] Latch timeouts ignored
Latch timeouts ignored sendInternal() and close() call CountDownLatch.await(timeout) but ignore the boolean return, so timeouts can silently turn into null incoming messages, misleading assertion failures, or leaked websocket/client resources.

Issue description

CountDownLatch.await(timeout, unit) returns false on timeout, but the code ignores this and continues. This can mask real websocket/protocol failures and produce confusing downstream errors (e.g., null incoming message).

Issue Context

This utility is used by multiple tests (and the stress tester). When communication fails or a callback is disrupted, you want an immediate, descriptive failure rather than a silent timeout that turns into a generic assertion.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/utils/OcppJsonChargePoint.java[142-152]
  • src/test/java/de/rwth/idsg/steve/utils/OcppJsonChargePoint.java[222-239]

Suggested fix

  • Capture the boolean return of await(...) and throw an AssertionFailedError (or RuntimeException) when it is false, including context (action/messageId/expected type).
  • For close(), if the await times out, attempt client.stop()/client.close() and throw, so tests don’t leak threads/sessions.
  • Optionally also add a timeout to connect.get(...) in start() for symmetry.

Pattern 3: Guard version-specific registry lookups before invoking or dereferencing the selected handler or type store. Convert unsupported or missing protocol-version mappings into a controlled protocol error instead of allowing a NullPointerException.

Example code before:

Handler handler = handlers.get(context.protocolVersion());
handler.accept(context);

Example code after:

Handler handler = handlers.get(context.protocolVersion());
if (handler == null) {
    context.setError(protocolError("Unsupported protocol version"));
    return;
}
handler.accept(context);
Relevant past accepted suggestions:
Suggestion 1: [reliability] `processCall()` may NPE handler
`processCall()` may NPE handler `IncomingPipeline.processCall()` retrieves a version-specific `OcppCallHandler` via `handlerMap.get(...)` and immediately calls `handler.accept(context)` without validating the lookup result. If the map lacks the version key (e.g., misconfiguration, unsupported/future protocol, or unexpected context), this can throw a NullPointerException and drop the CALL instead of producing a controlled validation error / proper OCPP CALL_ERROR response.

Issue description

IncomingPipeline.processCall(..) looks up a version-specific OcppCallHandler from handlerMap (keyed by context.getProtocol().getVersion()) and immediately calls handler.accept(context) without a null check; if the version key is missing, this can throw a runtime NPE and drop the CALL instead of failing explicitly and sending a proper OCPP CALL_ERROR.

Issue Context

  • Handlers are injected/configured (the map is populated from the Spring-injected List<OcppCallHandler>), and the pipeline is shared across versions, so routing errors must fail gracefully.
  • The compliance checklist (PR Compliance ID 1) requires validating configured values before dereferencing; a missing handler should be treated as a controlled validation/routing failure, not as an NPE.
  • The missing entry can occur due to misconfiguration, a future version being added but not wired, an unsupported protocol, or a context created with an unexpected protocol.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java[52-62]
  • src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/IncomingPipeline.java[92-98]
Suggestion 2: [reliability] `typeStoreHolder` lookup unchecked
`typeStoreHolder` lookup unchecked `Deserializer.handleCall()` selects a version-specific `TypeStore` from `typeStoreHolder` and immediately dereferences it via `typeStore.findRequestClass(action)` without validating the lookup result. If the protocol version key is missing or unknown, this can trigger a NullPointerException and prevent returning a controlled validation/CALL_ERROR response.

Issue description

Deserializer.handleCall(..) retrieves a version-specific TypeStore from typeStoreHolder using the protocol version and then immediately calls typeStore.findRequestClass(action); if the version key is missing/unknown, typeStore is null and this will throw a NullPointerException, which prevents emitting a controlled validation failure / proper OCPP CALL_ERROR.

Issue Context

  • The compliance checklist (PR Compliance ID 1) requires validating configured and request-derived values before dereferencing them, and failing explicitly.
  • The refactor moved version-specific configuration into a shared Deserializer instance, making missing/unknown version handling more important because failures are no longer isolated to a version-specific endpoint.
  • Today, the NPE is not caught inside handleCall, so it propagates to the outer accept(..) catch and gets wrapped in a SteveException, rather than setting an outgoing error on the context for serialization.
  • Add a null check after retrieving typeStore; if null, set an outgoing error on context (e.g., via ErrorFactory.payloadProcessingError(messageId, "Unsupported OCPP version")) and return so the incoming pipeline can serialize/send.
  • Consider also guarding sessionContextStoreHolder.getOrCreate(context.getProtocol().getVersion()) similarly so null/unknown versions produce a controlled error.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java[74-82]
  • src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java[113-155]
  • src/main/java/de/rwth/idsg/steve/ocpp/ws/pipeline/Deserializer.java[148-152]

Pattern 4: Make state-changing setup and scheduling operations exception-safe by rolling back inserted state or cleaning up resources in a finally block when later steps fail. Shared abstractions should own this compensation so every caller receives identical behavior.

Example code before:

long id = store.insert(item);
executor.execute(() -> process(id));
store.delete(id);

Example code after:

long id = store.insert(item);
boolean submitted = false;
try {
    executor.execute(() -> process(id));
    submitted = true;
} finally {
    if (!submitted) {
        store.delete(id);
    }
}
Relevant past accepted suggestions:
Suggestion 1: [reliability] Rejected scheduling leaks task
Rejected scheduling leaks task Each client operation now inserts its task before calling `TaskExecutor.execute`, but no rollback occurs when task submission throws. A rejected submission therefore leaves an unfinished task that was never dispatched in the overview even though the service call itself fails.

Issue description

Tasks are inserted before asynchronous submission, but an exception from TaskExecutor.execute leaves the inserted task permanently unfinished.

Issue Context

Apply the fix to the shared scheduling abstraction so every operation gets identical rollback behavior. Preserve store-before-execution ordering for successfully submitted work.

Fix Focus Areas

  • src/main/java/de/rwth/idsg/steve/service/BackgroundService.java[54-81]
  • src/main/java/de/rwth/idsg/steve/service/ChargePointServiceClient.java[203-209]
  • src/main/java/de/rwth/idsg/steve/repository/TaskStore.java[28-38]
  • src/main/java/de/rwth/idsg/steve/repository/impl/TaskStoreImpl.java[73-80]
Suggestion 2: [reliability] `Issue3IT` leaks station row
`Issue3IT` leaks station row `registerStation()` inserts a random `CHARGE_BOX` record, but the test has no transactional rollback or lifecycle cleanup that deletes it. The database side effect remains after both successful and failed runs, violating exception-safe state restoration.

Issue description

Issue3IT permanently leaves the CHARGE_BOX row created by registerStation().

Issue Context

Add transactional rollback or an exception-safe lifecycle cleanup that removes the generated station even when assertions, connection startup, or client shutdown fail.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/issues/Issue3IT.java[70-87]
  • src/test/java/de/rwth/idsg/steve/issues/Issue3IT.java[113-118]
Suggestion 3: [reliability] Test cleanup not in finally
Test cleanup not in finally `insertAndGetDateTime()` deletes the inserted `CHARGE_BOX` row only on the normal path; any exception before the delete (e.g., empty fetch causing `rows.get(0)`) will skip cleanup and leave stale state. This violates the requirement that side-effect operations (especially in tests) restore/cleanup state in a `try/finally` to remain consistent on failure.

Issue description

The test helper insertAndGetDateTime() performs DB insert/read/delete, but the cleanup deleteFrom(CHARGE_BOX) is not guaranteed to run if an exception occurs before it.

Issue Context

Compliance requires side-effect operations (especially in tests) to be exception-safe so failures do not leave stale DB state that can affect subsequent tests.

Fix Focus Areas

  • src/test/java/de/rwth/idsg/steve/issues/Issue1371Test.java[92-108]

Pattern 5: Keep expensive integration-test workflow steps behind an active, intentional GitHub Actions condition rather than commenting out the guard. If integration tests should run universally, remove stale commented policy and document or isolate the step as a deliberate job.

Example code before:

- name: Run integration tests
  # if: github.event_name == 'push'
  run: ./mvnw verify -Pintegration

Example code after:

- name: Run integration tests
  if: github.event_name == 'push' && github.ref == 'refs/heads/master'
  run: ./mvnw verify -Pintegration
Relevant past accepted suggestions:
Suggestion 1: [performance] ITs always run
ITs always run The GitHub Actions workflow now runs integration tests on all workflow invocations because the prior branch/event guard was commented out, which can significantly increase CI runtime and change PR pipeline behavior.

Issue description

The integration test step no longer has an active if: condition, so *IT tests run for every workflow run that reaches this job.

Issue Context

Previously, ITs were guarded (push to master only). Now the guard is commented out, which likely increases CI duration and may run ITs in contexts where they were intentionally skipped.

Fix Focus Areas

  • .github/workflows/main.yml[71-73]

Suggested fix approach

  • Either restore the original if: condition, or replace it with an intentional condition (e.g., also run on pull_request but maybe only for certain branches/labels), depending on desired CI policy.
  • If the intent is to always run ITs, remove the commented if: line to avoid confusion and document the policy change.
Suggestion 2: [reliability] Integration tests always run
Integration tests always run The GitHub Actions workflow comments out the step-level `if:` guard for running `*IT` tests, making integration tests execute on every workflow run (including PRs) and increasing CI time while making PR checks depend on integration-test stability.

Issue description

The workflow now runs integration tests on every run because the if: condition is commented out.

Issue Context

If the intent was to keep *IT tests only on master pushes (or on a separate schedule), this change will slow down all PRs.

Fix Focus Areas

  • .github/workflows/main.yml[67-73]

Suggested fix

  • Restore a real if: condition (or replace with a configurable input), e.g. run on push to main/master and optionally on a nightly schedule.
  • If the intent is to always run them, consider splitting into a separate job that can be marked optional or run only for certain paths/labels.
Suggestion 3: [reliability] IT step always runs
IT step always runs The integration-test step guard was commented out, making `*IT` tests run on every workflow trigger instead of only `push` to `master`, increasing CI time and potentially breaking PR workflows unexpectedly.

Issue description

The integration-test step is now unconditional because the if: guard was commented out.

Issue Context

This can significantly increase CI runtime and may fail in contexts where integration tests weren't intended to run.

Fix Focus Areas

  • .github/workflows/main.yml[70-72]

Suggested change

Replace #if: ... with a real if: ... (or an updated condition that matches the intended triggers).

[Auto-generated best practices - 2026-09-18]

Add a custom footer
Add a custom sidebar

Clone this wiki locally