Utilize validators #18
Conversation
…idators for MatriculationNumber and TUMID
WalkthroughThis set of changes introduces validation tags to struct fields in the codebase, ensuring input data conforms to specified formats (such as UUID, email, and custom formats for matriculation numbers and university logins). It adds custom validation logic for these fields, integrates new direct dependencies for validation and testing, and provides comprehensive unit tests for both the validation logic and its application to domain structs. Additionally, role constants are reorganized for clarity, and struct fields are annotated for JSON serialization and validation. No functional or behavioral changes are introduced beyond enhanced validation and code organization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GinHandler
participant Validator
participant CustomValidator
Client->>GinHandler: Submit struct data (e.g., Student, Resolution)
GinHandler->>Validator: Validate struct (with tags)
Validator->>CustomValidator: Invoke custom validation (matriculationNumber, tumid)
CustomValidator-->>Validator: Return validation result
Validator-->>GinHandler: Validation success or error
GinHandler-->>Client: Respond with success or validation error
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
utils/validator.go (3)
44-68: Add documentation for TUMIDValidatorThe TUMIDValidator function lacks proper documentation explaining its purpose and the expected format for TUM IDs. This makes it harder for other developers to understand and use the validation correctly.
+// TUMIDValidator checks if a string matches the TUM ID format: +// - exactly 7 characters +// - first two characters are lowercase letters +// - followed by two digits +// - ending with three lowercase letters func TUMIDValidator(fl validator.FieldLevel) bool {
25-42: Consider using unicode package for character validationThe direct character comparisons work correctly but could be replaced with more readable alternatives from the
unicodepackage.// MatriculationNumberValidator checks if a string is exactly 8 digits and starts with 0 func MatriculationNumberValidator(fl validator.FieldLevel) bool { matriculationNumber := fl.Field().String() // Must be exactly 8 characters, start with '0', and all digits if len(matriculationNumber) != 8 { return false } if matriculationNumber[0] != '0' { return false } + // Using a more idiomatic approach with unicode package + for _, r := range matriculationNumber { + if !unicode.IsDigit(r) { + return false + } + } - for _, r := range matriculationNumber { - if r < '0' || r > '9' { - return false - } - } return true }Remember to add
"unicode"to the imports.
1-23: Add usage example for the validator packageThe validator package is well-implemented, but lacks documentation on how to use it in other parts of the codebase. Consider adding a package-level comment with usage examples.
package utils + +// Package utils provides validation utilities for the Prompt SDK. +// +// To use the custom validators in your structs: +// type Student struct { +// MatriculationNumber string `binding:"required,matriculation"` +// UniversityLogin string `binding:"required,tumid"` +// } +// +// To validate a struct programmatically: +// student := Student{...} +// if err := utils.ValidateStruct(student); err != nil { +// // Handle validation error +// } import (🧰 Tools
🪛 golangci-lint (1.64.8)
15-15: Error return value of
v.RegisterValidationis not checked(errcheck)
16-16: Error return value of
v.RegisterValidationis not checked(errcheck)
🪛 GitHub Check: lint-go-code
[failure] 16-16:
Error return value ofv.RegisterValidationis not checked (errcheck)
[failure] 15-15:
Error return value ofv.RegisterValidationis not checked (errcheck)🪛 GitHub Actions: Lint SDK
[error] 15-15: golangci-lint: Error return value of
v.RegisterValidationis not checked (errcheck)promptTypes/student.go (1)
8-21: Consider validating remaining fields for completenessWhile you've added validation to several critical fields, others like FirstName, LastName, Nationality, and StudyProgram remain without validation. Consider whether these fields should also have validation requirements (e.g., required fields, minimum/maximum length).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
auth.go(1 hunks)go.mod(1 hunks)promptTypes/student.go(1 hunks)resolution.go(1 hunks)resolution_test.go(1 hunks)utils/validator.go(1 hunks)utils/validator_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
auth.go (1)
keycloakTokenVerifier/roles.go (5)
PromptAdmin(3-3)PromptLecturer(4-4)CourseLecturer(5-5)CourseEditor(6-6)CourseStudent(7-7)
resolution.go (2)
promptTypes/coursePhaseParticipation.go (1)
CoursePhaseParticipationWithStudent(5-13)promptTypes/metaData.go (1)
MetaData(3-3)
utils/validator_test.go (1)
utils/validator.go (2)
MatriculationNumberValidator(26-42)TUMIDValidator(44-68)
promptTypes/student.go (2)
promptTypes/gender.go (1)
Gender(3-3)promptTypes/studyDegree.go (1)
StudyDegree(3-3)
resolution_test.go (2)
resolution.go (2)
Resolution(13-18)CoursePhaseParticipationsWithResolutions(20-23)promptTypes/coursePhaseParticipation.go (1)
CoursePhaseParticipationWithStudent(5-13)
🪛 golangci-lint (1.64.8)
utils/validator.go
15-15: Error return value of v.RegisterValidation is not checked
(errcheck)
16-16: Error return value of v.RegisterValidation is not checked
(errcheck)
🪛 GitHub Check: lint-go-code
utils/validator.go
[failure] 16-16:
Error return value of v.RegisterValidation is not checked (errcheck)
[failure] 15-15:
Error return value of v.RegisterValidation is not checked (errcheck)
🪛 GitHub Actions: Lint SDK
utils/validator.go
[error] 15-15: golangci-lint: Error return value of v.RegisterValidation is not checked (errcheck)
🔇 Additional comments (18)
auth.go (1)
8-15: Improved organization of role constantsThe consolidation of individual constants into a single const block improves code organization and readability while maintaining the same functionality. This change aligns well with Go style conventions.
go.mod (3)
8-8: Validator dependency now directly referencedPromoting
github.com/go-playground/validator/v10from indirect to direct dependency correctly reflects its explicit usage in the custom validator implementation.
10-10: Database driver dependency now directly referencedMoving
github.com/jackc/pgx/v5to a direct dependency ensures clearer dependency management based on its explicit usage in the codebase.
12-12: Testing dependency now directly referencedAdding
github.com/stretchr/testifyas a direct dependency appropriately reflects its usage in the new validation tests.resolution.go (3)
13-18: Appropriate validation tags for Resolution structThe added validation tags enforce important constraints:
- Required fields ensure minimal data integrity
- URL validation for BaseURL ensures proper formatting
- UUID validation for CoursePhaseID prevents invalid IDs
This change strengthens the API contract and prevents invalid data from being processed.
22-22: Validation for nested Resolution objectsThe
binding:"dive"tag on the Resolutions field ensures that each Resolution in the slice is individually validated against its field constraints. This prevents invalid objects in collections and improves data integrity.
27-27: Consistent validation approachApplying the same
binding:"dive"tag to PrevCoursePhaseData.Resolutions ensures consistency across the codebase, with all Resolution collections receiving the same validation treatment.promptTypes/student.go (6)
9-9: LGTM - Added UUID validation to ID fieldThe addition of the
binding:"uuid"tag to the ID field enforces that only valid UUIDs are accepted, which strengthens input validation for this critical identifier field.
12-12: LGTM - Added email validationAdding the
binding:"email"tag to the Email field ensures the input follows standard email format, which is a good security practice for validating user inputs.
13-13: LGTM - Added custom matriculation number validationThe
binding:"matriculationNumber"tag enables custom validation for the matriculation number field, ensuring it follows the required format (8 digits starting with '0') as implemented in the custom validator.
14-14: LGTM - Added custom TUM ID validationThe
binding:"tumid"tag enables custom validation for the university login field, ensuring it follows the TUM ID format (2 lowercase letters + 2 digits + 3 lowercase letters) as implemented in the custom validator.
16-16: LGTM - Added gender value validationThe
binding:"oneof=male female diverse prefer_not_to_say"tag restricts the Gender field to one of four acceptable values, preventing invalid data entry.
18-18: LGTM - Added study degree validationThe
binding:"oneof=bachelor master"tag restricts the StudyDegree field to either "bachelor" or "master", ensuring data consistency.utils/validator_test.go (2)
44-76: LGTM - Comprehensive tests for matriculation number validationGood test coverage for the MatriculationNumberValidator with various test cases including valid number, too short, too long, incorrect format, and empty string. The parallel test execution is also a good practice for test performance.
78-114: LGTM - Comprehensive tests for TUM ID validationExcellent test coverage for the TUMIDValidator with a wide range of test cases covering the specific format requirements (2 lowercase letters + 2 digits + 3 lowercase letters). Cases handle various format violations, length issues, and empty string scenarios.
resolution_test.go (3)
13-153: LGTM - Well-structured validation test for Resolution objectsThis test thoroughly validates the Resolution struct's validation rules with good coverage of:
- Valid resolution objects
- Missing required fields
- Invalid URL format
- Invalid UUID
- Multiple invalid fields
- Multiple resolutions with some invalid
The tests are well-organized with clear test case descriptions and appropriate assertions.
155-171: LGTM - Good edge case testing for empty resolutionsTesting the validation behavior with an empty slice is important to verify that the
divevalidator works correctly. This ensures that empty collections are handled properly in the validation logic.
173-189: LGTM - Good edge case testing for nil resolutionsTesting with nil slice is another important edge case to verify that the
divevalidator correctly handles nil collections. This helps prevent nil pointer exceptions in the validation logic.
… for MatriculationNumber and TUMID
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utils/validator.go (3)
12-12: Update the import path in documentation example.The import path example uses a placeholder organization name
"github.com/your-org/Prompt-SDK/utils". This should be updated to reference the actual organization that hosts the repository.-// import "github.com/your-org/Prompt-SDK/utils" +// import "github.com/ls1intum/Prompt-SDK/utils"
117-141: Consider using unicode package functions for character validation in TUMIDValidator.While the current implementation using ASCII range checks works for the specified rules, using the unicode package functions (which is already imported) might be more maintainable, especially if validation rules change in the future.
func TUMIDValidator(fl validator.FieldLevel) bool { tumID := fl.Field().String() if len(tumID) != 7 { return false } for i := 0; i < 2; i++ { // first two letters - if tumID[i] < 'a' || tumID[i] > 'z' { + if !unicode.IsLower(rune(tumID[i])) { return false } } for i := 2; i < 4; i++ { // two digits - if tumID[i] < '0' || tumID[i] > '9' { + if !unicode.IsDigit(rune(tumID[i])) { return false } } for i := 4; i < 7; i++ { // last three letters - if tumID[i] < 'a' || tumID[i] > 'z' { + if !unicode.IsLower(rune(tumID[i])) { return false } } return true }
45-74: Consider adding helper functions that return descriptive error messages.While the current validators return boolean values (which is standard for the validator package), it might be helpful to provide additional helper functions that return descriptive error messages for validation failures. This would aid in debugging and error reporting.
For example, you could add:
// ValidateMatriculationNumber validates a matriculation number and returns an error with a descriptive message if invalid func ValidateMatriculationNumber(matriculationNumber string) error { if len(matriculationNumber) != 8 { return fmt.Errorf("matriculation number must be exactly 8 characters long") } if matriculationNumber[0] != '0' { return fmt.Errorf("matriculation number must start with '0'") } for i, r := range matriculationNumber { if !unicode.IsDigit(r) { return fmt.Errorf("invalid character at position %d: must be a digit", i) } } return nil } // ValidateTUMID validates a TUM ID and returns an error with a descriptive message if invalid func ValidateTUMID(tumID string) error { if len(tumID) != 7 { return fmt.Errorf("TUM ID must be exactly 7 characters long") } // Validate first two characters (lowercase letters) for i := 0; i < 2; i++ { if !unicode.IsLower(rune(tumID[i])) { return fmt.Errorf("character at position %d must be a lowercase letter", i) } } // Validate next two characters (digits) for i := 2; i < 4; i++ { if !unicode.IsDigit(rune(tumID[i])) { return fmt.Errorf("character at position %d must be a digit", i) } } // Validate last three characters (lowercase letters) for i := 4; i < 7; i++ { if !unicode.IsLower(rune(tumID[i])) { return fmt.Errorf("character at position %d must be a lowercase letter", i) } } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resolution.go(1 hunks)resolution_test.go(2 hunks)utils/validator.go(1 hunks)utils/validator_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- resolution.go
- utils/validator_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
resolution_test.go (2)
resolution.go (2)
Resolution(14-19)CoursePhaseParticipationsWithResolutions(21-24)promptTypes/coursePhaseParticipation.go (1)
CoursePhaseParticipationWithStudent(5-13)
🔇 Additional comments (11)
resolution_test.go (6)
6-10: Good import organization and package selection.The imports correctly add the required validation packages (gin binding and go-playground validator) while also importing the necessary promptTypes package. This demonstrates good dependency management by explicitly importing the validation tools needed for the tests.
13-22: Well-structured test setup with proper validator engine initialization.The test function correctly retrieves the validator engine from Gin binding and verifies it's the expected type before proceeding. The UUID initialization for test data follows best practices by using a static, known UUID value for consistent testing.
23-126: Comprehensive test cases covering all validation scenarios.The test cases thoroughly cover all validation rules defined in the
Resolutionstruct:
- Required fields (DtoName, EndpointPath)
- URL format validation for BaseURL
- UUID validation for CoursePhaseID
- Multiple error scenarios
- Multi-resolution validation
This provides excellent coverage of the validation rules and ensures the validation tags are working as expected.
128-153: Test execution follows best practices with parallel test execution and clear assertions.The implementation uses
t.Parallel()for efficient test execution and provides clear error message checks when validation errors are expected. The test structure clearly separates test case definition from execution logic.
178-194: Good edge case handling for empty resolution slices.Testing the validation behavior with an empty slice of resolutions is important to verify that the
divetag only applies to elements within the slice, not to the slice itself. This test ensures that an application can handle the case where no resolutions are provided.
196-212: Complete validation testing with nil slice handling.Testing with a nil slice complements the empty slice test and provides complete coverage of possible input scenarios. This confirms that the validation logic handles both empty and nil slices correctly, which is important for robust API behavior.
utils/validator.go (5)
52-58: Proper error handling implementation for validator registration.This code correctly checks for errors when registering validators and provides informative panic messages. This implementation addresses the issue identified in previous reviews.
61-68: Good implementation of validator registration with Gin.The code properly registers the custom validators with Gin's validator engine, ensuring they work seamlessly with Gin's binding system. The error handling is appropriate, with clear panic messages if registration fails.
71-74: The ValidateStruct function provides a simple interface for validation.This wrapper function is a good practice as it encapsulates the validation logic and provides a clean API for struct validation.
76-99: MatriculationNumberValidator implementation is clear and effective.The validation logic and documentation clearly describe the expected format for matriculation numbers. The implementation is efficient, checking for length first before performing character-by-character validation.
1-36: Well-documented package with clear usage examples.The package documentation is excellent, providing comprehensive information about the validators and their usage with both struct tags and the Gin framework. The examples are clear and cover both direct validation and integration with Gin.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
utils/validator.go (4)
12-12: Update import path in documentation exampleThe import path in the example code uses a placeholder organization name. Consider updating it to match the actual repository structure:
-import "github.com/your-org/Prompt-SDK/utils" +import "github.com/ls1intum/Prompt-SDK/utils"
116-132: Function name doesn't match tag nameThere's a naming inconsistency between the function name
TUMIDValidatorand the tag nameuniversityLogin. Consider aligning these for better code clarity:-func TUMIDValidator(fl validator.FieldLevel) bool { +func UniversityLoginValidator(fl validator.FieldLevel) bool {And update the registration accordingly:
-if err := validate.RegisterValidation("universityLogin", TUMIDValidator); err != nil { +if err := validate.RegisterValidation("universityLogin", UniversityLoginValidator); err != nil {Or alternatively, use a consistent tag name:
-if err := validate.RegisterValidation("universityLogin", TUMIDValidator); err != nil { +if err := validate.RegisterValidation("tumid", TUMIDValidator); err != nil {
139-153: Use consistent validation approachThe validation logic uses different approaches for different parts of the string:
unicode.IsLower()for the first two characters- Direct comparison for digits and the last three characters
Consider using a consistent approach throughout for better maintainability:
for i := 0; i < 2; i++ { // first two letters if !unicode.IsLower(rune(tumID[i])) { return false } } for i := 2; i < 4; i++ { // two digits - if tumID[i] < '0' || tumID[i] > '9' { + if !unicode.IsDigit(rune(tumID[i])) { return false } } for i := 4; i < 7; i++ { // last three letters - if tumID[i] < 'a' || tumID[i] > 'z' { + if !unicode.IsLower(rune(tumID[i])) { return false } }
87-89: Consider enhancing error information for easier debuggingThe
ValidateStructfunction returns errors directly from the validator. For better debugging, consider wrapping the error with additional context:func ValidateStruct(s interface{}) error { - return validate.Struct(s) + if err := validate.Struct(s); err != nil { + return fmt.Errorf("validation failed for %T: %w", s, err) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
promptTypes/student.go(1 hunks)utils/validator.go(1 hunks)utils/validator_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/validator_test.go
- promptTypes/student.go
🔇 Additional comments (3)
utils/validator.go (3)
58-80: Well-implemented initialization with proper error handlingThe validator initialization is thorough, registering custom validators with both the local instance and Gin's validator engine with appropriate error handling. This implementation properly addresses the previously identified issue with error checking.
1-47: Well-documented package with clear usage examplesThe package documentation is comprehensive and includes clear examples for different use cases. This greatly enhances usability for other developers.
91-114: Well-implemented MatriculationNumberValidatorThe validator implementation is clear, properly documented, and includes appropriate validation checks for the matriculation number format.
| // - Contain only numeric digits (0-9) | ||
| // | ||
| // This function is designed to be used as a custom validator with the validator package. | ||
| func MatriculationNumberValidator(fl validator.FieldLevel) bool { |
There was a problem hiding this comment.
as already discussed in person, I'm not sure if I like the idea of integrating TUM specific validators directly into the library.
@Mtze What is your take on it?
There was a problem hiding this comment.
Other universities can easily overwrite the validators with their own ones. We could extend this by allow them to be automatically injected using Golang Plugins?
This pull request introduces several enhancements and updates across the codebase, focusing on validation improvements, restructuring of constants, and addition of tests for better code reliability
For more details on validators see the following links:
Summary by CodeRabbit