Add reusable query parameter validation middleware#782
Add reusable query parameter validation middleware#782jeyamoorthi wants to merge 1 commit intoOneBusAway:mainfrom
Conversation
aaronbrethorst left a comment
There was a problem hiding this comment.
Verdict: Request Changes
Hey jeyamoorthi — nice concept here! The idea of centralizing query parameter validation into composable middleware is solid, and the code itself is clean and well-tested. Unfortunately there's a showstopper that needs fixing before this can merge.
Critical Issues (1 found)
1. Broken filename prevents the PR from being usable
The main source file is named:
internal/restapi/n success, calls next.ServeHTTP(w, r) [NEW] validation_middleware.go This appears to be an accidental paste of a description/note into the filename. It should be validation_middleware.go. This filename:
- Contains spaces and special characters that will cause problems across tooling
- Is not a valid Go source file name by convention
- Will confuse IDEs, CI, and other developers
Fix: Rename the file to internal/restapi/validation_middleware.go. Use:
git mv "internal/restapi/n success, calls next.ServeHTTP(w, r) [NEW] validation_middleware.go" internal/restapi/validation_middleware.goImportant Issues (1 found)
1. Empty string values silently pass validation
query.Get(rule.Param) returns "" for both absent parameters and parameters explicitly set to an empty string (e.g., ?maxCount=). The middleware treats both cases as "not provided" and skips validation. This means ?maxCount= bypasses the PositiveIntRule entirely and reaches the handler, where it may cause unexpected behavior depending on how the handler parses the value.
Consider distinguishing between "absent" and "present but empty":
if !query.Has(rule.Param) { continue // truly absent, skip } value := query.Get(rule.Param) // Now validate even if value is ""url.Values.Has() is available in Go 1.17+.
Suggestions (1 found)
1. Consider whether *RestAPI coupling is necessary
ValidateQueryParams takes *RestAPI solely to call api.validationErrorResponse. An alternative would be to have the middleware return errors in a way that doesn't require the full API struct — e.g., writing the JSON error response directly or accepting an error-writer interface. This would make the middleware more testable in isolation (the unit rule tests already work without *RestAPI; only the integration tests need it).
This is a minor design point — the current approach is consistent with how other middleware in this codebase works, so it's fine to keep as-is.
Strengths
- Well-structured rule abstraction:
QueryParamRuleis composable and easy to extend with new rule types - Comprehensive tests: Good coverage of edge cases (zero, negative, non-numeric, boundary values, multiple rules, error aggregation)
- Collects all errors: The middleware validates all parameters before responding, giving the caller a complete picture of what's wrong — much better than failing on the first error
- Proper use of existing error response format: Reuses
validationErrorResponsefor consistent API behavior - Clean PR scope: Wisely introduces the middleware without modifying existing handlers, making it easy to review
Recommended Action
- Rename the file — this is the blocker
- Consider the empty-string edge case — important for correctness
- After fixes, this should be ready to merge
Summary
Introduces a reusable query parameter validation middleware for the REST API.
Problem
Validation logic for query parameters is currently implemented inconsistently across handlers, with some cases duplicating logic and others silently ignoring invalid input.
Solution
Added a centralized middleware that applies validation rules to query parameters before the request reaches the handler. The middleware uses a composable rule-based approach, allowing each parameter to define its own validation logic.
Changes
QueryParamRuleabstraction for defining validation rulesPositiveIntRuleIntRangeRuleNonNegativeIntRuleValidateQueryParamsmiddleware to enforce validationScope
This PR only introduces the validation middleware and tests.
No existing handlers or routes are modified to keep the change minimal and safe for review.
Future Work
Benefits