BED-5784 - Search Asset Group Tags Endpoint #1648
Conversation
WalkthroughA new POST endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant GraphDB
Client->>API: POST /api/v2/asset-group-tags/search {query, tag_type}
API->>DB: GetAssetGroupTags (filter by tag_type)
DB-->>API: AssetGroupTags
API->>DB: GetAssetGroupTagSelectors (SQL filter, limit)
DB-->>API: AssetGroupTagSelectors
API->>GraphDB: Query nodes (name/object_id contains query, kind in tag kinds, limit)
GraphDB-->>API: Graph nodes (members)
API-->>Client: 200 OK {tags, selectors, members}
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningscmd/api/src/api/v2/assetgrouptags_test.go (4)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
stephanieslamb
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
cmd/api/src/database/assetgrouptags_test.go (1)
1034-1067: Test coverage is minimal - consider adding more comprehensive test scenarios.The new test function only covers the happy path with an empty filter. Consider adding test cases for:
- Different filter conditions (similar to existing tests in the file)
- Error handling scenarios
- Edge cases like no results found
- Verification of returned data structure and content
Here's an example of how to expand the test coverage:
func TestDatabase_GetAssetGroupTagSelectors(t *testing.T) { var ( dbInst = integration.SetupDB(t) testCtx = context.Background() isDefault = false allowDisable = true autoCertify = null.BoolFrom(false) test1Selector = model.AssetGroupTagSelector{ Name: "test selector name", Description: "test description", Seeds: []model.SelectorSeed{ {Type: model.SelectorTypeObjectId, Value: "ObjectID1234"}, }, } test2Selector = model.AssetGroupTagSelector{ Name: "test2 selector name", Description: "test2 description", Seeds: []model.SelectorSeed{ {Type: model.SelectorTypeCypher, Value: "MATCH (n:User) RETURN n LIMIT 1;"}, }, } ) _, err := dbInst.CreateAssetGroupTagSelector(testCtx, 1, model.User{}, test1Selector.Name, test1Selector.Description, isDefault, allowDisable, autoCertify, test1Selector.Seeds) require.NoError(t, err) _, err = dbInst.CreateAssetGroupTagSelector(testCtx, 1, model.User{}, test2Selector.Name, test2Selector.Description, isDefault, allowDisable, autoCertify, test2Selector.Seeds) require.NoError(t, err) t.Run("returns all selectors", func(t *testing.T) { items, err := dbInst.GetAssetGroupTagSelectors(testCtx, model.SQLFilter{}) require.NoError(t, err) require.GreaterOrEqual(t, len(items), 2) + + // Verify the returned selectors contain expected data + var foundTest1, foundTest2 bool + for _, item := range items { + if item.Name == test1Selector.Name { + foundTest1 = true + require.Equal(t, test1Selector.Description, item.Description) + } + if item.Name == test2Selector.Name { + foundTest2 = true + require.Equal(t, test2Selector.Description, item.Description) + } + } + require.True(t, foundTest1, "test1 selector not found") + require.True(t, foundTest2, "test2 selector not found") }) + + t.Run("filters by name", func(t *testing.T) { + items, err := dbInst.GetAssetGroupTagSelectors(testCtx, model.SQLFilter{ + SQLString: "name LIKE ?", + Params: []any{"%test2%"}, + }) + require.NoError(t, err) + require.GreaterOrEqual(t, len(items), 1) + + // Verify only test2 selector is returned + found := false + for _, item := range items { + if item.Name == test2Selector.Name { + found = true + break + } + } + require.True(t, found, "test2 selector not found in filtered results") + }) }packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (2)
25-25: Remove trailing spaces.YAML linting detected trailing spaces on these lines.
- description: The request body for searching asset group tags. + description: The request body for searching asset group tags.- tags: + tags:- members: + members:Also applies to: 51-51, 59-59
70-74: Fix indentation for nested properties.The indentation for
nameandpropertiesfields is incorrect according to YAML standards.name: - type: string + type: string properties: - type: object - additionalProperties: true + type: object + additionalProperties: truepackages/go/openapi/doc/openapi.json (1)
6141-6145: OperationId style is inconsistent with existing endpointsMost operations in this spec follow the verb-first
SearchAssetGroupTagspattern (e.g.ListAssets,CreateUser). Consider renaming"AssetGroupTagSearch"to"SearchAssetGroupTags"for uniformity and easier client generation.cmd/api/src/api/v2/assetgrouptags.go (2)
785-811: Simplify tag type filtering logic.The current logic for grouping owned tags with labels is complex and could be more readable. Consider extracting this into a helper function.
+func shouldIncludeTag(tag model.AssetGroupTag, requestedType model.AssetGroupTagType) bool { + switch requestedType { + case model.AssetGroupTagTypeLabel: + return tag.Type == model.AssetGroupTagTypeLabel || tag.Type == model.AssetGroupTagTypeOwned + case model.AssetGroupTagTypeTier: + return tag.Type == model.AssetGroupTagTypeTier + default: + return false + } +} for _, t := range tags { - isLabelType := t.Type == model.AssetGroupTagTypeLabel - isOwnedType := t.Type == model.AssetGroupTagTypeOwned - - // group owned with labels - if reqBody.TagType == model.AssetGroupTagTypeLabel && (isLabelType || isOwnedType) { - kinds = append(kinds, t.ToKind()) - if strings.Contains(strings.ToLower(t.Name), strings.ToLower(reqBody.Query)) { - matchedTags = append(matchedTags, t) - - if len(matchedTags) >= assetGroupTagsSearchLimit { - break - } - } - } - - if reqBody.TagType == model.AssetGroupTagTypeTier && t.Type == model.AssetGroupTagTypeTier { + if shouldIncludeTag(t, reqBody.TagType) { kinds = append(kinds, t.ToKind()) if strings.Contains(strings.ToLower(t.Name), strings.ToLower(reqBody.Query)) { matchedTags = append(matchedTags, t) @@ -807,7 +789,6 @@ break } } - } + } }
748-830: Consider extracting search logic into separate methods.The main handler function is quite long and handles multiple concerns. Consider extracting the search logic into separate methods for better maintainability.
Example structure:
func (s *Resources) SearchAssetGroupTags(response http.ResponseWriter, request *http.Request) { reqBody, err := s.parseSearchRequest(request) if err != nil { api.WriteErrorResponse(request.Context(), err, response) return } result, err := s.performSearch(request.Context(), reqBody, request.URL.Query()) if err != nil { api.HandleDatabaseError(request, response, err) return } api.WriteBasicResponse(request.Context(), result, http.StatusOK, response) } func (s *Resources) parseSearchRequest(request *http.Request) (*AssetGroupTagSearchRequest, error) { // validation logic } func (s *Resources) performSearch(ctx context.Context, req *AssetGroupTagSearchRequest, queryParams url.Values) (*SearchAssetGroupTagsResponse, error) { // search logic }cmd/api/src/api/v2/assetgrouptags_test.go (1)
2256-2256: Addt.Parallel()for improved test performance.Based on the codebase patterns, main test functions can safely use
t.Parallel()for performance benefits.func TestDatabase_SearchAssetGroupTags(t *testing.T) { + t.Parallel() var (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/database/assetgrouptags.go(3 hunks)cmd/api/src/database/assetgrouptags_test.go(2 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/model/assetgrouptags.go(1 hunks)packages/go/openapi/doc/openapi.json(1 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
cmd/api/src/database/assetgrouptags_test.go (2)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
cmd/api/src/api/v2/assetgrouptags_test.go (2)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
🧬 Code Graph Analysis (5)
cmd/api/src/model/assetgrouptags.go (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTagTypeTier(56-56)AssetGroupTagTypeLabel(57-57)AssetGroupTagTypeOwned(58-58)
cmd/api/src/api/registration/v2.go (1)
cmd/api/src/model/appcfg/flag.go (1)
FeatureTierManagement(42-42)
cmd/api/src/database/mocks/db.go (2)
cmd/api/src/model/filter.go (1)
SQLFilter(93-96)cmd/api/src/model/assetgrouptags.go (1)
AssetGroupTagSelectors(191-191)
cmd/api/src/database/assetgrouptags_test.go (3)
cmd/api/src/test/integration/database.go (1)
SetupDB(57-63)cmd/api/src/model/assetgrouptags.go (4)
AssetGroupTagSelector(193-209)AssetGroupTagSelector(211-213)SelectorSeed(170-174)SelectorSeed(176-178)cmd/api/src/model/filter.go (1)
SQLFilter(93-96)
cmd/api/src/database/assetgrouptags.go (4)
cmd/api/src/model/filter.go (1)
SQLFilter(93-96)cmd/api/src/model/assetgrouptags.go (5)
AssetGroupTagSelectors(191-191)AssetGroupTag(74-89)AssetGroupTag(93-95)AssetGroupTagSelector(193-209)AssetGroupTagSelector(211-213)cmd/api/src/database/helper.go (1)
CheckError(26-32)cmd/api/src/database/db.go (1)
BloodhoundDB(176-179)
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[warning] 71-71: wrong indentation: expected 26 but found 28
(indentation)
[warning] 73-73: wrong indentation: expected 26 but found 28
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (11)
cmd/api/src/model/assetgrouptags.go (1)
151-153: LGTM! Clean validation function implementation.The
IsValidTagTypefunction is well-implemented with clear logic and purpose. It properly validates against the three defined constants and follows Go naming conventions.cmd/api/src/api/registration/v2.go (1)
177-177: LGTM! Route registration follows established patterns.The new search endpoint is properly registered with appropriate feature flag checks and permissions. The placement within the asset group management API section is logical and consistent with existing routes.
packages/go/openapi/src/openapi.yaml (1)
355-356: LGTM! OpenAPI specification properly updated.The new search endpoint is correctly added to the OpenAPI specification with proper path structure and external file reference. The placement within the asset isolation section maintains logical organization.
cmd/api/src/database/mocks/db.go (1)
1164-1177: LGTM! Well-implemented mock method following GoMock conventions.The new
GetAssetGroupTagSelectorsmock method and its corresponding recorder method are properly implemented following standard GoMock patterns. The method signature correctly matches the expected parameters (context.Contextandmodel.SQLFilter) and return types (model.AssetGroupTagSelectorsanderror), which aligns with the search functionality requirements described in the PR objectives.cmd/api/src/database/assetgrouptags_test.go (1)
833-833: Function rename looks good.The renaming from
TestDatabase_GetAssetGroupTagSelectorstoTestDatabase_GetAssetGroupTagSelectorsBySelectorIdbetter reflects the actual method being tested (GetAssetGroupTagSelectorBySelectorId).packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
16-89: API specification structure looks comprehensive.The endpoint definition includes:
- Proper operation ID and summary
- Clear request body schema with required fields
- Well-structured response schema with nested objects
- Standard error response references
The API design supports searching across tags, selectors, and members which aligns with the PR objectives.
cmd/api/src/database/assetgrouptags.go (2)
245-245: Good addition of explicit ordering.Adding
ORDER BY name ASC, id ASCensures consistent and deterministic ordering of results, which is important for pagination and user experience.
712-731: Well-implemented method following existing patterns.The implementation correctly uses parameterized queries and follows the established pattern for handling SQL filters. The ordering by name, asset_group_tag_id, and id ensures consistent results.
Note that this method includes disabled selectors (no
disabled_at IS NULLfilter), which differs from some other methods likeGetAssetGroupTagSelectorCounts. Please verify this is intentional for the search functionality.cmd/api/src/api/v2/assetgrouptags.go (3)
27-27: LGTM: Import addition is appropriate.The
stringsimport is correctly added to support case-insensitive string operations in the search functionality.
51-51: LGTM: Search limit constant is well-defined.The
assetGroupTagsSearchLimitconstant provides a reasonable limit for search results and follows the existing naming convention.
737-746: LGTM: Type definitions are well-structured.The request and response types are appropriately defined with clear field names and proper JSON tags.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (4)
25-25: Fix trailing spaces.Remove trailing spaces as flagged by yamllint.
- description: The request body for searching asset group tags. + description: The request body for searching asset group tags.
51-51: Fix trailing spaces.Remove trailing spaces as flagged by yamllint.
- tags: + tags:
59-59: Fix trailing spaces.Remove trailing spaces as flagged by yamllint.
- members: + members:
71-73: Fix indentation.The indentation for these properties is inconsistent. Adjust to match the expected 26-space indentation.
name: - type: string + type: string properties: - type: object + type: object
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)packages/go/openapi/doc/openapi.json(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🧠 Learnings (1)
cmd/api/src/api/v2/assetgrouptags_test.go (3)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[warning] 71-71: wrong indentation: expected 26 but found 28
(indentation)
[warning] 73-73: wrong indentation: expected 26 but found 28
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (2)
cmd/api/src/api/v2/assetgrouptags_test.go (2)
28-28: LGTM - Good addition of strings import.The strings import is appropriately added to support the test implementation.
2256-2595: Excellent comprehensive test coverage.The test function
TestDatabase_SearchAssetGroupTagsprovides thorough coverage of the search endpoint including:
- Input validation (malformed JSON, invalid tag types, empty queries)
- Database error scenarios
- Parameter parsing errors
- Multiple successful search scenarios by name, type, and object ID
- Proper response structure validation
The test structure is well-organized with clear test case names and proper setup/teardown using mocks.
cmd/api/src/model/assetgrouptags.go
Outdated
| func IsValidTagType(tagType AssetGroupTagType) bool { | ||
| return tagType == AssetGroupTagTypeTier || tagType == AssetGroupTagTypeLabel || tagType == AssetGroupTagTypeOwned | ||
| } |
There was a problem hiding this comment.
Maybe this is a question for @mistahj67 but now that we have 0 for hygiene should that be included here too?
There was a problem hiding this comment.
I guess for the particular usage here this is what we want, but just based on the method name I'm still wondering if we should handle 0 somehow.
There was a problem hiding this comment.
I'd likely nix this entirely in favor of leveraging the ToType() method and kicking out if it's unknown. Findings arent a factor for this endpoint, it's just the search for tags / selectors / members
cmd/api/src/model/assetgrouptags.go
Outdated
| func IsValidTagType(tagType AssetGroupTagType) bool { | ||
| return tagType == AssetGroupTagTypeTier || tagType == AssetGroupTagTypeLabel || tagType == AssetGroupTagTypeOwned | ||
| } |
There was a problem hiding this comment.
I guess for the particular usage here this is what we want, but just based on the method name I'm still wondering if we should handle 0 somehow.
cmd/api/src/api/v2/assetgrouptags.go
Outdated
| api.WriteErrorResponse(request.Context(), ErrBadQueryParameter(request, model.PaginationQueryParameterSkip, err), response) | ||
| } else { | ||
| var ( | ||
| kinds []graph.Kind |
There was a problem hiding this comment.
| kinds []graph.Kind | |
| kinds graph.Kinds |
This will also allow you to use kinds.Add(...) instead of append if you want.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
Outdated
Show resolved
Hide resolved
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
25-25: Fix trailing spaces to comply with YAML formatting standards.The static analysis tool identified trailing spaces that should be removed for consistent formatting.
- description: The request body for searching asset group tags. + description: The request body for searching asset group tags.- tags: + tags:- members: + members:Also applies to: 51-51, 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/api/error.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/database/assetgrouptags.go(3 hunks)packages/go/openapi/doc/openapi.json(5 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml
- cmd/api/src/api/error.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
- packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/api/v2/assetgrouptags.go
- cmd/api/src/database/assetgrouptags.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
cmd/api/src/api/v2/assetgrouptags_test.go (4)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
packages/go/openapi/doc/openapi.json (1)
undefined
<retrieved_learning>
Learnt from: mistahj67
PR: #1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete kinds array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
</retrieved_learning>
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🧬 Code Graph Analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (9)
cmd/api/src/database/mocks/db.go (1)
NewMockDatabase(55-59)cmd/api/src/queries/mocks/graph.go (1)
NewMockGraph(53-57)cmd/api/src/api/v2/model.go (1)
Resources(102-115)cmd/api/src/api/v2/assetgrouptags.go (2)
SearchAssetGroupTagsResponse(737-741)AssetGroupMember(528-536)cmd/api/src/ctx/ctx.go (1)
Set(88-90)packages/go/headers/headers.go (1)
ContentType(75-75)packages/go/mediatypes/application.go (1)
ApplicationJson(214-214)cmd/api/src/model/assetgrouptags.go (2)
AssetGroupTags(91-91)AssetGroupTagSelectors(191-191)packages/go/graphschema/common/common.go (3)
Name(53-53)ObjectID(52-52)PrimaryKind(70-70)
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (7)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml (1)
54-61: Excellent schema refactoring for reusability.The change from inline schema definition to
allOfcomposition with a shared reference promotes consistency and maintainability across the API. This approach correctly extends the base member schema while adding the specificselectorsproperty.cmd/api/src/api/v2/assetgrouptags_test.go (5)
2256-2277: Comprehensive test coverage for the new search endpoint.The test function provides excellent coverage of the SearchAssetGroupTags functionality, including error scenarios, input validation, and success cases across different tag types. The test setup with proper mocking and context creation follows established patterns in the codebase.
2282-2318: Well-structured error case testing.The error case tests properly validate input validation scenarios including malformed JSON, invalid tag types, and query length validation. The test cases appropriately use mock expectations and validate the expected HTTP status codes and error messages.
2375-2428: Thorough success scenario testing with proper response validation.The success test cases comprehensively validate the search functionality across different tag types (tier, label) and search criteria (name, object ID). The test properly validates the response structure and content, ensuring the API returns the expected data format.
2485-2538: Good test coverage for mixed tag type scenarios.This test case effectively validates the behavior when searching across multiple tag types, ensuring the API correctly handles queries that match tags of different types (label and owned). The test validates that the response includes all matching tags while filtering selectors appropriately.
2540-2594: Proper validation of object ID search functionality.The test correctly validates the search behavior when querying by object ID, ensuring that only members with matching object IDs are returned while tags and selectors remain empty when no name matches occur. This demonstrates proper separation of search criteria handling.
packages/go/openapi/doc/openapi.json (1)
5350-5354: 👍 Reference consolidation looks goodReplacing the inline member definitions with a shared
$refimproves consistency and reduces spec drift. Nice clean-up!Also applies to: 5523-5527
| "properties": { | ||
| "query": { | ||
| "type": "string" | ||
| }, | ||
| "tag_type": { | ||
| "type": "integer" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "query", | ||
| "tag_type" | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add basic validation for query and constrain tag_type to known values
The handler rejects queries shorter than 3 characters and validates tag_type against a finite enum. Encoding those rules in the schema prevents round-trips with immediately‐invalid payloads.
"properties": {
"query": {
- "type": "string"
+ "type": "string",
+ "minLength": 3,
+ "description": "Case-insensitive substring to match; must be ≥ 3 characters."
},
"tag_type": {
- "type": "integer"
+ "type": "integer",
+ "enum": [0, 1], // 0 = selector tag, 1 = member tag
+ "description": "Asset-group tag type. See model.AssetGroupTagType."
}
},
"required": [
"query",
"tag_type"
]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/go/openapi/doc/openapi.json around lines 6110 to 6121, the schema
for the "query" and "tag_type" properties lacks validation constraints. Add a
"minLength" of 3 to the "query" property to enforce the minimum character
length, and define an "enum" for "tag_type" listing all valid integer values to
restrict it to known options. This will ensure the schema validates inputs
consistently with the handler logic.
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/model.asset-group-tags-member" | ||
| }, | ||
| "properties": { | ||
| { | ||
| "type": "object", | ||
| "additionalProperties": true | ||
| }, | ||
| "source": { | ||
| "type": "integer" | ||
| }, | ||
| "selectors": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | ||
| "properties": { | ||
| "selectors": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark selectors as a required property inside the composed member object
selectors is not optional in the actual response – every call that returns a member object includes the matched selectors. Add a required array to the second allOf object so schema-driven clients don’t think the field might be omitted.
"properties": {
"selectors": {
"type": "array",
"items": {
"$ref": "#/components/schemas/model.asset-group-tags-selector-response"
}
}
+ },
+ "required": [
+ "selectors"
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "allOf": [ | |
| { | |
| "$ref": "#/components/schemas/model.asset-group-tags-member" | |
| }, | |
| "properties": { | |
| { | |
| "type": "object", | |
| "additionalProperties": true | |
| }, | |
| "source": { | |
| "type": "integer" | |
| }, | |
| "selectors": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | |
| "properties": { | |
| "selectors": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | |
| } | |
| } | |
| } | |
| "allOf": [ | |
| { | |
| "$ref": "#/components/schemas/model.asset-group-tags-member" | |
| }, | |
| { | |
| "type": "object", | |
| "properties": { | |
| "selectors": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | |
| } | |
| } | |
| }, | |
| "required": [ | |
| "selectors" | |
| ] | |
| } |
🤖 Prompt for AI Agents
In packages/go/openapi/doc/openapi.json around lines 4850 to 4863, the
`selectors` property inside the composed `member` object is missing from the
`required` array, making it appear optional. Add a `required` array to the
second `allOf` object and include `"selectors"` in it to indicate that this
property is mandatory in the schema.
| "data": { | ||
| "type": "object", | ||
| "properties": { | ||
| "tags": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/model.asset-group-tag" | ||
| } | ||
| }, | ||
| "selectors": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | ||
| } | ||
| }, | ||
| "members": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/model.asset-group-tags-member" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define required fields for the success payload
Clients can’t rely on tags, selectors, or members being present because they’re not marked as required. They are always returned (even if empty arrays), so mark them as mandatory.
"properties": {
"tags": {
"type": "array",
"items": {
"$ref": "#/components/schemas/model.asset-group-tag"
}
},
"selectors": {
"type": "array",
"items": {
"$ref": "#/components/schemas/model.asset-group-tags-selector-response"
}
},
"members": {
"type": "array",
"items": {
"$ref": "#/components/schemas/model.asset-group-tags-member"
}
}
+ },
+ "required": [
+ "tags",
+ "selectors",
+ "members"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "data": { | |
| "type": "object", | |
| "properties": { | |
| "tags": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tag" | |
| } | |
| }, | |
| "selectors": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | |
| } | |
| }, | |
| "members": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-member" | |
| } | |
| } | |
| } | |
| "data": { | |
| "type": "object", | |
| "properties": { | |
| "tags": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tag" | |
| } | |
| }, | |
| "selectors": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-selector-response" | |
| } | |
| }, | |
| "members": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/model.asset-group-tags-member" | |
| } | |
| } | |
| }, | |
| "required": [ | |
| "tags", | |
| "selectors", | |
| "members" | |
| ] | |
| } |
🤖 Prompt for AI Agents
In packages/go/openapi/doc/openapi.json around lines 6134 to 6155, the
properties "tags", "selectors", and "members" inside the "data" object are not
marked as required, but they are always returned even if empty. To fix this, add
a "required" array listing these three properties to the "data" object schema to
indicate they are mandatory fields.
| "model.asset-group-tags-member": { | ||
| "type": "object", | ||
| "properties": { | ||
| "id": { | ||
| "type": "integer" | ||
| }, | ||
| "object_id": { | ||
| "type": "string" | ||
| }, | ||
| "primary_kind": { | ||
| "type": "string" | ||
| }, | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "properties": { | ||
| "type": "object", | ||
| "additionalProperties": true | ||
| }, | ||
| "source": { | ||
| "type": "integer" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schema completeness: list required member fields and lock down extra properties
id, object_id, primary_kind, and name are always present, and unknown keys should be forbidden for forward-compatibility.
"model.asset-group-tags-member": {
"type": "object",
+ "additionalProperties": false,
"properties": {
"id": {
"type": "integer"
},
"object_id": {
"type": "string"
},
"primary_kind": {
"type": "string"
},
"name": {
"type": "string"
},
"properties": {
"type": "object",
"additionalProperties": true
},
"source": {
"type": "integer"
}
- }
+ },
+ "required": [
+ "id",
+ "object_id",
+ "primary_kind",
+ "name"
+ ]
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "model.asset-group-tags-member": { | |
| "type": "object", | |
| "properties": { | |
| "id": { | |
| "type": "integer" | |
| }, | |
| "object_id": { | |
| "type": "string" | |
| }, | |
| "primary_kind": { | |
| "type": "string" | |
| }, | |
| "name": { | |
| "type": "string" | |
| }, | |
| "properties": { | |
| "type": "object", | |
| "additionalProperties": true | |
| }, | |
| "source": { | |
| "type": "integer" | |
| } | |
| } | |
| "model.asset-group-tags-member": { | |
| "type": "object", | |
| "additionalProperties": false, | |
| "properties": { | |
| "id": { | |
| "type": "integer" | |
| }, | |
| "object_id": { | |
| "type": "string" | |
| }, | |
| "primary_kind": { | |
| "type": "string" | |
| }, | |
| "name": { | |
| "type": "string" | |
| }, | |
| "properties": { | |
| "type": "object", | |
| "additionalProperties": true | |
| }, | |
| "source": { | |
| "type": "integer" | |
| } | |
| }, | |
| "required": [ | |
| "id", | |
| "object_id", | |
| "primary_kind", | |
| "name" | |
| ] | |
| }, |
🤖 Prompt for AI Agents
In packages/go/openapi/doc/openapi.json around lines 17723 to 17745, the schema
for "model.asset-group-tags-member" lacks a "required" array listing mandatory
fields and does not forbid additional unknown properties. Add a "required" field
listing "id", "object_id", "primary_kind", and "name" to enforce their presence,
and set "additionalProperties" to false to prevent unknown keys for better
forward-compatibility.
mistahj67
force-pushed
the
BED-5784-search
branch
from
c67b7c2 to
13d6723
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
25-25: Fix trailing spaces identified by static analysis.Remove trailing spaces on lines 25, 51, and 59 to maintain consistent formatting.
- description: The request body for searching asset group tags. + description: The request body for searching asset group tags.- tags: + tags:- members: + members:Also applies to: 51-51, 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/api/src/api/error.go(1 hunks)cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/database/assetgrouptags.go(4 hunks)cmd/api/src/database/assetgrouptags_test.go(2 hunks)cmd/api/src/database/migration/migrations/v8.0.0.sql(1 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/services/graphify/analysis_integration_test.go(4 hunks)packages/go/openapi/doc/openapi.json(5 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx
- packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts
- cmd/api/src/database/migration/migrations/v8.0.0.sql
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx
- packages/go/openapi/src/openapi.yaml
- cmd/api/src/services/graphify/analysis_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
- cmd/api/src/api/registration/v2.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml
- packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml
- cmd/api/src/database/mocks/db.go
- cmd/api/src/database/assetgrouptags_test.go
- cmd/api/src/api/error.go
- cmd/api/src/api/v2/assetgrouptags.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/database/assetgrouptags.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
cmd/api/src/api/v2/assetgrouptags_test.go (4)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🧬 Code Graph Analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (2)
cmd/api/src/model/assetgrouptags.go (2)
AssetGroupTags(92-92)AssetGroupTagSelectors(188-188)packages/go/graphschema/common/common.go (1)
Name(53-53)
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (2)
cmd/api/src/api/v2/assetgrouptags_test.go (2)
2254-2593: Comprehensive test coverage for SearchAssetGroupTags endpoint.This test function provides excellent coverage of the new search functionality, including:
- Input validation scenarios (malformed JSON, invalid tag type, short queries)
- Database error handling for both tags and selectors
- Query parameter parsing validation
- Success scenarios for different search patterns (by name, by object ID, different tag types)
The test structure follows Go best practices with clear subtest organization and appropriate mock expectations. The response validation thoroughly checks both HTTP status codes and JSON response content.
28-28: Import addition looks correct.The addition of the
stringsimport is appropriate for the new test functions that usestrings.NewReader.
mistahj67
force-pushed
the
BED-5784-search
branch
from
ebb7008 to
76e597f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
25-25: Remove trailing spaces.Static analysis detected trailing spaces on these lines that should be removed for consistency.
- description: The request body for searching asset group tags. + description: The request body for searching asset group tags.- tags: + tags:- members: + members:Also applies to: 51-51, 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/api/src/api/error.go(1 hunks)cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/database/assetgrouptags.go(4 hunks)cmd/api/src/database/assetgrouptags_test.go(2 hunks)cmd/api/src/database/migration/migrations/v8.0.0.sql(1 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/services/graphify/analysis_integration_test.go(4 hunks)packages/go/openapi/doc/openapi.json(5 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts(1 hunks)packages/javascript/bh-shared-ui/src/graphSchema.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/graphSchema.ts
✅ Files skipped from review due to trivial changes (3)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx
- cmd/api/src/services/graphify/analysis_integration_test.go
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/api/src/database/migration/migrations/v8.0.0.sql
- packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts
- cmd/api/src/api/registration/v2.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml
- packages/go/openapi/src/openapi.yaml
- cmd/api/src/api/error.go
- cmd/api/src/database/assetgrouptags_test.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/api/v2/assetgrouptags.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
- cmd/api/src/database/assetgrouptags.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml
- packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml
- cmd/api/src/database/mocks/db.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
cmd/api/src/api/v2/assetgrouptags_test.go (4)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (4)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (3)
16-39: Well-structured OpenAPI endpoint definition.The endpoint definition is complete with appropriate operationId, summary, description, and request schema. The required fields correctly match the defined properties.
40-62: Good use of schema references for response structure.The response schema appropriately uses $ref to reference reusable schemas, promoting consistency across the API while maintaining clear structure.
63-74: Complete error response coverage.The endpoint correctly defines all standard error responses with appropriate HTTP status codes and references to reusable error schemas.
cmd/api/src/api/v2/assetgrouptags_test.go (1)
2254-2593: Excellent comprehensive test coverage for the search endpoint.This test function provides thorough coverage of the
SearchAssetGroupTagshandler with well-structured test cases covering:
- Input validation errors (malformed JSON, invalid tag type, short query)
- Database error scenarios
- Parameter parsing errors
- Multiple success scenarios with different tag types
- Search by both name and object ID
The test properly uses mocks, follows Go testing conventions, and consistently uses the endpoint variable as requested in previous feedback. The assertions are appropriate and the test data is realistic.
…ter to be all tags and not just by name
mistahj67
force-pushed
the
BED-5784-search
branch
from
76e597f to
084d972
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
18-23: Consider widening the summaryThe description correctly mentions selectors & members, but the summary only says “Search Asset Group Tags”. To avoid confusion in API docs, maybe expand to “Search Asset Group Tags, Selectors, and Members”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/api/src/api/error.go(1 hunks)cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(2 hunks)cmd/api/src/database/assetgrouptags.go(4 hunks)cmd/api/src/database/assetgrouptags_test.go(2 hunks)cmd/api/src/database/migration/migrations/v8.0.0.sql(1 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/services/graphify/analysis_integration_test.go(4 hunks)packages/go/openapi/doc/openapi.json(5 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsComboBox.tsx
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ManageColumnsComboBox/ManageColumnsListItem.tsx
- cmd/api/src/services/graphify/analysis_integration_test.go
- packages/go/openapi/src/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/explore-table-utils.ts
- cmd/api/src/database/migration/migrations/v8.0.0.sql
- cmd/api/src/api/registration/v2.go
- cmd/api/src/database/mocks/db.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.selectors.id.members.yaml
- cmd/api/src/api/error.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.id.yaml
- packages/go/openapi/src/schemas/model.asset-group-tags-member.yaml
- cmd/api/src/database/assetgrouptags_test.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
- cmd/api/src/api/v2/assetgrouptags_test.go
- cmd/api/src/database/assetgrouptags.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/api/v2/assetgrouptags.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml (1)
17-23: Align operation naming with existing conventionsOther endpoints use
SearchAssetGroupTags(verb first) rather thanAssetGroupTagSearch. Renaming keeps generated client methods predictable:- operationId: AssetGroupTagSearch + operationId: SearchAssetGroupTags
| requestBody: | ||
| description: The request body for searching asset group tags. | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| query: | ||
| type: string | ||
| tag_type: | ||
| type: integer | ||
| required: | ||
| - query | ||
| - tag_type |
There was a problem hiding this comment.
Strengthen request schema ‒ add validation & clean up lint errors
query
• NominLength→ empty strings will pass validation and execute a DB search.tag_type
• Bareintegergives no clue what values are acceptable. Anenum(or$refto a sharedTagTypeschema) prevents invalid numbers.- Trailing spaces on lines 25, 37 → YAML-lint fails CI.
application/json:
schema:
type: object
properties:
query:
type: string
+ description: The case-insensitive substring to match.
+ minLength: 1
tag_type:
- type: integer
+ description: Asset-group tag category to limit the search.
+ type: integer
+ enum: [1, 2] # ← replace with correct set or `$ref`
required:
- query
- tag_type📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requestBody: | |
| description: The request body for searching asset group tags. | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| type: object | |
| properties: | |
| query: | |
| type: string | |
| tag_type: | |
| type: integer | |
| required: | |
| - query | |
| - tag_type | |
| requestBody: | |
| description: The request body for searching asset group tags. | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| type: object | |
| properties: | |
| query: | |
| type: string | |
| description: The case-insensitive substring to match. | |
| minLength: 1 | |
| tag_type: | |
| description: Asset-group tag category to limit the search. | |
| type: integer | |
| enum: [1, 2] # ← replace with correct set or `$ref` | |
| required: | |
| - query | |
| - tag_type |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 25-25: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
lines 24 to 38, enhance the requestBody schema by adding a minLength constraint
to the query property to prevent empty strings from passing validation. For
tag_type, replace the bare integer type with an enum or a $ref to a shared
TagType schema to restrict acceptable values and prevent invalid inputs. Also,
remove trailing spaces on lines 25 and 37 to fix YAML lint errors and ensure CI
passes.
| responses: | ||
| 200: | ||
| description: OK | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| data: | ||
| type: object | ||
| properties: | ||
| tags: | ||
| type: array | ||
| items: | ||
| $ref: './../schemas/model.asset-group-tag.yaml' | ||
| selectors: | ||
| type: array | ||
| items: | ||
| $ref: './../schemas/model.asset-group-tags-selector-response.yaml' | ||
| members: | ||
| type: array | ||
| items: | ||
| $ref: './../schemas/model.asset-group-tags-member.yaml' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Response arrays should declare minItems: 0 and remove trailing spaces
OpenAPI generators treat absent minItems as “must have ≥1 item”, which breaks typed clients when the search yields no matches. Add minItems: 0 (or set arrays as nullable) so an empty search is valid. Also drop the trailing spaces on lines 51 & 59.
properties:
tags:
type: array
+ minItems: 0
items:
$ref: './../schemas/model.asset-group-tag.yaml'
selectors:
type: array
+ minItems: 0
items:
$ref: './../schemas/model.asset-group-tags-selector-response.yaml'
members:
type: array
+ minItems: 0
items:
$ref: './../schemas/model.asset-group-tags-member.yaml'🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In packages/go/openapi/src/paths/asset-isolation.asset-group-tags.search.yaml
between lines 40 and 62, the response arrays for tags, selectors, and members
lack the minItems property, causing generated clients to expect at least one
item. Add minItems: 0 to each array definition to allow empty arrays for no
search results. Also, remove any trailing spaces on lines 51 and 59 to clean up
the YAML formatting.
Description
Adding an endpoint for the ability to search tags or labels, selectors and members
Motivation and Context
Resolves BED-5784
Why is this change required? What problem does it solve?
This change will allow searching on the privileged zones page whether on the tiers or labels page.
How Has This Been Tested?
New unit tests have been added for new functionality and existing unit tests pass. (will post screenshots)
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit