-
Notifications
You must be signed in to change notification settings - Fork 212
peers: add filters.setParams support #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ee5bec3
peers: add filters.setParams support
claude 3dd0245
peers: add filters.stripParams support
claude 5c0ad5c
models: add filters.setParams support
claude a0b02d2
config: consolidate filters into shared type
claude 5fb6d92
fix: use nested struct syntax for embedded Filters in test
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "slices" | ||
| "sort" | ||
| "strings" | ||
| ) | ||
|
|
||
| // ProtectedParams is a list of parameters that cannot be set or stripped via filters | ||
| // These are protected to prevent breaking the proxy's ability to route requests correctly | ||
| var ProtectedParams = []string{"model"} | ||
|
|
||
| // Filters contains filter settings for modifying request parameters | ||
| // Used by both models and peers | ||
| type Filters struct { | ||
| // StripParams is a comma-separated list of parameters to remove from requests | ||
| // The "model" parameter can never be removed | ||
| StripParams string `yaml:"stripParams"` | ||
|
|
||
| // SetParams is a dictionary of parameters to set/override in requests | ||
| // Protected params (like "model") cannot be set | ||
| SetParams map[string]any `yaml:"setParams"` | ||
| } | ||
|
|
||
| // SanitizedStripParams returns a sorted list of parameters to strip, | ||
| // with duplicates, empty strings, and protected params removed | ||
| func (f Filters) SanitizedStripParams() []string { | ||
| if f.StripParams == "" { | ||
| return nil | ||
| } | ||
|
|
||
| params := strings.Split(f.StripParams, ",") | ||
| cleaned := make([]string, 0, len(params)) | ||
| seen := make(map[string]bool) | ||
|
|
||
| for _, param := range params { | ||
| trimmed := strings.TrimSpace(param) | ||
| // Skip protected params, empty strings, and duplicates | ||
| if slices.Contains(ProtectedParams, trimmed) || trimmed == "" || seen[trimmed] { | ||
| continue | ||
| } | ||
| seen[trimmed] = true | ||
| cleaned = append(cleaned, trimmed) | ||
| } | ||
|
|
||
| if len(cleaned) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| slices.Sort(cleaned) | ||
| return cleaned | ||
| } | ||
|
|
||
| // SanitizedSetParams returns a copy of SetParams with protected params removed | ||
| // and keys sorted for consistent iteration order | ||
| func (f Filters) SanitizedSetParams() (map[string]any, []string) { | ||
| if len(f.SetParams) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| result := make(map[string]any, len(f.SetParams)) | ||
| keys := make([]string, 0, len(f.SetParams)) | ||
|
|
||
| for key, value := range f.SetParams { | ||
| // Skip protected params | ||
| if slices.Contains(ProtectedParams, key) { | ||
| continue | ||
| } | ||
| result[key] = value | ||
| keys = append(keys, key) | ||
| } | ||
|
|
||
| // Sort keys for consistent ordering | ||
| sort.Strings(keys) | ||
|
|
||
| if len(result) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| return result, keys | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestFilters_SanitizedStripParams(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| stripParams string | ||
| want []string | ||
| }{ | ||
| { | ||
| name: "empty string", | ||
| stripParams: "", | ||
| want: nil, | ||
| }, | ||
| { | ||
| name: "single param", | ||
| stripParams: "temperature", | ||
| want: []string{"temperature"}, | ||
| }, | ||
| { | ||
| name: "multiple params", | ||
| stripParams: "temperature, top_p, top_k", | ||
| want: []string{"temperature", "top_k", "top_p"}, // sorted | ||
| }, | ||
| { | ||
| name: "model param filtered", | ||
| stripParams: "model, temperature, top_p", | ||
| want: []string{"temperature", "top_p"}, | ||
| }, | ||
| { | ||
| name: "only model param", | ||
| stripParams: "model", | ||
| want: nil, | ||
| }, | ||
| { | ||
| name: "duplicates removed", | ||
| stripParams: "temperature, top_p, temperature", | ||
| want: []string{"temperature", "top_p"}, | ||
| }, | ||
| { | ||
| name: "extra whitespace", | ||
| stripParams: " temperature , top_p ", | ||
| want: []string{"temperature", "top_p"}, | ||
| }, | ||
| { | ||
| name: "empty values filtered", | ||
| stripParams: "temperature,,top_p,", | ||
| want: []string{"temperature", "top_p"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| f := Filters{StripParams: tt.stripParams} | ||
| got := f.SanitizedStripParams() | ||
| assert.Equal(t, tt.want, got) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestFilters_SanitizedSetParams(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| setParams map[string]any | ||
| wantParams map[string]any | ||
| wantKeys []string | ||
| }{ | ||
| { | ||
| name: "empty setParams", | ||
| setParams: nil, | ||
| wantParams: nil, | ||
| wantKeys: nil, | ||
| }, | ||
| { | ||
| name: "empty map", | ||
| setParams: map[string]any{}, | ||
| wantParams: nil, | ||
| wantKeys: nil, | ||
| }, | ||
| { | ||
| name: "normal params", | ||
| setParams: map[string]any{ | ||
| "temperature": 0.7, | ||
| "top_p": 0.9, | ||
| }, | ||
| wantParams: map[string]any{ | ||
| "temperature": 0.7, | ||
| "top_p": 0.9, | ||
| }, | ||
| wantKeys: []string{"temperature", "top_p"}, | ||
| }, | ||
| { | ||
| name: "protected model param filtered", | ||
| setParams: map[string]any{ | ||
| "model": "should-be-filtered", | ||
| "temperature": 0.7, | ||
| }, | ||
| wantParams: map[string]any{ | ||
| "temperature": 0.7, | ||
| }, | ||
| wantKeys: []string{"temperature"}, | ||
| }, | ||
| { | ||
| name: "only protected param", | ||
| setParams: map[string]any{ | ||
| "model": "should-be-filtered", | ||
| }, | ||
| wantParams: nil, | ||
| wantKeys: nil, | ||
| }, | ||
| { | ||
| name: "complex nested values", | ||
| setParams: map[string]any{ | ||
| "provider": map[string]any{ | ||
| "data_collection": "deny", | ||
| "allow_fallbacks": false, | ||
| }, | ||
| "transforms": []string{"middle-out"}, | ||
| }, | ||
| wantParams: map[string]any{ | ||
| "provider": map[string]any{ | ||
| "data_collection": "deny", | ||
| "allow_fallbacks": false, | ||
| }, | ||
| "transforms": []string{"middle-out"}, | ||
| }, | ||
| wantKeys: []string{"provider", "transforms"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| f := Filters{SetParams: tt.setParams} | ||
| gotParams, gotKeys := f.SanitizedSetParams() | ||
|
|
||
| assert.Equal(t, len(tt.wantKeys), len(gotKeys), "keys length mismatch") | ||
| for i, key := range gotKeys { | ||
| assert.Equal(t, tt.wantKeys[i], key, "key mismatch at %d", i) | ||
| } | ||
|
|
||
| if tt.wantParams == nil { | ||
| assert.Nil(t, gotParams, "expected nil params") | ||
| return | ||
| } | ||
|
|
||
| assert.Equal(t, len(tt.wantParams), len(gotParams), "params length mismatch") | ||
| for key, wantValue := range tt.wantParams { | ||
| gotValue, exists := gotParams[key] | ||
| assert.True(t, exists, "missing key: %s", key) | ||
| // Simple comparison for basic types | ||
| switch v := wantValue.(type) { | ||
| case string, int, float64, bool: | ||
| assert.Equal(t, v, gotValue, "value mismatch for key %s", key) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestProtectedParams(t *testing.T) { | ||
| // Verify that "model" is protected | ||
| assert.Contains(t, ProtectedParams, "model") | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complex nested values are not fully asserted.
The "complex nested values" test case at lines 117-133 includes a nested map and slice, but the assertion logic at lines 156-159 only compares basic types (
string,int,float64,bool). The nestedprovidermap andtransformsslice are not actually verified for correctness.Proposed fix to assert nested values
for key, wantValue := range tt.wantParams { gotValue, exists := gotParams[key] assert.True(t, exists, "missing key: %s", key) - // Simple comparison for basic types - switch v := wantValue.(type) { - case string, int, float64, bool: - assert.Equal(t, v, gotValue, "value mismatch for key %s", key) - } + assert.Equal(t, wantValue, gotValue, "value mismatch for key %s", key) }Also applies to: 151-160