BED-5278: Tiering list history records #1583
Conversation
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
2 times, most recently
from
3c6a0a6 to
92bce29
Compare
|
""" WalkthroughA new API endpoint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
Client->>API: GET /api/v2/asset-group-tags/history?filters...
API->>API: Parse & validate query parameters
API->>DB: GetAssetGroupHistoryRecords(filter, sort, skip, limit)
DB-->>API: Return filtered, sorted, paginated records and count
API-->>Client: 200 OK with paginated history data
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (11)
🧰 Additional context used🧠 Learnings (2)📓 Common learningscmd/api/src/database/assetgrouphistory_test.go (4)🧬 Code Graph Analysis (1)cmd/api/src/database/assetgrouphistory_test.go (3)
⏰ 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)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 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 (
|
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
3 times, most recently
from
16a2652 to
6450c89
Compare
AD7ZJ
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: 3
🧹 Nitpick comments (2)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml (1)
17-87: Comprehensive API endpoint definition with minor formatting issues.The OpenAPI specification correctly defines all query parameters for filtering and includes appropriate response schemas. However, there are YAML formatting issues that should be addressed.
Apply these formatting fixes:
parameters: - - name: created_at + - name: created_at in: query required: false schema: $ref: './../schemas/api.params.predicate.filter.time.yaml' responses: - 200: + 200: description: OK content: application/json: schema: $ref: './../schemas/model.asset-group-tags-history.yaml' - $ref: './../responses/not-found.yaml' + $ref: './../responses/not-found.yaml'packages/go/openapi/doc/openapi.json (1)
17770-17820: Extract item schema and declare required properties.The inline object under
data.itemscould be refactored into a reusable component (e.g.model.asset-group-tag-history-item) to improve maintainability. Also consider adding arequiredarray onmodel.asset-group-tags-historyforcount,limit,skip, anddatato enforce consistent responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(1 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(1 hunks)cmd/api/src/database/assetgrouphistory.go(1 hunks)cmd/api/src/database/assetgrouphistory_test.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(10 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/model/assetgrouphistory.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
[warning] 29-29: wrong indentation: expected 4 but found 2
(indentation)
[error] 70-70: trailing spaces
(trailing-spaces)
[warning] 83-83: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (15)
packages/go/openapi/src/openapi.yaml (1)
355-356: LGTM! Endpoint follows established patterns.The new history endpoint is correctly placed within the asset isolation section and follows the same
$refpattern used by other endpoints in the specification.cmd/api/src/api/registration/v2.go (1)
176-176: LGTM! Route registration follows security best practices.The new history endpoint is properly protected with the same feature flag and permissions as other asset group tag endpoints, ensuring consistent access control.
cmd/api/src/database/assetgrouptags_test.go (1)
71-71: LGTM! Consistent database method signature updates.All calls to
GetAssetGroupHistoryRecordshave been systematically updated with the new parameters for filtering, sorting, and pagination. The empty filter, ascending sort, and zero pagination values maintain the original behavior while supporting the enhanced method signature needed for the new history endpoint.Also applies to: 224-224, 291-291, 341-341, 531-531, 585-585, 651-651, 673-673, 981-981, 995-995
cmd/api/src/api/v2/assetgrouptags.go (1)
735-774: Well-implemented paginated history endpoint.The new
GetAssetGroupTagHistorymethod follows established patterns in the codebase for filter validation, pagination, and error handling. The implementation correctly validates query filters against the model's allowed predicates and returns appropriate HTTP status codes for various error conditions.cmd/api/src/model/assetgrouphistory.go (1)
68-83: Solid implementation of filter metadata methods.The
IsStringColumnandValidFiltersmethods provide essential metadata for the filtering system. The operator restrictions are appropriately applied based on field types (comparison operators for dates, equality/approximate matching for strings, equality only for integers).packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml (1)
1-49: Comprehensive OpenAPI schema for paginated history response.The schema correctly defines the response structure with proper pagination metadata and detailed history record objects. Field types and formats are appropriately specified, and the structure aligns with the Go model implementation.
cmd/api/src/database/assetgrouphistory.go (2)
30-30: Updated interface signature supports enhanced querying capabilities.The new method signature properly adds filtering, sorting, and pagination parameters needed for the history endpoint functionality.
38-76: Secure and well-structured database implementation.The implementation correctly handles dynamic SQL construction with proper parameterization to prevent SQL injection. The logic for building WHERE, ORDER BY, LIMIT, and OFFSET clauses is sound and follows established patterns.
cmd/api/src/database/assetgrouphistory_test.go (5)
44-57: LGTM! Good test setup with comprehensive verification.The test properly creates a record and verifies all fields are correctly stored and retrieved with the new method signature.
59-68: Excellent test coverage for multiple record scenarios.The test creates records with different actions (
DeleteSelector,CreateTag,DeleteTag) which provides good coverage for the filtering functionality that follows.
70-79: Good verification of sort functionality.The test properly verifies both ascending and descending sort orders by checking the first and last records in each case.
81-92: Comprehensive pagination testing.The test covers both
limitandskipparameters correctly, ensuring the pagination functionality works as expected.
94-100: Well-implemented SQL filter testing.The test uses a proper SQL filter with parameterized query (
action = ?) and verifies that exactly one record matching the filter criteria is returned.cmd/api/src/database/mocks/db.go (1)
1046-1058: Appropriate mock method signature update.The generated mock correctly reflects the updated interface with new parameters for SQL filtering (
sqlFilter model.SQLFilter), sort direction (sortDirectionAscending bool), and pagination (skip, limit int). This aligns with the enhanced functionality being added.cmd/api/src/api/v2/assetgrouptags_test.go (1)
2255-2393: Excellent test coverage with a minor clarification needed.This test function provides comprehensive coverage of the new asset group tag history endpoint with proper error handling, filtering, and pagination scenarios. The test structure follows established patterns and uses appropriate mock expectations.
However, I notice in the "Success" test case (line 2363) you're setting
api.URIPathVariableAssetGroupTagIDas a URL variable, but based on the PR summary, this endpoint appears to be for general asset group tag history records rather than being filtered by a specific tag ID. Could you clarify if this URL variable is actually used by the endpoint, or if it should be removed from this test case?#!/bin/bash # Description: Verify if the GetAssetGroupTagHistory endpoint uses asset group tag ID as a URL parameter # Check the route registration for this endpoint rg -A 5 -B 5 "GetAssetGroupTagHistory" # Check if the endpoint path includes asset group tag ID parameter rg "asset-group-tags.*history"
| "/api/v2/asset-group-tags/history": { | ||
| "parameters": [ | ||
| { | ||
| "$ref": "#/components/parameters/header.prefer" | ||
| } | ||
| ], | ||
| "get": { |
There was a problem hiding this comment.
Missing security definition for the endpoint.
This new route doesn’t include a security section—unlike other v2 endpoints that enforce bearer authentication. Add a security block (e.g., bearerAuth: []) at the path or operation level to protect this API.
🤖 Prompt for AI Agents
In packages/go/openapi/doc/openapi.json around lines 6132 to 6138, the new
endpoint /api/v2/asset-group-tags/history is missing a security definition. Add
a security block with bearerAuth (e.g., "security": [{"bearerAuth": []}]) either
at the path level or inside the get operation to enforce bearer token
authentication consistent with other v2 endpoints.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/database/assetgrouphistory_test.go (3)
70-79: Consider testing sort order more reliably.The sorting tests assume that the creation order matches the timestamp-based sort order. This could be flaky in concurrent environments or systems with low timestamp precision.
Consider explicitly verifying the timestamp order instead of relying on creation sequence:
// verify ascending sort -require.Equal(t, model.AssetGroupHistoryActionCreateSelector, records[0].Action) -require.Equal(t, model.AssetGroupHistoryActionDeleteTag, records[3].Action) +// Verify ascending order by timestamp +for i := 1; i < len(records); i++ { + require.True(t, records[i-1].CreatedAt.Before(records[i].CreatedAt) || records[i-1].CreatedAt.Equal(records[i].CreatedAt)) +}
61-64: Consider using more descriptive test data.The hardcoded asset group tag IDs (1, 2) could be made more explicit or use constants for better test readability.
-err = dbInst.CreateAssetGroupHistoryRecord(testCtx, testActor.ID.String(), testActor.EmailAddress.ValueOrZero(), testTarget, model.AssetGroupHistoryActionCreateTag, 2, null.String{}, null.String{}) +testAssetGroupTag2 := 2 +err = dbInst.CreateAssetGroupHistoryRecord(testCtx, testActor.ID.String(), testActor.EmailAddress.ValueOrZero(), testTarget, model.AssetGroupHistoryActionCreateTag, testAssetGroupTag2, null.String{}, null.String{})
95-101: Consider adding edge case testing for filtering.The SQL filter test only covers a basic positive case. Consider adding tests for edge cases like invalid filters, empty results, or malformed SQL.
Add additional filter test cases:
// Test empty filter results records, _, err = dbInst.GetAssetGroupHistoryRecords(testCtx, model.SQLFilter{SQLString: "action = ?", Params: []any{"NonExistentAction"}}, true, 0, 0) require.NoError(t, err) require.Len(t, records, 0) // Test multiple conditions records, _, err = dbInst.GetAssetGroupHistoryRecords(testCtx, model.SQLFilter{SQLString: "action = ? AND asset_group_tag_id = ?", Params: []any{model.AssetGroupHistoryActionCreateSelector, testAssetGroupTag}}, true, 0, 0) require.NoError(t, err) require.Len(t, records, 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(1 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(1 hunks)cmd/api/src/database/assetgrouphistory.go(1 hunks)cmd/api/src/database/assetgrouphistory_test.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(10 hunks)cmd/api/src/database/mocks/db.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/go/openapi/src/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/api/src/api/registration/v2.go
- cmd/api/src/database/assetgrouptags_test.go
- cmd/api/src/api/v2/assetgrouptags.go
- cmd/api/src/database/assetgrouphistory.go
- cmd/api/src/api/v2/assetgrouptags_test.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/database/mocks/db.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (4)
cmd/api/src/database/assetgrouphistory_test.go (4)
44-44: LGTM: Test data initialization updated correctly.The change from
DeleteSelectortoCreateSelectoraligns well with the expanded test structure that now tests multiple action types in sequence.
81-86: LGTM: Pagination testing is comprehensive.The limit and total count verification properly tests the pagination functionality.
88-93: LGTM: Skip functionality tested correctly.The skip test properly verifies that the correct records are returned when skipping the first 2 records.
47-47: Verify the new method signature is correctly implemented.The method signature has been updated to include filtering, sorting, and pagination parameters. The new signature appears to be
GetAssetGroupHistoryRecords(ctx, filter, ascending, skip, limit)returning(records, totalCount, error).#!/bin/bash # Description: Verify the GetAssetGroupHistoryRecords method signature matches the test usage # Expected: Method should accept SQLFilter, bool, int, int parameters and return records, int, error # Search for the method definition ast-grep --pattern $'func ($DB) GetAssetGroupHistoryRecords($$$) ($$$) { $$$ }'
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/assetgrouptags.go(1 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(1 hunks)cmd/api/src/database/assetgrouphistory.go(2 hunks)cmd/api/src/database/assetgrouphistory_test.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(13 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/model/assetgrouphistory.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/api/src/database/assetgrouptags_test.go
- cmd/api/src/api/v2/assetgrouptags.go
- cmd/api/src/model/assetgrouphistory.go
- cmd/api/src/database/mocks/db.go
- cmd/api/src/api/v2/assetgrouptags_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/api/src/database/assetgrouphistory_test.go (2)
cmd/api/src/model/assetgrouphistory.go (4)
AssetGroupHistoryActionCreateSelector(32-32)AssetGroupHistoryActionDeleteSelector(34-34)AssetGroupHistoryActionCreateTag(28-28)AssetGroupHistoryActionDeleteTag(30-30)cmd/api/src/model/filter.go (4)
SQLFilter(93-96)Sort(384-384)AscendingSortDirection(375-375)DescendingSortDirection(376-376)
cmd/api/src/database/assetgrouphistory.go (4)
cmd/api/src/model/filter.go (3)
SQLFilter(93-96)Sort(384-384)DescendingSortDirection(376-376)cmd/api/src/model/assetgrouphistory.go (3)
AssetGroupHistory(38-48)AssetGroupHistory(50-52)AssetGroupHistoryAction(25-25)cmd/api/src/database/db.go (1)
BloodhoundDB(176-179)cmd/api/src/database/helper.go (1)
CheckError(26-32)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (7)
cmd/api/src/database/assetgrouphistory_test.go (2)
44-57: Well-structured test setup with comprehensive record creation.The test correctly creates multiple records with different actions and verifies the basic functionality. The change from
DeleteSelectortoCreateSelectorfor the initial record is consistent with the test flow.
59-101: Excellent comprehensive testing of enhanced functionality.The test thoroughly covers all the new features:
- Sorting in both ascending and descending order
- Pagination with limit and skip parameters
- SQL filtering with parameterized queries
- Total count verification
The test scenarios are well-designed and the assertions properly validate the expected behavior for each case.
cmd/api/src/database/assetgrouphistory.go (5)
22-22: Good addition of strings import for SQL construction.The strings package is properly imported to support the new sorting functionality.
39-49: Solid method signature and filter handling.The enhanced method signature properly supports the new filtering, sorting, and pagination requirements. The filter handling correctly prepends "WHERE " when a filter is provided.
51-58: Well-implemented dynamic sorting logic.The sort column construction properly handles multiple sort fields and directions, building a clean ORDER BY clause.
84-90: Efficient count query optimization.The implementation correctly optimizes by only executing the count query when pagination is applied, using the result set length otherwise. This is a good performance consideration.
68-78: ```bash
#!/bin/bashShow context around sortString and sqlFilter in assetgrouphistory.go
echo "=== assetgrouphistory DB file snippet ==="
sed -n '1,120p' cmd/api/src/database/assetgrouphistory.goLocate any SortableFields definitions across models
echo "=== Searching for SortableFields definitions ==="
rg -n "SortableFields" --type goSearch for generic sort builder functions
echo "=== Searching for sort builder usage ==="
rg -n "sortString" -n cmd/api/src/database/assetgrouphistory.go
rg -n "Sort" -n cmd/api/src/database/assetgrouphistory.go</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml (1)
33-34: Remove stray trailing whitespaceLines 33 and 78 contain trailing spaces, again caught by
yamllint. These will break “strict” linters in CI.- description:␠ + description: … - 200: ␠ + 200:Also applies to: 78-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/openapi/doc/openapi.json(3 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
[warning] 29-29: wrong indentation: expected 4 but found 2
(indentation)
[error] 33-33: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[warning] 91-91: wrong indentation: expected 6 but found 8
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (1)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml (1)
31-36: Clarifysort_byschema to restrict valid valuesYou note in the description that only
created_atis sortable, but the referenced generic sort-by schema likely exposes more fields. Either tighten the schema here (e.g.,enum: [created_at]) or document that onlycreated_atis honored to avoid confusing consumers.
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
Outdated
Show resolved
Hide resolved
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
Outdated
Show resolved
Hide resolved
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
Outdated
Show resolved
Hide resolved
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
from
75072bd to
7891430
Compare
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
Outdated
Show resolved
Hide resolved
packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml
Outdated
Show resolved
Hide resolved
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
from
7891430 to
e39d446
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/database/assetgrouphistory_test.go (1)
32-108: Consider adding t.Parallel() for better test performance.Based on the retrieved learnings, integration tests can benefit from function-level parallelization unless they have specific dependencies on shared state. Since this test uses a fresh database instance from
integration.SetupDB(t), it might be safe to addt.Parallel()at the function level.Add at the beginning of the test function:
func TestDatabase_CreateAndGetAssetGroupHistory(t *testing.T) { + t.Parallel() var ( dbInst = integration.SetupDB(t)Note: Only add this if the integration test setup supports parallel execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/assetgrouptags.go(1 hunks)cmd/api/src/database/assetgrouphistory.go(2 hunks)cmd/api/src/database/assetgrouphistory_test.go(1 hunks)packages/go/openapi/doc/openapi.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/api/src/database/assetgrouphistory.go
- cmd/api/src/api/v2/assetgrouptags.go
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🧠 Learnings (1)
cmd/api/src/database/assetgrouphistory_test.go (3)
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2197-2197
Timestamp: 2025-06-17T22:37:50.192Z
Learning: In Go tests using table-driven patterns, t.Parallel() at the function level allows test functions to run in parallel with other test functions, which is generally beneficial. However, when subtests contain deferred goroutines, the individual t.Run() calls should not use t.Parallel() to avoid flakiness. The comment "Tests will be flaky if run in parallel due to use of go routine in deferred function" specifically refers to subtest parallelization, not function-level parallelization.
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 (1)
cmd/api/src/database/assetgrouphistory_test.go (3)
cmd/api/src/model/assetgrouphistory.go (4)
AssetGroupHistoryActionCreateSelector(32-32)AssetGroupHistoryActionDeleteSelector(34-34)AssetGroupHistoryActionCreateTag(28-28)AssetGroupHistoryActionDeleteTag(30-30)cmd/api/src/model/filter.go (4)
SQLFilter(93-96)Sort(384-384)AscendingSortDirection(375-375)DescendingSortDirection(376-376)packages/go/graphschema/common/common.go (1)
⏰ 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 (5)
cmd/api/src/database/assetgrouphistory_test.go (5)
44-44: Consider the test setup sequence for better readability.The initial record creation uses
CreateSelectoraction, which aligns with the test logic. However, consider documenting why this specific action was chosen as the first record in the test sequence.
47-59: Good verification of record fields.The subtest thoroughly verifies all expected fields of the retrieved record, including proper handling of null values and timestamp validation.
85-91: Verify limit functionality correctly.The limit test properly verifies that when limiting to 2 records, exactly 2 records are returned while the total count remains 4. This correctly tests the pagination functionality.
93-99: Verify skip functionality with proper assertions.The skip test correctly verifies that when skipping the first 2 records, the remaining records are returned in the expected order. The assertions check the specific actions of the remaining records.
101-107: SQL filter test uses safe parameterized queries.The SQL filter test correctly uses parameterized queries to prevent SQL injection and properly tests the filtering functionality by action type.
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
from
af8c768 to
91395d1
Compare
| } | ||
| sortColumns = append(sortColumns, fmt.Sprintf("%s %s", item.Column, dirString)) | ||
| } | ||
| sortString := "ORDER BY " + strings.Join(sortColumns, ", ") |
There was a problem hiding this comment.
This variable assumes the sortColumns slice is never empty. This likely needs to be built out similar to the skipLimitString where if nothing is supplied it will be an empty string
There was a problem hiding this comment.
Would also benefit from a db test for the empty sortColumns argument to prevent regression as well
There was a problem hiding this comment.
That's a good catch, thanks!
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
Outdated
Show resolved
Hide resolved
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
from
91395d1 to
7ad0f03
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml (1)
43-45:format: email– same issue flagged previouslyA prior review already requested the email format hint; it has not been applied. Add the format so generators recognise the field as an RFC-5322 email.
email: $ref: './null.string.yaml' + format: email
🧹 Nitpick comments (1)
packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml (1)
49-51: Consider specifyingformat: int64forasset_group_tag_idfor consistency
idis declared withformat: int64; declaringasset_group_tag_idthe same way ensures uniform typing across record identifiers and avoids code-gen discrepancies.- asset_group_tag_id: - type: integer + asset_group_tag_id: + type: integer + format: int64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/api/src/api/registration/v2.go(1 hunks)cmd/api/src/api/v2/assetgrouptags.go(1 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(1 hunks)cmd/api/src/database/assetgrouphistory.go(2 hunks)cmd/api/src/database/assetgrouphistory_test.go(1 hunks)cmd/api/src/database/assetgrouptags_test.go(13 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/model/assetgrouphistory.go(2 hunks)packages/go/openapi/doc/openapi.json(2 hunks)packages/go/openapi/src/openapi.yaml(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml(1 hunks)packages/go/openapi/src/schemas/model.asset-group-tags-history.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/go/openapi/src/openapi.yaml
- cmd/api/src/database/assetgrouptags_test.go
- cmd/api/src/database/assetgrouphistory_test.go
- cmd/api/src/database/assetgrouphistory.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.history.yaml
- cmd/api/src/model/assetgrouphistory.go
- cmd/api/src/api/v2/assetgrouptags.go
- cmd/api/src/api/registration/v2.go
- cmd/api/src/database/mocks/db.go
- packages/go/openapi/doc/openapi.json
- cmd/api/src/api/v2/assetgrouptags_test.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/schemas/model.asset-group-tags-history.yaml (2)
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: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
⏰ 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-analysis
- GitHub Check: run-tests
AD7ZJ
force-pushed
the
BED-5278--Tiering-list-history-recs
branch
from
7ad0f03 to
fef08b6
Compare
AD7ZJ
dismissed
ALCooper12’s stale review
Product and team was OK with it as-is, so lets' run with this for now :)
Description
Creates an endpoint to return history records associated with the asset group tag feature.
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-5278
How Has This Been Tested?
Tested locally using bruno. Unit tests have also been added to verify the endpoint and database functions.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation