Skip to content

Add reusable query parameter validation middleware#782

Open
jeyamoorthi wants to merge 1 commit intoOneBusAway:mainfrom
jeyamoorthi:feat/validation-middleware
Open

Add reusable query parameter validation middleware#782
jeyamoorthi wants to merge 1 commit intoOneBusAway:mainfrom
jeyamoorthi:feat/validation-middleware

Conversation

@jeyamoorthi
Copy link

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

  • Added QueryParamRule abstraction for defining validation rules
  • Implemented reusable rules:
    • PositiveIntRule
    • IntRangeRule
    • NonNegativeIntRule
  • Added ValidateQueryParams middleware to enforce validation
  • Added comprehensive unit tests covering valid, invalid, and edge cases

Scope

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

  • Apply this middleware incrementally to existing endpoints
  • Remove duplicated inline validation logic across handlers

Benefits

  • Improves API consistency and reliability
  • Prevents silent failures on invalid input
  • Reduces duplicated validation logic
  • Provides a scalable foundation for future validation needs
Copy link
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go

Important 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: QueryParamRule is 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 validationErrorResponse for consistent API behavior
  • Clean PR scope: Wisely introduces the middleware without modifying existing handlers, making it easy to review

Recommended Action

  1. Rename the file — this is the blocker
  2. Consider the empty-string edge case — important for correctness
  3. After fixes, this should be ready to merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants