registry icon indicating copy to clipboard operation
registry copied to clipboard

feat: add regex validation to server name in API types (#471)

Open ossamalafhel opened this issue 5 months ago • 0 comments

Summary

Following @domdomegg's suggestion in #476, this PR started as adding regex validation to the server name field but evolved based on extensive reviewer feedback into a comprehensive validation improvement.

Changes

Initial Implementation

  • Added pattern validation to the Name field in pkg/api/v0/types.go
  • Updated tests to expect HTTP 422 for validation errors

Based on Review Feedback

  • Removed regex from API annotations - Moved validation to handler code to maintain HTTP 400 status (more familiar than 422) as suggested by @joelverhagen
  • Updated regex pattern - Changed from ^[^/]+/[^/]+$ to the more specific ^[a-zA-Z0-9.-]+/[a-zA-Z0-9._-]+$ to match the schema definition
  • Moved regex patterns to utils.go - Centralized all validation regexes in one place as suggested by @Avish34
  • Added error constants to constants.go - Created ErrInvalidNamespaceCharacters and ErrInvalidNameCharacters for consistent error messages
  • Improved error messages - Now provides clearer messages like "server name must match pattern..." instead of generic validation errors
  • Removed redundant validation - Eliminated unnecessary strings.Contains(name, "//") check that was already covered by slash count validation

Key Decisions

  • Chose handler validation over schema validation to maintain HTTP 400 status and provide better error messages
  • Kept case-sensitive validation allowing uppercase letters (discussion with @joelverhagen about potentially enforcing lowercase for future consideration)
  • Centralized validation logic following the existing codebase patterns

Context

This PR demonstrates the value of code review - what started as a simple regex addition evolved into a better implementation that:

  • Maintains backward compatibility with HTTP 400 status codes
  • Provides clearer, more actionable error messages
  • Follows the codebase's existing patterns for validation
  • Makes the code more maintainable by centralizing regex patterns and error messages

Related to #471 Builds on #476

Testing

  • All existing tests updated and passing
  • Added comprehensive test cases for the new validation pattern
  • Verified error messages are clear and helpful
  • Tested various edge cases discovered during review

Note: Special thanks to @joelverhagen, @Avish34, and @domdomegg for their thorough reviews that significantly improved this implementation.

ossamalafhel avatar Sep 15 '25 12:09 ossamalafhel