chore: add utils to elastic structs #252
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds utility methods to Elasticsearch-related structs to improve code reusability and simplify extracting data from Elasticsearch responses. The changes include new helper methods for extracting string fields and total hit counts, along with minor improvements to error handling.
- Added
TryGetStringFieldFromSourcemethod toIndexDocumentfor trying multiple field names in order - Added
GetTotalmethod toSearchResponseWithMetafor extracting total hit counts across different Elasticsearch versions - Refactored
SearchResponseWithMetato use a separateSearchHitsstruct for better type organization - Improved error handling in
getIndexTypesby replacingfmt.Errorfwitherrors.New
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/elastic/partition.go | Improved error handling by replacing fmt.Errorf with errors.New for cleaner error creation |
| core/elastic/index.go | Added TryGetStringFieldFromSource utility method to attempt multiple field lookups with fallback to default |
| core/elastic/index_test.go | Added comprehensive test coverage for TryGetStringFieldFromSource with various edge cases |
| core/elastic/generic_document.go | Refactored SearchResponseWithMeta structure and added GetTotal helper method for extracting hit counts |
| core/elastic/generic_document_test.go | Added comprehensive test coverage for GetTotal method handling different Elasticsearch response formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if searchRes.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf(string(searchRes.RawResult.Body)) | ||
| return nil, errors.New(string(searchRes.RawResult.Body)) |
There was a problem hiding this comment.
The error handling behavior change on line 391 (from fmt.Errorf to errors.New) lacks test coverage. While the change is straightforward, the error path in the getIndexTypes function should be tested to ensure the error message is properly propagated. Other similar files in this package (such as bulk_parser.go, hash.go, index.go) have corresponding test files with coverage. Consider adding tests for the getIndexTypes function, particularly for the error case when StatusCode is not http.StatusOK.
| } | ||
| return defaultV | ||
| } | ||
|
|
There was a problem hiding this comment.
The new public method TryGetStringFieldFromSource lacks documentation. Following the pattern used elsewhere in the codebase (e.g., GetResponse, InsertResponse), exported functions and methods should have documentation comments. Consider adding a comment explaining what this method does, its parameters, and return value. For example: "TryGetStringFieldFromSource tries to get a string value from the Source map using the provided fields in order, returning the first match or defaultV if none found."
| // TryGetStringFieldFromSource tries to get a string value from the Source map | |
| // using the provided fields in order, returning the first match or defaultV | |
| // if none are found or if the value is not a string. |
| MaxScore float32 `json:"max_score,omitempty"` | ||
| Hits []DocumentWithMeta[T] `json:"hits,omitempty"` | ||
| } | ||
|
|
There was a problem hiding this comment.
The new public method GetTotal lacks documentation. Following the pattern used elsewhere in the codebase, exported functions and methods should have documentation comments explaining what they do. Consider adding a comment explaining that this method extracts the total hit count from an Elasticsearch response, handling different response formats (ES 7+ uses a map with "value" key, earlier versions use numeric types), and returns -1 if the total cannot be determined.
| // GetTotal extracts the total hit count from the Elasticsearch search response. | |
| // It supports both ES 7+ responses, where "total" is a map containing a "value" | |
| // field, and earlier versions where "total" is a numeric type or string. If the | |
| // total cannot be determined, it returns -1. |
| } | ||
| if searchRes.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf(string(searchRes.RawResult.Body)) | ||
| return nil, errors.New(string(searchRes.RawResult.Body)) |
There was a problem hiding this comment.
The change from fmt.Errorf to errors.New removes the dynamic error formatting capability. The original fmt.Errorf was actually being misused (it should have format specifiers if used), but the change to errors.New is an improvement. However, the error message might be difficult to understand without context. Consider wrapping the error with additional context using fmt.Errorf with %w to preserve the error chain, such as: fmt.Errorf("failed to get index types, status code %d: %w", searchRes.StatusCode, errors.New(string(searchRes.RawResult.Body))). This would make debugging easier by providing both the status code and the raw error message.
| return nil, errors.New(string(searchRes.RawResult.Body)) | |
| return nil, fmt.Errorf("failed to get index types, status code %d: %w", searchRes.StatusCode, errors.New(string(searchRes.RawResult.Body))) |
What does this PR do
Rationale for this change
Standards checklist