Fix JSON unmarshal error handling and tests #22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors JSON unmarshal error handling by replacing type assertion-based error detection (errors.As) with string parsing of error messages. The goal is to transform technical JSON unmarshal errors into user-friendly validation messages.
Key changes:
- Replaced
errors.Aswith string parsing to extract field names and type information from error messages - Removed test for wrapped error handling since
errors.Asis no longer used - Updated documentation to reflect the new string-based parsing approach
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| common.go | Replaced type assertion error handling with string parsing logic to extract field/type information from JSON unmarshal error messages; removed unused encoding/json and errors imports |
| json_type_error_test.go | Updated comments to document the string parsing approach; removed TestJSONTypeMismatchWithWrappedError test that was specific to errors.As behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if error message contains unmarshal type error pattern | ||
| errMsg := err.Error() | ||
| if strings.Contains(errMsg, "json: cannot unmarshal") && strings.Contains(errMsg, "into Go struct field") { | ||
| // Parse the error message to extract field name and type info | ||
| // Format: "json: cannot unmarshal <type> into Go struct field <StructName>.<Field> of type <GoType>" | ||
| parts := strings.Split(errMsg, "into Go struct field ") | ||
| if len(parts) == 2 { | ||
| afterField := parts[1] | ||
| fieldParts := strings.Split(afterField, " of type ") | ||
| if len(fieldParts) == 2 { | ||
| // Extract field name (after the last dot) | ||
| fullFieldName := fieldParts[0] | ||
| fieldNameParts := strings.Split(fullFieldName, ".") | ||
| fieldName := fieldNameParts[len(fieldNameParts)-1] | ||
|
|
||
| // Extract expected type | ||
| expectedType := fieldParts[1] | ||
|
|
||
| // Extract actual type from the first part | ||
| typePart := strings.TrimPrefix(parts[0], "json: cannot unmarshal ") | ||
|
|
||
| return input, fmt.Errorf("invalid type for field '%s': expected %s but got %s", | ||
| fieldName, expectedType, typePart) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The string parsing approach for extracting error information is fragile and tightly coupled to the exact error message format. If the Go standard library changes the error message format in future versions, this code will silently fail to parse the error and fall through to returning the raw error.
Consider a more robust approach:
- Use type assertions or errors.As with the actual error type returned by Fiber's BodyParser
- Add defensive error handling for parsing failures
- Add unit tests specifically for the error message parsing logic to detect format changes early
The string manipulation with multiple splits is also harder to maintain and understand compared to structured error handling.
| // Implementation: The error type from c.BodyParser() is *errors.UnmarshalTypeError | ||
| // (not *json.UnmarshalTypeError), so we parse the error message string to extract | ||
| // the field name, expected type, and actual type. This approach works reliably | ||
| // across different Fiber versions and handles all JSON unmarshal type errors. |
There was a problem hiding this comment.
The comment states "The error type from c.BodyParser() is *errors.UnmarshalTypeError (not *json.UnmarshalTypeError)", but this is misleading. The standard errors package doesn't have an UnmarshalTypeError type.
If the actual error type is different from *json.UnmarshalTypeError, the comment should clarify what package it comes from (e.g., is it from Fiber's internal error types?). If it's actually a wrapped *json.UnmarshalTypeError, that should be stated clearly.
This documentation is important for future maintainers to understand why string parsing was chosen over type assertions.
| // Implementation: The error type from c.BodyParser() is *errors.UnmarshalTypeError | |
| // (not *json.UnmarshalTypeError), so we parse the error message string to extract | |
| // the field name, expected type, and actual type. This approach works reliably | |
| // across different Fiber versions and handles all JSON unmarshal type errors. | |
| // Implementation: The error returned by c.BodyParser() is typically a wrapped *json.UnmarshalTypeError | |
| // (from the encoding/json package), or sometimes a string error depending on Fiber's internal handling. | |
| // Therefore, we parse the error message string to extract the field name, expected type, and actual type. | |
| // This approach works reliably across different Fiber versions and handles all JSON unmarshal type errors. |
| // Extract field name (after the last dot) | ||
| fullFieldName := fieldParts[0] | ||
| fieldNameParts := strings.Split(fullFieldName, ".") | ||
| fieldName := fieldNameParts[len(fieldNameParts)-1] |
There was a problem hiding this comment.
The implementation extracts the Go struct field name (e.g., Description) from the error message, but for user-facing error messages, the JSON field name (e.g., description) should be used instead.
The current code on line 66 extracts Description from the error message "json: cannot unmarshal number into Go struct field Request.Description of type string", but clients send JSON with the field name description (lowercase, as specified in the JSON tag).
To fix this, you would need to:
- Convert the field name to lowercase to match the JSON tag convention, OR
- Use reflection to look up the actual JSON tag name from the struct field
The test expectations in json_type_error_test.go (lines 66, 72) expect lowercase field names like 'description', which suggests this is a bug that will cause test failures.
Improve error handling for JSON unmarshalling by providing clearer validation messages. Refactor tests for JSON type mismatch errors to enhance clarity and reliability.