758 - Unable to diagnose Mailjet-related errors due to missing logs #395

mariusmarin-dev · 2025-12-22T19:33:11Z

No description provided.

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

checkstyle

tag on the next line.

github-actions · 2025-12-22T19:33:44Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+
+      } catch (MailjetRateLimitException e) {
+        metrics.incrementRateLimitError();
+        log.warn("MAILJETT - Mailjet rate limit exceeded while processing user ID: {}. Processed {} users before limit.",


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.sizes.LineLengthCheck> _{reported by reviewdog 🐶}
Line is longer than 120 characters (found 121).

github-actions · 2025-12-22T19:33:44Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    private int mailjetErrorCount = 0;
+    private int unexpectedErrorCount = 0;
+
+    void incrementSuccess() { successCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 29 should have line break after.

github-actions · 2025-12-22T19:33:44Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    private int mailjetErrorCount = 0;
+    private int unexpectedErrorCount = 0;
+
+    void incrementSuccess() { successCount++; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 47 should be alone on a line.

github-actions · 2025-12-22T19:33:44Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    private int unexpectedErrorCount = 0;
+
+    void incrementSuccess() { successCount++; }
+    void incrementCreated() { createdCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
'METHOD_DEF' should be separated from previous line.

github-actions · 2025-12-22T19:33:44Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    private int unexpectedErrorCount = 0;
+
+    void incrementSuccess() { successCount++; }
+    void incrementCreated() { createdCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 29 should have line break after.

github-actions · 2025-12-22T19:33:45Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementUnsubscribed() { unsubscribedCount++; }
+    void incrementSkipped() { skippedCount++; }
+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 40 should have line break after.

github-actions · 2025-12-22T19:33:45Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementUnsubscribed() { unsubscribedCount++; }
+    void incrementSkipped() { skippedCount++; }
+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 69 should be alone on a line.

github-actions · 2025-12-22T19:33:45Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementSkipped() { skippedCount++; }
+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
'METHOD_DEF' should be separated from previous line.

github-actions · 2025-12-22T19:33:45Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementSkipped() { skippedCount++; }
+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 36 should have line break after.

github-actions · 2025-12-22T19:33:45Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementSkipped() { skippedCount++; }
+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 61 should be alone on a line.

src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java

src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

checkstyle

tag on the next line.

github-actions · 2025-12-23T22:59:48Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+
+      } catch (MailjetRateLimitException e) {
+        metrics.incrementRateLimitError();
+        log.warn("MAILJET - Mailjet rate limit exceeded while processing user ID: {}. " +


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.OperatorWrapCheck> _{reported by reviewdog 🐶}
'+' should be on a new line.

github-actions · 2025-12-23T22:59:48Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }
+    void incrementMailjetError() { mailjetErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
'METHOD_DEF' should be separated from previous line.

github-actions · 2025-12-23T22:59:48Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }
+    void incrementMailjetError() { mailjetErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 34 should have line break after.

github-actions · 2025-12-23T22:59:48Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementDatabaseError() { databaseErrorCount++; }
+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }
+    void incrementMailjetError() { mailjetErrorCount++; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 57 should be alone on a line.

github-actions · 2025-12-23T22:59:48Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    void incrementCommunicationError() { communicationErrorCount++; }
+    void incrementRateLimitError() { rateLimitErrorCount++; }
+    void incrementMailjetError() { mailjetErrorCount++; }
+    void incrementUnexpectedError() { unexpectedErrorCount++; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
'METHOD_DEF' should be separated from previous line.

github-actions · 2025-12-23T22:59:49Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    int getDeletedCount() { return deletedCount; }
+    int getEmailChangedCount() { return emailChangedCount; }
+    int getUnsubscribedCount() { return unsubscribedCount; }
+    int getSkippedCount() { return skippedCount; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 27 should have line break after.

github-actions · 2025-12-23T22:59:49Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    int getDeletedCount() { return deletedCount; }
+    int getEmailChangedCount() { return emailChangedCount; }
+    int getUnsubscribedCount() { return unsubscribedCount; }
+    int getSkippedCount() { return skippedCount; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 50 should be alone on a line.

github-actions · 2025-12-23T22:59:49Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    int getEmailChangedCount() { return emailChangedCount; }
+    int getUnsubscribedCount() { return unsubscribedCount; }
+    int getSkippedCount() { return skippedCount; }
+    int getDatabaseErrorCount() { return databaseErrorCount; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.EmptyLineSeparatorCheck> _{reported by reviewdog 🐶}
'METHOD_DEF' should be separated from previous line.

github-actions · 2025-12-23T22:59:49Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    int getEmailChangedCount() { return emailChangedCount; }
+    int getUnsubscribedCount() { return unsubscribedCount; }
+    int getSkippedCount() { return skippedCount; }
+    int getDatabaseErrorCount() { return databaseErrorCount; }


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 33 should have line break after.

github-actions · 2025-12-23T22:59:49Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

+    int getEmailChangedCount() { return emailChangedCount; }
+    int getUnsubscribedCount() { return unsubscribedCount; }
+    int getSkippedCount() { return skippedCount; }
+    int getDatabaseErrorCount() { return databaseErrorCount; }


🚫 [checkstyle] _{reported by reviewdog 🐶}
'}' at column 62 should be alone on a line.

src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

checkstyle

src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java

github-actions · 2026-01-02T10:09:46Z

Coverage Report

Overall Project	31.31% `-0.35%`	❌
Files changed	71.09%	❌

File	Coverage
ExternalAccountManager.java	93.68% `-6.32%`	✅
MailJetApiClientWrapper.java	85.55% `-14.32%`	✅
PgExternalAccountPersistenceManager.java	34.75% `-59.64%`	❌
SegueGuiceConfigurationModule.java	5.2% `-0.4%`	❌

github-actions · 2026-01-02T10:33:25Z

src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java

+
+  /**
+   * Extract stage information from registered_contexts JSONB[] field.
+   *


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocParagraphCheck> _{reported by reviewdog 🐶}
Empty line should be followed by
tag on the next line.

github-actions · 2026-01-02T10:33:25Z

src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java

+  private String fallbackStageDetection(String jsonString) {
+    String lower = jsonString.toLowerCase();
+    boolean hasGcse = lower.contains("gcse");
+    boolean hasALevel = lower.contains("a_level") || lower.contains("alevel") || lower.contains("a level");


📝 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.naming.AbbreviationAsWordInNameCheck> _{reported by reviewdog 🐶}
Abbreviation in name 'hasALevel' must contain no more than '1' consecutive capital letters.

github-actions · 2026-01-02T11:21:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java

 import uk.ac.cam.cl.dtg.segue.dao.users.IExternalAccountDataManager;
 import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper;
 import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+import org.junit.jupiter.params.ParameterizedTest;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'org.junit.jupiter.params.ParameterizedTest' import. Should be before 'uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction'.

github-actions · 2026-01-02T11:21:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java

 import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper;
 import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'org.junit.jupiter.params.provider.NullAndEmptySource' import. Should be before 'uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction'.

github-actions · 2026-01-02T11:21:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java

 import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
+import org.junit.jupiter.params.provider.ValueSource;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'org.junit.jupiter.params.provider.ValueSource' import. Should be before 'uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction'.

github-actions · 2026-01-02T11:21:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java

+import org.junit.jupiter.params.provider.NullAndEmptySource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.util.ArrayList;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Extra separation in import group before 'java.util.ArrayList'

github-actions · 2026-01-02T11:21:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java

+import org.junit.jupiter.params.provider.NullAndEmptySource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.util.ArrayList;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'java.util.ArrayList' import. Should be before 'uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction'.

github-actions · 2026-01-02T11:21:55Z

src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java

 import com.mailjet.client.MailjetResponse;
+import com.mailjet.client.errors.MailjetClientRequestException;
 import com.mailjet.client.errors.MailjetException;
+import com.mailjet.client.errors.MailjetClientCommunicationException;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'com.mailjet.client.errors.MailjetClientCommunicationException' import. Should be before 'com.mailjet.client.errors.MailjetException'.

github-actions · 2026-01-02T11:21:55Z

src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java

 import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper;
+import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+
+import static org.easymock.EasyMock.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Import statement for 'org.easymock.EasyMock.*' is in the wrong order. Should be in the 'STATIC' group, expecting not assigned imports on this line.

github-actions · 2026-01-02T11:21:55Z

src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java

 import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper;
+import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+
+import static org.easymock.EasyMock.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck> _{reported by reviewdog 🐶}
Using the '.' form of import should be avoided - org.easymock.EasyMock..

github-actions · 2026-01-02T11:21:55Z

src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java

+import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+
+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Import statement for 'org.junit.jupiter.api.Assertions.*' is in the wrong order. Should be in the 'STATIC' group, expecting not assigned imports on this line.

github-actions · 2026-01-02T11:21:55Z

src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java

+import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction;
+
+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck> _{reported by reviewdog 🐶}
Using the '.' form of import should be avoided - org.junit.jupiter.api.Assertions..

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+      String registeredContextsJson = "[{\"stage\": \"gcse\"}]";
+
+      expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+      expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);


src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+
+      expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+      expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);
+      expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet);


In general, the best fix for a database resource leak is to ensure that every ResultSet, Statement, and similar AutoCloseable is closed in all control-flow paths (including exceptions), typically using try-with-resources. Here we can’t change the production code, but we can address the CodeQL warning and strengthen the tests by making the resource-handling expectations explicit and consistent.

The core issue CodeQL sees is a ResultSet returned by executeQuery() that is not always closed. In this test class we already verify close() calls on the ResultSet, PreparedStatement, and Connection in the first two tests. To make the behavior clear and to give the analyzer a well-structured resource lifecycle, we can:

Add EasyMock expectations for closing the ResultSet, PreparedStatement, and Connection in the error-path test if the real code is expected to close these even on failure; or

If the connection fails before any of these are created (which is what the current third test models), we can add an explicit test that covers an exception occurring after executeQuery() has returned a ResultSet and verify that close() is still invoked on all resources.

Because we cannot modify the production implementation and want to keep existing behavior unchanged, the second option is cleaner: add a new test method in the same nested class that simulates a failure after the query has been executed. In that new test we will:

Arrange the mocks so that getDatabaseConnection(), prepareStatement(...), and executeQuery() all succeed and return our mocks.

Have mockResultSet.next() throw a SQLException on first call to simulate a processing error after the ResultSet is acquired.

Set expectations that close() is called on mockResultSet, mockPreparedStatement, and mockConnection despite the exception.

Assert that persistenceManager.getRecentlyChangedRecords() throws SegueDatabaseException.

This directly exercises the path CodeQL is worried about (where a ResultSet exists but an exception aborts normal processing) and confirms that it is closed. All changes are confined to src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java within the shown nested test class; no new imports are needed.

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+    void getRecentlyChangedRecords_WithEmptyResults_ShouldReturnEmptyList() throws Exception {
+      // Arrange
+      expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+      expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);


In general, to fix database resource leaks, you must ensure that all AutoCloseable JDBC resources (ResultSet, PreparedStatement, Connection when owned by the method) are closed on all code paths, preferably using try‑with‑resources. In tests using mocks, this means setting expectations that close() is called even when errors occur, so that the tests enforce correct resource management in the production code.

Within PgExternalAccountPersistenceManagerTest, the “happy path” tests already expect mockResultSet.close(), mockPreparedStatement.close(), and mockConnection.close(). However, the test getRecentlyChangedRecords_WithDatabaseError_ShouldThrowException configures mockDatabase.getDatabaseConnection() to throw, and then only verifies that the exception is wrapped in a SegueDatabaseException; it does not assert anything about resource closure. To make the test consistent with the others and with proper resource management, we should:

Use createMock(Connection.class) and createMock(PreparedStatement.class) as local mocks in this error test.

Configure mockDatabase.getDatabaseConnection() to return the local Connection mock.

Configure connection.prepareStatement(anyString()) to throw SQLException.

Set expectations that connection.close() is called (and, if any statement were created successfully, that it would be closed as well).

Include these mocks in the replay and verify calls.

This updated test will assert that even when preparing the statement fails, the Connection is closed. It also removes the need for the static analyzer to infer unverified resource closure behavior on this path.

All changes are confined to the getRecentlyChangedRecords_WithDatabaseError_ShouldThrowException test method in src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java; no new imports or helper methods are required.

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+      // Arrange
+      expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+      expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);
+      expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet);


In general, to fix the problem you ensure that any ResultSet obtained from PreparedStatement.executeQuery() is always closed, including in error paths. For Java 7+, this is best done via try‑with‑resources in the production code. In this specific test file, we can’t edit PgExternalAccountPersistenceManager, but we can make the tests assert that ResultSet.close() is called in all paths where a ResultSet is obtained. This aligns with the recommendation and satisfies CodeQL’s dataflow that assumes the executeQuery() result is a resource requiring closure.

The best, minimal change within the shown file is to add a new test (or extend an existing one) that exercises a scenario where executeQuery() succeeds but something later fails, and explicitly sets and verifies expectations that mockResultSet.close() (and other JDBC resources) are called in that scenario as well. Since we cannot see or change the internals of getRecentlyChangedRecords, we should not alter existing successful-path tests except as necessary, but we can add an additional failure-path test where prepareStatement succeeds and executeQuery() is called, then a later operation throws, while still requiring that ResultSet.close() is invoked. This reinforces the contract that any ResultSet opened by getRecentlyChangedRecords must be closed regardless of success or failure, which addresses the leak warning.

Concretely, in PgExternalAccountPersistenceManagerTest.java:

Within the GetRecentlyChangedRecordsTests nested class, add a new test method, e.g. getRecentlyChangedRecords_WhenProcessingRowFails_ShouldCloseResourcesAndThrow, that:

Mocks getDatabaseConnection(), prepareStatement(...), and executeQuery() to return mockResultSet.

Mocks mockResultSet.next() to return true once and then throw a SQLException (to simulate a processing error after the query is executed and a row is available).

Sets expectations that mockResultSet.close(), mockPreparedStatement.close(), and mockConnection.close() are called.

Replays all mocks, calls persistenceManager.getRecentlyChangedRecords(), and asserts that a SegueDatabaseException is thrown, then verifies the mocks.

No new imports are needed; all required types (SQLException, SegueDatabaseException, JUnit, EasyMock) are already imported.

This change keeps existing functionality intact and simply strengthens the tests to ensure that ResultSet is closed across an additional exceptional path that begins with executeQuery(), thereby addressing the CodeQL concern about the ResultSet created on line 94.

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

github-actions · 2026-01-02T11:43:24Z

src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java

+  /**
+   * Extract stage information from registered_contexts JSONB[] field.
+   *
+   * <p>


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocParagraphCheck> _{reported by reviewdog 🐶}
tag should be placed immediately before the first word, with no space after.

github-actions · 2026-01-02T13:16:53Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.sql.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Extra separation in import group before 'java.sql.*'

github-actions · 2026-01-02T13:16:53Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.sql.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'java.sql.*' import. Should be before 'uk.ac.cam.cl.dtg.util.ReflectionUtils'.

github-actions · 2026-01-02T13:16:54Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.sql.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportCheck> _{reported by reviewdog 🐶}
Using the '.' form of import should be avoided - java.sql..

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

github-actions · 2026-01-02T13:35:17Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

-import uk.ac.cam.cl.dtg.util.ReflectionUtils;
+import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException;
+import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb;
+import org.junit.jupiter.api.Nested;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'org.junit.jupiter.api.Nested' import. Should be before 'uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb'.

github-actions · 2026-01-02T13:35:17Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb;
+import org.junit.jupiter.api.Nested;
+
+import java.sql.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'java.sql.*' import. Should be before 'uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb'.

github-actions · 2026-01-02T13:35:17Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import org.junit.jupiter.api.Nested;
+
+import java.sql.*;
+import java.util.List;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'java.util.List' import. Should be before 'uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb'.

github-actions · 2026-01-02T13:35:17Z

src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java

+import static org.easymock.EasyMock.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.sql.*;


🚫 [checkstyle] <com.puppycrawl.tools.checkstyle.checks.imports.CustomImportOrderCheck> _{reported by reviewdog 🐶}
Wrong lexicographical order for 'java.sql.*' import. Should be before 'uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb'.

sonarqubecloud · 2026-01-02T14:33:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarQube Cloud

mariusmarin-dev added 9 commits December 18, 2025 21:38

PATCH 8

54c8460

PATCH 9

e136ec0

PATCH 10

89aedb6

PATCH 11

51ae442

PATCH 12

25cc8a9

PATCH 13

86df1fc

PATCH 14

8113130

PATCH 15

0343513

PATCH 16

87711b7

github-actions bot reviewed Dec 22, 2025

View reviewed changes

github-advanced-security bot found potential problems Dec 22, 2025

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java Fixed Show fixed Hide fixed

src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java Fixed Show fixed Hide fixed

PATCH 17

6a97aca

github-actions bot reviewed Dec 23, 2025

View reviewed changes

github-advanced-security bot found potential problems Dec 23, 2025

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java Fixed Show fixed Hide fixed

PATCH 18

93b42c2

github-actions bot reviewed Jan 2, 2026

View reviewed changes

mariusmarin-dev added 2 commits January 2, 2026 11:38

PATCH 19

c7332f5

PATCH 19

c15160c

PATCH 19

e429eae

github-actions bot reviewed Jan 2, 2026

View reviewed changes

PATCH 19

69e303d

github-actions bot reviewed Jan 2, 2026

View reviewed changes

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

mariusmarin-dev added 2 commits January 2, 2026 13:42

PATCH 19

acef139

PATCH 19

5309df8

github-actions bot reviewed Jan 2, 2026

View reviewed changes

mariusmarin-dev added 2 commits January 2, 2026 14:03

PATCH 19

a86135e

PATCH 19

586ea07

github-actions bot reviewed Jan 2, 2026

View reviewed changes

github-advanced-security bot found potential problems Jan 2, 2026

View reviewed changes

PATCH 19

8b435fd

github-actions bot reviewed Jan 2, 2026

View reviewed changes

mariusmarin-dev added 2 commits January 2, 2026 16:07

PATCH 19

cf78d57

PATCH 19

c0270c3

madhurabhujbal approved these changes Jan 8, 2026

View reviewed changes

mariusmarin-dev merged commit 781fee4 into main Jan 8, 2026
6 checks passed

mariusmarin-dev deleted the mailjet_patch_2 branch January 8, 2026 15:13

@@ -124,6 +124,33 @@
                   verify(mockDatabase);
                 }
+                @Test
+                void getRecentlyChangedRecords_WhenResultSetProcessingFails_ShouldCloseResources()
+                    throws Exception {
+                  // Arrange
+                  expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+                  expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);
+                  expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet);
+                  // Simulate an exception while iterating over the ResultSet after it has been obtained.
+                  expect(mockResultSet.next()).andThrow(new SQLException("ResultSet processing failed"));
+                  mockResultSet.close();
+                  expectLastCall();
+                  mockPreparedStatement.close();
+                  expectLastCall();
+                  mockConnection.close();
+                  expectLastCall();
+                  replay(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet);
+                  // Act & Assert
+                  assertThrows(SegueDatabaseException.class,
+                      () -> persistenceManager.getRecentlyChangedRecords());
+                  verify(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet);
+                }
               }
               // Helper method to setup mock ResultSet with all expected calls

@@ -114,15 +114,23 @@
                 @Test
                 void getRecentlyChangedRecords_WithDatabaseError_ShouldThrowException() throws Exception {
                   // Arrange
-                  expect(mockDatabase.getDatabaseConnection()).andThrow(new SQLException("Connection failed"));
+                  Connection failingConnection = createMock(Connection.class);
+                  PreparedStatement failingPreparedStatement = createMock(PreparedStatement.class);
-                  replay(mockDatabase);
+                  expect(mockDatabase.getDatabaseConnection()).andReturn(failingConnection);
+                  expect(failingConnection.prepareStatement(anyString()))
+                      .andThrow(new SQLException("Connection failed"));
+                  failingConnection.close();
+                  expectLastCall();
+                  replay(mockDatabase, failingConnection, failingPreparedStatement);
                   // Act & Assert
                   assertThrows(SegueDatabaseException.class,
                       () -> persistenceManager.getRecentlyChangedRecords());
-                  verify(mockDatabase);
+                  verify(mockDatabase, failingConnection, failingPreparedStatement);
                 }
               }

@@ -124,6 +124,34 @@
                   verify(mockDatabase);
                 }
+                @Test
+                void getRecentlyChangedRecords_WhenProcessingRowFails_ShouldCloseResourcesAndThrow()
+                    throws Exception {
+                  // Arrange
+                  expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection);
+                  expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement);
+                  expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet);
+                  // First call to next() returns a row, second call throws an exception to simulate failure
+                  expect(mockResultSet.next()).andReturn(true).once();
+                  expect(mockResultSet.next()).andThrow(new SQLException("Row processing failed"));
+                  mockResultSet.close();
+                  expectLastCall();
+                  mockPreparedStatement.close();
+                  expectLastCall();
+                  mockConnection.close();
+                  expectLastCall();
+                  replay(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet);
+                  // Act & Assert
+                  assertThrows(SegueDatabaseException.class,
+                      () -> persistenceManager.getRecentlyChangedRecords());
+                  verify(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet);
+                }
               }
               // Helper method to setup mock ResultSet with all expected calls

758 - Unable to diagnose Mailjet-related errors due to missing logs #395

758 - Unable to diagnose Mailjet-related errors due to missing logs #395

Uh oh!

Conversation

mariusmarin-dev commented Dec 22, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 2, 2026 •

edited

Loading