Guard against null ResultSet in EnvironmentRepositoryImpl#134
Merged
Conversation
Add the missing null-ResultSet guard to the seven query methods of EnvironmentRepositoryImpl (findAll, findById, findByName, findByEntityId, findEntityIdsByEnvironmentId, findLocationIdsByEnvironmentId, findGridIdsByEnvironmentId), matching the pattern already used by GridRepositoryImpl, EntityRepositoryImpl, and LocationRepositoryImpl. Previously these methods called rs.next() directly, so a null ResultSet (the signal DbInteractions.query returns on failure) caused a NullPointerException instead of an empty result. They now log and return an empty result, consistent with the sibling repositories. Adds the corresponding ...ReturnsEmptyWhenResultSetIsNull tests (which fail with NPE without the guard) and removes the now-obsolete class comment documenting the omission. Closes #132 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dmccoystephenson
left a comment
Member
Author
There was a problem hiding this comment.
Self-review rubric (adversarial; anchored on green CI run 27466954846 + a direct regression demonstration):
- Scope: PASS — only two files, both required for #132:
EnvironmentRepositoryImpl.java(the guards) and its test. No unrelated edits. - Tests-fix: PASS — demonstrated, not asserted. Reverting just the production file to
mainand re-running the new null tests errors withNullPointerException: Cannot invoke "ResultSet.next()" because "rs" is nullon all sampled cases; with the guard they pass. Seven...ReturnsEmptyWhenResultSetIsNulltests, one per guarded method. - Tests-new: N/A — no new public methods; guards added to existing ones.
- Sibling structure: PASS — each guard mirrors
GridRepositoryImplexactly:if (rs == null) { log.error("<method prefix>: ResultSet is null"); return <empty>; }, reusing each method's existing log-message prefix. - Sibling renames: N/A — no identifiers renamed.
- Docs: N/A — the change is internal error-handling (null → empty result); no API/DTO/endpoint contract or Phase 7 doc source-of-truth row is affected.
- Issue resolution: PASS — #132's named surface (the seven query methods lacking the guard) is fully addressed;
grepconfirms 7 guards + 7 null tests. - CI: PASS —
buildgreen on PR head. - DTO boundary / Spec alignment: N/A — no controller or endpoint change.
- Java/Python parallelism: PASS — repository layer is Java-only; the Python client has no repository mirror.
- Override correctness: PASS —
git diffshows zero@Overridelines added or changed in production.
Observation (not a FAIL, pre-existing & repo-wide): treating a null ResultSet (a failed query) as an empty result silently hides DB failures from callers. That is the established convention across all four repositories, so this PR conforms rather than introduces it; changing the convention is out of scope.
Summary: ready to merge — the fix is small, matches the sibling pattern, and the regression is proven by the new tests failing without the guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
if (rs == null)guard to the seven query methods ofEnvironmentRepositoryImpl(findAll,findById,findByName,findByEntityId,findEntityIdsByEnvironmentId,findLocationIdsByEnvironmentId,findGridIdsByEnvironmentId), matching the existing pattern inGridRepositoryImpl,EntityRepositoryImpl, andLocationRepositoryImpl.rs.next()directly, so anullResultSet— the valueDbInteractions.query(...)returns on failure — produced aNullPointerExceptioninstead of an empty result, diverging from every sibling repository."... : ResultSet is null") and returns the empty result (Optional.empty()/ empty list), exactly as the siblings do.Tests
...ReturnsEmptyWhenResultSetIsNullcases toEnvironmentRepositoryImplTest. These are the regression guard: without the newrs == nullcheck each one throwsNullPointerExceptionatrs.next()and fails.Test plan
./mvnw test -Dtest=EnvironmentRepositoryImplTest— 45/45 pass (JDK 21)./mvnw test(full suite) — 213/213 pass, no regressionssrc/main/pythonchange)Notes
save,deleteById,updateName,delete*) are unchanged: they delegate toDbInteractions.update, which returns abooleanand has noResultSetto guard.Closes #132
🤖 Generated with Claude Code