Skip to content

Conversation

@Montana
Copy link
Contributor

@Montana Montana commented Sep 4, 2025

Hey all,

Your original code created a Statement and ResultSet but never closed them. JDBC objects hold sockets and memory buffers; if they aren’t closed, they can leak. The new version uses try-with-resources, which guarantees everything is closed correctly, even if an exception is thrown.

Before:

ResultSet rs = s.executeQuery("SELECT COUNT(*) FROM foo");
rs.next();
assertEquals(0, rs.getInt(1));

After:

try (ResultSet rs = s.executeQuery("SELECT COUNT(*) AS cnt FROM foo")) {
    rs.next();
    long count = rs.getLong("cnt");
    assertEquals(0L, count);
}

The other thing is COUNT(*) can exceed Integer.MAX_VALUE if a table grows large. Using getLong avoids silent overflows. Even in a test, it’s better to match the SQL type (BIGINT) with the right Java type (long).

I ran this locally, passed as expected.

Cheers,
Michael

@mikebell90
Copy link
Contributor

mikebell90 commented Sep 5, 2025

The clean up resources, fine. That just good code style although since it's in a test it's NOT REALLY going to run out of memory without quite a few more tests added. The other? A nitpick. I'll accept it because I appreciate contributions, but this migration is CONTROLLED and ephemeral. So it's NOT going to exceed an int. That's spurious and kind of wasteful memory wise. That argument would say NEVER use an int. ;)

@mikebell90 mikebell90 merged commit 3268649 into opentable:master Sep 5, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants