Skip to content

Commit

Permalink
Have the filter parser allow exist tests only on labels.
Browse files Browse the repository at this point in the history 
Also reduce the case where there's no namespace function.
  • Loading branch information
@ericpromislow
ericpromislow committed Dec 4, 2024
1 parent fcfa04c commit 697fdee
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 78 deletions.
101 changes: 24 additions & 77 deletions pkg/stores/sqlpartition/listprocessor/processor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func TestParseQuery(t *testing.T) {
req *types.APIRequest
expectedLO informer.ListOptions
errExpected bool
errorText string
}
var tests []testCase
tests = append(tests, testCase{
Expand All @@ -41,9 +42,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If projectsornamespaces is not empty" +
Expand Down Expand Up @@ -233,9 +231,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with filter param set, with value in single quotes, should include filter with partial set to false in list options.",
Expand All @@ -262,9 +257,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with filter param set, with value in double quotes, should include filter with partial set to false in list options.",
Expand All @@ -291,9 +283,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with a labels filter param should create a labels-specific filter.",
Expand All @@ -320,9 +309,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with multiple filter params, should include multiple or filters.",
Expand Down Expand Up @@ -359,9 +345,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with multiple filter params, should include multiple or filters.",
Expand Down Expand Up @@ -398,9 +381,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle comma-separated standard and labels filters.",
Expand Down Expand Up @@ -433,9 +413,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle simple dot-separated label filters.",
Expand Down Expand Up @@ -468,9 +445,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle 'in' and 'IN' with one arg",
Expand Down Expand Up @@ -503,9 +477,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle 'in' with multiple args",
Expand All @@ -532,9 +503,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle 'notin' and 'NOTIN' with one arg",
Expand Down Expand Up @@ -567,9 +535,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle 'in' with multiple args",
Expand All @@ -596,9 +561,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle 'in' and 'notin' in mixed case",
Expand Down Expand Up @@ -631,36 +593,43 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle exists tests",
description: "ParseQuery() should complain on non-label exists tests",
req: &types.APIRequest{
Request: &http.Request{
URL: &url.URL{RawQuery: "filter=a5In1,!a5In2, ! a5In3"},
},
},
errExpected: true,
errorText: "unable to parse requirement: existence tests are valid only for labels; not valid for field 'a5In1'",
})
tests = append(tests, testCase{
description: "ParseQuery() should allow label exists tests",
req: &types.APIRequest{
Request: &http.Request{
URL: &url.URL{RawQuery: "filter=metadata.labels.a5In1,!metadata.labels.a5In2, ! metadata.labels.a5In3"},
},
},
expectedLO: informer.ListOptions{
ChunkSize: defaultLimit,
Filters: []informer.OrFilter{
{
Filters: []informer.Filter{
{
Field: []string{"a5In1"},
Field: []string{"metadata", "labels", "a5In1"},
Op: informer.Exists,
Matches: []string{},
Partial: true,
},
{
Field: []string{"a5In2"},
Field: []string{"metadata", "labels", "a5In2"},
Op: informer.NotExists,
Matches: []string{},
Partial: true,
},
{
Field: []string{"a5In3"},
Field: []string{"metadata", "labels", "a5In3"},
Op: informer.NotExists,
Matches: []string{},
Partial: true,
Expand All @@ -672,9 +641,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() should handle numeric comparisons",
Expand Down Expand Up @@ -707,9 +673,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If one sort param is given, primary field" +
Expand All @@ -729,9 +692,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If one sort param is given primary field " +
Expand All @@ -752,9 +712,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If two sort params are given, sort " +
Expand All @@ -777,9 +734,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If continue params is given, resume" +
Expand All @@ -797,9 +751,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If continue param is given, resume" +
Expand All @@ -817,9 +768,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If limit param is given, chunksize" +
Expand All @@ -836,9 +784,6 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If page param is given, page" +
Expand All @@ -855,9 +800,6 @@ func TestParseQuery(t *testing.T) {
Page: 3,
},
},
setupNSCache: func() Cache {
return nil
},
})
tests = append(tests, testCase{
description: "ParseQuery() with no errors returned should returned no errors. If pagesize param is given, pageSize" +
Expand All @@ -875,21 +817,26 @@ func TestParseQuery(t *testing.T) {
Page: 1,
},
},
setupNSCache: func() Cache {
return nil
},
})
t.Parallel()
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
test.nsc = test.setupNSCache()
if test.setupNSCache == nil {
test.nsc = nil
} else {
test.nsc = test.setupNSCache()
}
x := test.req.Request.URL.Query()
if x != nil {
fmt.Printf("stop here")
}
lo, err := ParseQuery(test.req, test.nsc)
if test.errExpected {
assert.NotNil(t, err)
if test.errorText != "" {
assert.Contains(t, test.errorText, err.Error())
}
//assert.Equal(t, err, errors.New("unable to parse requirement: existence tests are valid only for labels; not valid for field 'a5In1'"))
return
} else {
assert.Nil(t, err)
Expand Down
3 changes: 3 additions & 0 deletions pkg/stores/sqlpartition/queryparser/selector.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,9 @@ func (p *Parser) parseRequirement() (*Requirement, error) {
return nil, err
}
if operator == selection.Exists || operator == selection.DoesNotExist { // operator found lookahead set checked
if !strings.HasPrefix(key, "metadata.labels.") {
return nil, fmt.Errorf("existence tests are valid only for labels; not valid for field '%s'", key)
}
return NewRequirement(key, operator, []string{}, field.WithPath(p.path))
}
operator, err = p.parseOperator()
Expand Down
5 changes: 4 additions & 1 deletion pkg/stores/sqlpartition/queryparser/selector_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,17 @@ func TestSelectorParse(t *testing.T) {
"x= ",
"x=,z= ",
"x= ,z= ",
"!x",
"x>1",
"x>1,z<5",
"x gt 1,z lt 5",
`x == "abc"`,
`y == 'def'`,
"metadata.labels.im-here",
"!metadata.labels.im-not-here",
}
testBadStrings := []string{
"!no-label-absence-test",
"no-label-presence-test",
"x=a||y=b",
"x==a==b",
"!x=a",
Expand Down

0 comments on commit 697fdee

Please sign in to comment.