fix: support []interface{} in GetStringSliceValue and GetIntSliceValue for YAML arrays#351
fix: support []interface{} in GetStringSliceValue and GetIntSliceValue for YAML arrays#351zy84338719 wants to merge 2 commits intoapolloconfig:masterfrom
Conversation
This fixes issue apolloconfig#348 where YAML array fields were parsed as []interface{} but GetStringSliceValue/GetIntSliceValue methods only handled []string/[]int. Changes: - Extended GetStringSliceValue to convert []interface{} to []string - Extended GetStringSliceValueImmediately to convert []interface{} to []string - Extended GetIntSliceValue to convert []interface{} to []int (handles float64 from YAML) - Extended GetIntSliceValueImmediately to convert []interface{} to []int - Added test cases for YAML array parsing in parser_test.go - Added test cases for slice conversion in repository_test.go
📝 WalkthroughWalkthroughEnhances repository getters to accept and convert additional slice input shapes (including []interface{}, []string, []int, and delimited strings), adds robust conversion and logging on failures, and adds tests validating YAML/JSON array parsing and conversion behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
There was a problem hiding this comment.
Pull request overview
This PR fixes YAML array handling by extending the Config slice accessors to properly convert Viper-parsed YAML arrays ([]interface{}) into []string / []int, preventing stringified array outputs and conversion failures for YAML-backed namespaces.
Changes:
- Extend
GetStringSliceValue/GetStringSliceValueImmediatelyto convert[]interface{}to[]string. - Extend
GetIntSliceValue/GetIntSliceValueImmediatelyto convert[]interface{}(including numeric/string elements) to[]int. - Add unit tests covering YAML array parsing and the new slice conversion behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| storage/repository.go | Adds []interface{} handling for string/int slice getters. |
| storage/repository_test.go | Adds tests validating conversion from []interface{} to []string / []int. |
| utils/parse/yaml/parser_test.go | Adds a test verifying YAML arrays are parsed as []interface{} via Viper. |
| utils/parse/yml/parser_test.go | Same as above for .yml parser variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case float64: | ||
| // JSON/YAML 数字默认解析为 float64 | ||
| result = append(result, int(v)) | ||
| case string: |
There was a problem hiding this comment.
When handling float64 elements in a YAML/JSON-derived []interface{}, the code casts via int(v), which silently truncates fractional values (e.g., 1.9 -> 1) and can also produce implementation-dependent results on overflow. Consider validating that v is an integer value (no fractional part) and within int range; otherwise return defaultValue (and optionally log). Adding a test for a non-integer float element would lock in the intended behavior.
| case float64: | ||
| // JSON/YAML 数字默认解析为 float64 | ||
| result = append(result, int(v)) | ||
| case string: |
There was a problem hiding this comment.
The float64 -> int conversion uses int(v) without checking for fractional values or overflow, so values like 1.9 will be truncated to 1. It would be safer to reject non-integer floats (and out-of-range floats) and fall back to defaultValue to avoid silently returning incorrect configuration.
| // 尝试直接转换为 []string | ||
| if v, ok := value.([]string); ok { | ||
| return v | ||
| } | ||
| return v | ||
|
|
||
| // 尝试从 []interface{} 转换为 []string (YAML 数组解析后的类型) | ||
| if ifaceSlice, ok := value.([]interface{}); ok { | ||
| result := make([]string, 0, len(ifaceSlice)) | ||
| for _, item := range ifaceSlice { | ||
| if s, ok := item.(string); ok { | ||
| result = append(result, s) | ||
| } else { | ||
| result = append(result, fmt.Sprintf("%v", item)) | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The []interface{} -> []string conversion logic here is duplicated in both the immediate and non-immediate variants (and similarly for []int). Consider extracting a small private helper to keep behavior consistent and reduce maintenance overhead if conversion rules change again.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storage/repository.go`:
- Around line 208-223: The float64 branch in the switch inside
GetIntSliceValueImmediately and GetIntSliceValue currently casts float64 to int
directly, which silently truncates fractional values; modify both functions so
the float64 case first validates the value is integral (e.g., compare v to
math.Trunc(v) or check v == float64(int(v))) and if it is not integral log the
conversion failure and return defaultValue, otherwise cast to int and append to
result; update the log message references in the float64 branch to match the
existing debug style used for other failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 805c0617-16cc-470f-a4f5-509c8a9c3d3b
📒 Files selected for processing (4)
storage/repository.gostorage/repository_test.goutils/parse/yaml/parser_test.goutils/parse/yml/parser_test.go
This addresses the CodeRabbit review comment: - float64 to int conversion previously silently truncated fractional values (e.g., 1.5 -> 1) - Now validates that float64 values have no fractional part before conversion - Returns defaultValue and logs a debug message if fractional part exists - Added test cases for fractional number scenarios
There was a problem hiding this comment.
🧹 Nitpick comments (1)
storage/repository.go (1)
199-235: Extract the[]interface{}→[]intcoercion into a shared helper.The conversion rules and failure handling are duplicated in both getters. Pulling this into one helper would reduce drift and make follow-up fixes much safer.
Also applies to: 400-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/repository.go` around lines 199 - 235, The []interface{}→[]int conversion logic is duplicated; extract it into a shared helper (e.g., convertInterfaceSliceToIntSlice or parseIntSliceFromAny) that accepts the input value and a default []int and returns the converted []int, preserving current rules (handle int, float64 only if integer, string via strconv.Atoi, log debug on failures and return defaultValue). Replace the inlined loops in the current getter(s) (the code block that checks value.([]int) and value.([]interface{})) and the other similar getter (also referenced in the comment range 400-449) to call this helper, keeping the initial fast-path []int check in the getters if desired but delegating []interface{} handling to the new function. Ensure the helper uses the same log messages and returns defaultValue on any conversion error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@storage/repository.go`:
- Around line 199-235: The []interface{}→[]int conversion logic is duplicated;
extract it into a shared helper (e.g., convertInterfaceSliceToIntSlice or
parseIntSliceFromAny) that accepts the input value and a default []int and
returns the converted []int, preserving current rules (handle int, float64 only
if integer, string via strconv.Atoi, log debug on failures and return
defaultValue). Replace the inlined loops in the current getter(s) (the code
block that checks value.([]int) and value.([]interface{})) and the other similar
getter (also referenced in the comment range 400-449) to call this helper,
keeping the initial fast-path []int check in the getters if desired but
delegating []interface{} handling to the new function. Ensure the helper uses
the same log messages and returns defaultValue on any conversion error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2794ef0-70cd-419e-96ea-d4f14371b9f5
📒 Files selected for processing (2)
storage/repository.gostorage/repository_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/repository_test.go
Problem
Fixes #348
When parsing YAML namespace configurations, array fields are stored as
[]interface{}type (this is how Viper parses YAML arrays). However,GetStringSliceValueandGetIntSliceValuemethods only handled[]stringand[]inttypes directly, causing YAML arrays to be returned as stringified format like"[test111 test222]"when usingGetContent(), or failing to convert when using slice accessor methods.Solution
Extended the slice accessor methods to properly handle
[]interface{}type:GetStringSliceValue/GetStringSliceValueImmediately[]interface{}to[]stringfmt.Sprintf("%v", item)GetIntSliceValue/GetIntSliceValueImmediately[]interface{}to[]intfloat64type (YAML/JSON default for numbers)strconv.AtoiChanges
storage/repository.go
GetStringSliceValueto handle[]interface{}GetStringSliceValueImmediatelyto handle[]interface{}GetIntSliceValueto handle[]interface{}GetIntSliceValueImmediatelyto handle[]interface{}storage/repository_test.go
TestGetStringSliceValueFromInterfaceSlicetestTestGetIntSliceValueFromInterfaceSlicetestutils/parse/yaml/parser_test.go & utils/parse/yml/parser_test.go
TestYAMLParserArray/TestYMLParserArraytestUsage Example
Testing
All existing tests pass, new tests added for the YAML array scenario.
Summary by CodeRabbit