-
Notifications
You must be signed in to change notification settings - Fork 86
Query language support for child iteration & area #2182
Changes from 40 commits
e06b5c6
375ee86
4fc0296
7047576
fe3528a
31b3511
0b46360
b53c8b9
d19f69f
949cb11
c43fd0a
2db9f35
09b67fd
10ae3d8
5588a87
9c02141
924ef7a
9afe79c
bdf423e
1a6bf6a
826c6cc
8450f41
4dc2094
c77e254
0c39c44
bb8f8a7
c8b23b5
33d6069
d35ccb4
a84a234
88e26d1
51ef43e
f3c1463
761d3dc
28909d0
dcbad2b
3ef0e22
387c959
0c6e586
0ed33f8
366f81b
be44bd2
80f7103
65e483a
1184566
10b021e
7e29206
d524455
1488af0
251645a
20a5d4f
3e69d32
46f2e98
b8392a4
42d62d5
3dbf8fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1050,6 +1050,175 @@ func (s *searchControllerTestSuite) TestSearchByJoinedData() { | |
}) | ||
} | ||
|
||
// TestSearchWorkItemsWithChildIterationsOption verifies the child work item search for iteration | ||
func (s *searchControllerTestSuite) TestSearchWorkItemsWithChildIterationsOption() { | ||
s.T().Run("iterations", func(t *testing.T) { | ||
fxt := tf.NewTestFixture(t, s.DB, | ||
tf.Iterations(3, func(fxt *tf.TestFixture, idx int) error { | ||
i := fxt.Iterations[idx] | ||
switch idx { | ||
case 0: | ||
i.Name = "Top level iteration" | ||
case 1: | ||
i.Name = "Level 1 iteration" | ||
i.MakeChildOf(*fxt.Iterations[idx-1]) | ||
case 2: | ||
i.Name = "Level 2 iteration" | ||
i.MakeChildOf(*fxt.Iterations[idx-1]) | ||
} | ||
return nil | ||
}), | ||
|
||
tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { | ||
switch idx { | ||
case 0, 1, 2: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[0].ID.String() | ||
case 3, 4: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[1].ID.String() | ||
case 5, 6, 7, 8: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[2].ID.String() | ||
} | ||
return nil | ||
}), | ||
) | ||
|
||
spaceIDStr := fxt.Spaces[0].ID.String() | ||
|
||
t.Run("without child iteration", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s", "child": true}`, fxt.Iterations[2].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 4) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[5].ID, | ||
fxt.WorkItems[6].ID, | ||
fxt.WorkItems[7].ID, | ||
fxt.WorkItems[8].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
t.Run("with one child iteration", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s/**"}`, fxt.Iterations[1].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 6) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[3].ID, | ||
fxt.WorkItems[4].ID, | ||
fxt.WorkItems[5].ID, | ||
fxt.WorkItems[6].ID, | ||
fxt.WorkItems[7].ID, | ||
fxt.WorkItems[8].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should say: "unexpected work item found" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 10b021e |
||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
t.Run("with two child iteration", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s/**"}`, fxt.Iterations[0].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 9) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[0].ID, | ||
fxt.WorkItems[1].ID, | ||
fxt.WorkItems[2].ID, | ||
fxt.WorkItems[3].ID, | ||
fxt.WorkItems[4].ID, | ||
fxt.WorkItems[5].ID, | ||
fxt.WorkItems[6].ID, | ||
fxt.WorkItems[7].ID, | ||
fxt.WorkItems[8].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
|
||
t.Run("without child iteration - implicit", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[2].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 4) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[5].ID, | ||
fxt.WorkItems[6].ID, | ||
fxt.WorkItems[7].ID, | ||
fxt.WorkItems[8].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
t.Run("without child iteration - no /** suffix", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[2].ID) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need to provide 'child:false' option here as the test says 'false option'. Otherwise how would this test be diffferent from the one above it, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 33d6069 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reverted the commit, because it's no more relying on |
||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 4) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[5].ID, | ||
fxt.WorkItems[6].ID, | ||
fxt.WorkItems[7].ID, | ||
fxt.WorkItems[8].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
t.Run("with one child iteration - no /** suffix", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[1].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 2) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[3].ID, | ||
fxt.WorkItems[4].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to find all work items: %+s", toBeFound) | ||
}) | ||
t.Run("with two child iteration - no /** suffix", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "child" attribute is not recognized by the parser, and it won't make any difference. So, I am not sure we need to test it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. It looks like this line https://github.com/fabric8-services/fabric8-wit/pull/2182/files#diff-acc8d3974d80a8260ecd28803a0b064aR278 would set |
||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[0].ID) | ||
_, result := test.ShowSearchOK(t, nil, nil, s.controller, &filter, nil, nil, nil, nil, &spaceIDStr) | ||
require.NotEmpty(t, result.Data) | ||
assert.Len(t, result.Data, 3) | ||
toBeFound := id.MapFromSlice(id.Slice{ | ||
fxt.WorkItems[0].ID, | ||
fxt.WorkItems[1].ID, | ||
fxt.WorkItems[2].ID, | ||
}) | ||
for _, wi := range result.Data { | ||
_, ok := toBeFound[*wi.ID] | ||
require.True(t, ok, "unknown work item found: %s", *wi.ID) | ||
delete(toBeFound, *wi.ID) | ||
} | ||
require.Empty(t, toBeFound, "failed to found all work items: %+s", toBeFound) | ||
jarifibrahim marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
}) | ||
|
||
} | ||
|
||
// TestIncludedParents verifies the Included list of parents | ||
func (s *searchControllerTestSuite) TestIncludedParents() { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package criteria | ||
|
||
// ChildExpression represents the child operator | ||
type ChildExpression struct { | ||
binaryExpression | ||
} | ||
|
||
// Ensure ChildExpression implements the Expression interface | ||
var _ Expression = &ChildExpression{} | ||
var _ Expression = (*ChildExpression)(nil) | ||
|
||
// Accept implements ExpressionVisitor | ||
func (t *ChildExpression) Accept(visitor ExpressionVisitor) interface{} { | ||
return visitor.Child(t) | ||
} | ||
|
||
// Child constructs a ChildExpression | ||
func Child(left Expression, right Expression) Expression { | ||
return reparent(&ChildExpression{binaryExpression{expression{}, left, right}}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -275,9 +275,18 @@ func parseMap(queryMap map[string]interface{}, q *Query) { | |
q.Name = key | ||
s := string(concreteVal) | ||
q.Value = &s | ||
if q.Name == "iteration" || q.Name == "area" { | ||
if strings.HasSuffix(s, "/**") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you choose this suffix to be appended and not have a different compare function alongside Equal, Not? This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not getting the last of part. When this is not going to work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me try to rephrase my question... How can I query for an iteration or area by name and expect the children of the given iteration/area to be found? You're checking for
I think we can only allow for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is only considering ID column to lookup for child iteration/area. Since IDs are UUIDs, there won't be any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did get that and that is why I'm so concerned. But then you would have to learn a new syntax when you want to allow searching for an iteration/area that is given by any other field. I don't see the real value in this other than a quick fix with problems down the line. I mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If path & name requires similar syntax, we can do it in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is exactly my point, path, resolved_path, name, number, start date, ... they probably require different suffixes because of the domain and allowed vocabulary. And as a user I probably should not care by what field I query (e.g. name, path, id, number) the syntax should remain the same everywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, what should I do with this PR? Avoid using |
||
q.Child = true | ||
ns := s[0 : len(s)-3] | ||
q.Value = &ns | ||
} | ||
} | ||
case bool: | ||
s := concreteVal | ||
q.Negate = s | ||
if key == "negate" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this fixing? Should this maybe be moved out into its own PR? It seems like it is solving some other problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 366f81b |
||
q.Negate = s | ||
} | ||
case nil: | ||
q.Name = key | ||
q.Value = nil | ||
|
@@ -378,6 +387,8 @@ type Query struct { | |
Children []Query | ||
// The Options represent the query options provided by the user. | ||
Options *QueryOptions | ||
// Consider child iteration/area | ||
Child bool | ||
kwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func isOperator(str string) bool { | ||
|
@@ -436,7 +447,11 @@ func (q Query) generateExpression() (criteria.Expression, error) { | |
if q.Substring { | ||
myexpr = append(myexpr, criteria.Substring(left, right)) | ||
} else { | ||
myexpr = append(myexpr, criteria.Equals(left, right)) | ||
if q.Child { | ||
myexpr = append(myexpr, criteria.Child(left, right)) | ||
} else { | ||
myexpr = append(myexpr, criteria.Equals(left, right)) | ||
} | ||
} | ||
} | ||
} else { | ||
|
@@ -477,7 +492,11 @@ func (q Query) generateExpression() (criteria.Expression, error) { | |
if child.Substring { | ||
myexpr = append(myexpr, criteria.Substring(left, right)) | ||
} else { | ||
myexpr = append(myexpr, criteria.Equals(left, right)) | ||
if child.Child { | ||
myexpr = append(myexpr, criteria.Child(left, right)) | ||
} else { | ||
myexpr = append(myexpr, criteria.Equals(left, right)) | ||
} | ||
} | ||
} | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,82 @@ func (s *searchRepositoryBlackboxTest) getTestFixture() *tf.TestFixture { | |
) | ||
} | ||
|
||
func (s *searchRepositoryBlackboxTest) TestSearchWithChildIterationWorkItems() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "child" attribute is not recognized by the parser, and it won't make any difference. So, I am not sure we need to test it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me it is very unclear at this point if you have to specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no support for |
||
s.T().Run("iterations", func(t *testing.T) { | ||
fxt := tf.NewTestFixture(t, s.DB, | ||
tf.Iterations(3, func(fxt *tf.TestFixture, idx int) error { | ||
i := fxt.Iterations[idx] | ||
switch idx { | ||
case 0: | ||
i.Name = "Top level iteration" | ||
case 1: | ||
i.Name = "Level 1 iteration" | ||
i.MakeChildOf(*fxt.Iterations[idx-1]) | ||
case 2: | ||
i.Name = "Level 2 iteration" | ||
i.MakeChildOf(*fxt.Iterations[idx-1]) | ||
} | ||
return nil | ||
}), | ||
|
||
tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { | ||
switch idx { | ||
case 0, 1, 2: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[0].ID.String() | ||
case 3, 4: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[1].ID.String() | ||
case 5, 6, 7, 8: | ||
fxt.WorkItems[idx].Fields[workitem.SystemIteration] = fxt.Iterations[2].ID.String() | ||
} | ||
return nil | ||
}), | ||
) | ||
t.Run("without child iteration", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test name is confusion. You're searching for work items in iteration "Level 2 iteration" which has no children but the search itself does ask for children if I'm not mistaken. |
||
filter := fmt.Sprintf(`{"$AND": [{"iteration": "%s/**"}, {"space": "%s"}]}`, fxt.Iterations[2].ID, fxt.Spaces[0].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 4, count) | ||
}) | ||
|
||
t.Run("with one child iteration", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s/**"}`, fxt.Iterations[1].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 6, count) | ||
}) | ||
t.Run("with two child iteration", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s/**"}`, fxt.Iterations[0].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 9, count) | ||
}) | ||
t.Run("without child iteration - no /** suffix", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[2].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 4, count) | ||
}) | ||
t.Run("with one child iteration - no /** suffix", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[1].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 2, count) | ||
}) | ||
t.Run("with two child iteration - no /** suffix", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"iteration": "%s"}`, fxt.Iterations[0].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 3, count) | ||
}) | ||
t.Run("with two child iteration and space", func(t *testing.T) { | ||
filter := fmt.Sprintf(`{"$AND": [{"iteration": "%s/**"},{"space": "%s"}]}`, fxt.Iterations[0].ID, fxt.Spaces[0].ID) | ||
_, count, _, _, err := s.searchRepo.Filter(context.Background(), filter, nil, nil, nil) | ||
require.NoError(t, err) | ||
assert.Equal(t, 9, count) | ||
}) | ||
}) | ||
} | ||
|
||
func (s *searchRepositoryBlackboxTest) TestSearchWithJoin() { | ||
s.T().Run("join iterations", func(t *testing.T) { | ||
fxt := tf.NewTestFixture(t, s.DB, | ||
|
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.
This setup is all good, but if you're searching for something easier to set up please consider using this:
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.
Done. d524455