Make location lookups resilient against non-conformant data (#26)#358
Draft
morissette wants to merge 4 commits intoCodeForPhilly:mainfrom
Draft
Make location lookups resilient against non-conformant data (#26)#358morissette wants to merge 4 commits intoCodeForPhilly:mainfrom
morissette wants to merge 4 commits intoCodeForPhilly:mainfrom
Conversation
…ForPhilly#26) - lookup(): use COLLATE NOCASE so "montgomery" and "MONTGOMERY" both match "Montgomery" without requiring exact case from callers - lookup(): trim leading/trailing whitespace from all filter values before querying, so "Montgomery " works as well as "Montgomery" - lookupFuzzy(): new method for prefix/abbreviation matching; strips trailing periods then appends % for LIKE COLLATE NOCASE, so "Mont." matches both "Montgomery" and "Montour" - Extract shared JDBC execution into private lookupWithOperator() to eliminate duplication between lookup() and lookupFuzzy() - Both changes applied identically to library-api and builder-api Tests: - Add LocationServiceTest (11 tests) covering exact match, case insensitivity, whitespace trimming, fuzzy prefix, fuzzy abbreviation, and empty-result cases - Add src/test/resources/application.properties with quarkus.http.test-port=0 so tests don't conflict with running services on port 8081 (fixes pre-existing issue where all 14 existing tests were being skipped in local dev) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Errors should never pass silently:
- Replace e.printStackTrace() with a proper Logger and re-throw as
RuntimeException so callers can distinguish "no results" from
"query failed". Silent swallowing made DB failures invisible.
Flat is better than nested:
- Replace try/finally/null-guard pattern with try-with-resources.
Connection, PreparedStatement, and ResultSet are all AutoCloseable;
the old 3-level nesting with manual null checks is now a single
try-with-resources block.
Explicit is better than implicit / Readability counts:
- Rename lookupWithOperator -> query; the old name described the
mechanism rather than the intent.
- Move COLLATE NOCASE out of the operator string and into the SQL
builder so the operator param is a plain SQL operator ("=" or
"LIKE"), not an opaque fragment embedding a JDBC placeholder.
- Rename normalised -> patterns in lookupFuzzy; the variable holds
LIKE patterns, not normalised data.
Beautiful is better than ugly:
- Replace inline java.util.LinkedHashMap with a proper import.
- Add Logger as a named constant rather than inline creation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Filter values were already parameterized, but column names and filter keys were interpolated directly into the SQL string. Validate both against an allowlist of known locations-table columns before building the query. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix ALLOWED_COLUMNS: remove non-existent 'stateName', add 'countyFips' which is a real column in the locations schema - Guard against empty filters before building SQL (would produce invalid syntax and a cryptic SQLException) - Move ALLOWED_COLUMNS constant to top of class per Java convention - Import Set properly instead of inlining java.util.Set - Make getDbConnection() package-private (only called via Arc handle within the same class; not part of the public API) - Remove redundant inline comment in lookupFuzzy (covered by Javadoc) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Closes #26
Summary
COLLATE NOCASEso"montgomery","MONTGOMERY", and"Montgomery"all return the same results."Montgomery "and" Montgomery"work correctly.lookupFuzzy()method uses SQLLIKEwith a%suffix so abbreviations like"Mont."match"Montgomery"and"Montour". Trailing periods are stripped before the wildcard is appended.locationstable columns (zipCode,countyName,countyFips,stateAbbreviation) before being interpolated into SQL. Filter values were already parameterized.e.printStackTrace()with a structured log +RuntimeExceptionso failures surface instead of returning empty results unexpectedly.try/finallyblocks with try-with-resources forConnection,PreparedStatement, andResultSet.@QuarkusTesttests covering exact match, case-insensitive variants, whitespace trimming, empty result, and all fuzzy matching cases. Also fixed a pre-existing test port conflict (quarkus.http.test-port=0) that was causing all existing tests to be silently skipped in local dev when another service occupied port 8081. All 25 tests pass.Changes applied to both
library-apiandbuilder-apicopies ofLocationService.Manual testing note
No checks currently call
LocationService, so there is no end-to-end path to exercise this through the screener UI. Residence checks would be the natural place to wire this in — for example, a "lives in county X" check that looks up whether a ZIP code falls within a given county. That work is out of scope for this PR but would be needed to validate the feature manually.Test plan
cd library-api && mvn test— all 25 tests passLocationService.lookup()orLocationService.lookupFuzzy()and verify results in the screener UI🤖 Generated with Claude Code