feat: OG DB - List/Query Extension Entries BED-6778 #2091
Conversation
AD7ZJ
added
enhancement
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-06-06T23:12:14.181ZApplied to files:
📚 Learning: 2025-11-25T22:11:53.518ZApplied to files:
📚 Learning: 2025-07-10T14:33:20.317ZApplied to files:
🧬 Code graph analysis (2)cmd/api/src/database/graphschema.go (3)
cmd/api/src/database/mocks/db.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). (4)
🔇 Additional comments (3)
Comment |
…eleted_at is null." This reverts commit c9da85e.
…pecterOps/BloodHound into BED-6778--OG-db-list-extensions
AD7ZJ
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/database/graphschema_integration_test.go (3)
80-119: Consider making this integration test parallel for consistencyOther integration tests in this file call
t.Parallel()at the top; this one does not. Unless this test must serialize due to shared state, consider addingt.Parallel()for consistency and to keep overall suite runtime down.
120-169: Make pagination assertions independent of implicit DB row orderingThe skip/limit subtests assert specific element positions (
"charlie"at index 0 afterskip=2,"bob"at index 1 forlimit=2) while passing an emptySort{}. That couples the test to whatever default ordering the query/DB happens to use (often insertion order, but not guaranteed across engines or future refactors).Consider passing an explicit sort (for example, by
nameordisplay_name) in these pagination tests and basing expectations on that ordering. This will make the tests more robust and self-document the intended default behavior.
171-175: Relax brittle assertion on the exact Postgres error stringThe bogus-filtering subtest asserts the full error message string including driver/engine formatting and SQLSTATE:
require.Equal(t, "ERROR: column \"nonexitentcolumn\" does not exist (SQLSTATE 42703)", err.Error())This is quite brittle: minor changes in driver, Postgres version, or localization could break the test even though the behavior is still correct. Also, the column name is misspelled in both the query and expected string.
Prefer a looser check such as
require.ErrorContains(t, err, "column \"nonexitentcolumn\" does not exist")or asserting on the SQLSTATE code/type if you surface that, and optionally fix the spelling for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/graphschema_integration_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
80-82: Consider addingt.Parallel()for consistency.Other tests in this file (
TestDatabase_CreateAndGetGraphSchemaExtensions,TestBloodhoundDB_SchemaNodeKind_CRUD, etc.) uset.Parallel(). Adding it here would maintain consistency and enable parallel test execution.func TestDatabase_GetGraphSchemaExtensions(t *testing.T) { + t.Parallel() suite := setupIntegrationTestSuite(t)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/graphschema.go(2 hunks)cmd/api/src/database/graphschema_integration_test.go(6 hunks)cmd/api/src/database/mocks/db.go(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
🧬 Code graph analysis (2)
cmd/api/src/database/graphschema_integration_test.go (2)
cmd/api/src/model/filter.go (4)
SQLFilter(98-101)Sort(486-486)AscendingSortDirection(466-466)DescendingSortDirection(467-467)cmd/api/src/database/db.go (2)
ErrDuplicateSchemaNodeKindName(56-56)ErrDuplicateSchemaEdgeKindName(58-58)
cmd/api/src/database/mocks/db.go (3)
cmd/api/src/auth/model.go (1)
Context(174-178)cmd/api/src/model/graphschema.go (5)
SchemaNodeKind(45-55)SchemaNodeKind(58-60)GraphSchemaExtensions(19-19)SchemaEdgeKind(77-83)SchemaEdgeKind(85-87)cmd/api/src/model/filter.go (2)
SQLFilter(98-101)Sort(486-486)
🪛 golangci-lint (2.5.0)
cmd/api/src/database/graphschema_integration_test.go
[major] 236-236: : # github.com/specterops/bloodhound/cmd/api/src/database_test [github.com/specterops/bloodhound/cmd/api/src/database.test]
cmd/api/src/database/graphschema_integration_test.go:236:43: testSuite.BHDatabase.GetSchemaNodeKindByID undefined (type *database.BloodhoundDB has no field or method GetSchemaNodeKindByID, but does have method GetSchemaNodeKindById)
cmd/api/src/database/graphschema_integration_test.go:240:32: testSuite.BHDatabase.GetSchemaNodeKindByID undefined (type *database.BloodhoundDB has no field or method GetSchemaNodeKindByID, but does have method GetSchemaNodeKindById)
(typecheck)
⏰ 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 (3)
cmd/api/src/database/graphschema.go (1)
98-156: Implementation looks good and follows established patterns.The
GetGraphSchemaExtensionsmethod correctly implements:
- SQL filter application with parameterized queries
- Default ascending sort by
idwhen no sort is specified- Pagination with
LIMIT/OFFSET- Separate count query for total rows when pagination is used
The use of
ToSqlString()for sort direction aligns with the prior review discussion about creating a helper function.cmd/api/src/database/graphschema_integration_test.go (1)
120-176: Comprehensive test coverage for the newGetGraphSchemaExtensionsfunction.The test scenarios cover:
- No filtering/sorting (baseline)
- Exact match filtering
- Fuzzy/ILIKE filtering
- Ascending and descending sort
- Skip (offset) pagination
- Limit pagination
- Error path with invalid column
cmd/api/src/database/mocks/db.go (1)
1635-1649: Generated mock code correctly reflects the new interface method.The
GetGraphSchemaExtensionsmock method and its recorder are correctly generated to match the interface signature.
AD7ZJ
force-pushed
the
BED-6778--OG-db-list-extensions
branch
from
512b630 to
e3e6204
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/database/graphschema.go (1)
98-156: Implementation is sound; consider tightening the doc comment aroundtotalRowCountThe query assembly (filter via
SQLFilter, default sort onid ASC,SortDirection.ToSqlString(), and optionalLIMIT/OFFSET) looks correct and mirrors existing patterns, and theCOUNT(*)for paginated calls correctly returns the total matching rows ignoring pagination.The only nit is the docstring, which currently reads a bit ambiguously about what the integer represents. Consider clarifying that it’s the total number of rows matching the filter, independent of
skip/limit, e.g.:-// GetGraphSchemaExtensions gets all the rows from the extensions table that match the given SQLFilter. It returns a slice of GraphSchemaExtension structs -// populated with the data, as well as an integer giving the total number of rows returned by the query (excluding any given pagination) +// GetGraphSchemaExtensions returns rows from the extensions table that match the given SQLFilter, optionally paginated via skip and limit. +// It returns the matching extensions and the total number of rows matching the filter (ignoring skip and limit). func (s *BloodhoundDB) GetGraphSchemaExtensions(ctx context.Context, extensionSqlFilter model.SQLFilter, sort model.Sort, skip, limit int) (model.GraphSchemaExtensions, int, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/database/graphschema.go(2 hunks)cmd/api/src/database/graphschema_integration_test.go(1 hunks)cmd/api/src/database/mocks/db.go(1 hunks)cmd/api/src/model/filter.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/model/filter.go
- cmd/api/src/database/graphschema_integration_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema.go
🧬 Code graph analysis (1)
cmd/api/src/database/mocks/db.go (2)
cmd/api/src/model/filter.go (2)
SQLFilter(98-101)Sort(518-518)cmd/api/src/model/graphschema.go (1)
GraphSchemaExtensions(19-19)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-go-unit-tests
🔇 Additional comments (2)
cmd/api/src/database/graphschema.go (1)
27-46: Interface surface forGetGraphSchemaExtensionslooks consistentThe new method on
OpenGraphSchemamatches the implementation and mock signatures (SQLFilter,Sort,skip/limit, slice + total count + error) and is consistent with other filtered/paginated getters in this interface.cmd/api/src/database/mocks/db.go (1)
1635-1649: NewGetGraphSchemaExtensionsmock wiring is correctThe mock and recorder for
GetGraphSchemaExtensionsfollow the standard gomock pattern and match the database interface signature, so tests can safely stub and assert the new method.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
80-81: Consider addingt.Parallel()for test isolation.Other integration tests in this file use
t.Parallel()(see line 31, 188, 318, 429). While this test doesn't share state with those tests due to unique extension names, addingt.Parallel()would maintain consistency and allow concurrent execution.Apply this diff:
func TestDatabase_GetGraphSchemaExtensions(t *testing.T) { + t.Parallel() suite := setupIntegrationTestSuite(t)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/database/graphschema.go(2 hunks)cmd/api/src/database/graphschema_integration_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
🧬 Code graph analysis (1)
cmd/api/src/database/graphschema.go (4)
cmd/api/src/model/graphschema.go (3)
GraphSchemaExtensions(19-19)GraphSchemaExtension(21-28)GraphSchemaExtension(30-32)packages/go/schemagen/csgen/models.go (1)
Operator(64-64)cmd/api/src/database/db.go (1)
BloodhoundDB(191-194)cmd/api/src/database/helper.go (1)
CheckError(26-32)
⏰ 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
…r from service layer This should still be refactored further, but it gives an idea of what we can shoot for.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/api/src/database/helper.go (1)
171-194: Add a default case to handle unexpected sort directions defensively.The switch statement handles
AscendingSortDirection,DescendingSortDirection, andInvalidSortDirection, but lacks adefaultcase. If a newSortDirectionconstant is added in the future, the function would silently produce an empty direction string.switch item.Direction { case model.AscendingSortDirection: dirString = "ASC" case model.DescendingSortDirection: dirString = "DESC" case model.InvalidSortDirection: return "", ErrInvalidSortDirection + default: + return "", ErrInvalidSortDirection }cmd/api/src/database/graphschema_integration_test.go (1)
80-83: Missingt.Parallel()call unlike other tests in this file.Other test functions in this file (
TestDatabase_CreateAndGetGraphSchemaExtensions,TestBloodhoundDB_SchemaNodeKind_CRUD, etc.) includet.Parallel()at the start. This test is missing it, which is inconsistent.If the omission is intentional due to potential test interference, please add a comment explaining why. Otherwise, add
t.Parallel()for consistency.func TestDatabase_GetGraphSchemaExtensions(t *testing.T) { + t.Parallel() suite := setupIntegrationTestSuite(t) defer teardownIntegrationTestSuite(t, &suite)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/graphschema.go(2 hunks)cmd/api/src/database/graphschema_integration_test.go(1 hunks)cmd/api/src/database/helper.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema.go
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
cmd/api/src/database/graphschema.go
🧬 Code graph analysis (2)
cmd/api/src/database/graphschema.go (4)
cmd/api/src/model/filter.go (4)
Filters(485-485)Sort(518-518)SortItem(513-516)AscendingSortDirection(491-491)cmd/api/src/model/graphschema.go (3)
GraphSchemaExtensions(19-19)GraphSchemaExtension(21-28)GraphSchemaExtension(30-32)cmd/api/src/database/db.go (1)
BloodhoundDB(191-194)cmd/api/src/database/helper.go (1)
CheckError(39-45)
cmd/api/src/database/helper.go (2)
cmd/api/src/model/filter.go (12)
NullString(57-57)GreaterThan(41-41)GreaterThanOrEquals(42-42)LessThan(43-43)LessThanOrEquals(44-44)NotEquals(46-46)ApproximatelyEquals(47-47)FilterOr(510-510)Sort(518-518)AscendingSortDirection(491-491)DescendingSortDirection(492-492)InvalidSortDirection(490-490)packages/go/schemagen/csgen/models.go (3)
Operator(64-64)OperatorEquals(79-79)SyntaxNode(226-229)
⏰ 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
🔇 Additional comments (7)
cmd/api/src/database/helper.go (2)
32-37: Well-structured additions for SQL building helpers.The
sqlFilterstruct andErrInvalidSortDirectionerror are appropriately scoped as package-private and public respectively. This aligns with past review feedback to keep helpers private and move them to helper.go.
188-188: Add column name validation tobuildSQLSortto prevent SQL injection.The
item.Columnvalue is directly concatenated into the ORDER BY clause without validation. WhilebuildSQLFilteruses safe identifier handling,buildSQLSortconcatenates user input directly. Although the API layer (ParseSortParameters) validates columns before calling this function,GetGraphSchemaExtensionscan be called directly from other code paths without validation.Consider validating column names against a whitelist within
buildSQLSort:var allowedColumns = map[string]bool{ "id": true, "name": true, "created_at": true, // ... other sortable columns } if !allowedColumns[item.Column] { return "", fmt.Errorf("unsupported sort column: %q", item.Column) } sortColumns = append(sortColumns, item.Column+" "+dirString)This ensures defense-in-depth by validating at the database layer rather than relying solely on upstream API validation.
cmd/api/src/database/graphschema_integration_test.go (1)
120-232: Good test coverage for the newGetGraphSchemaExtensionsfunctionality.The test cases thoroughly cover filtering (single, multiple, fuzzy), sorting (ascending, descending), and pagination (skip, limit), as well as error handling for invalid columns.
Consider adding test cases for:
- Invalid sort direction (to verify
ErrInvalidSortDirectionis returned)- OR filter set operator (
model.FilterOr)These would further strengthen the test suite but are not blockers.
cmd/api/src/database/graphschema.go (4)
30-30: Interface signature looks correct.The new
GetGraphSchemaExtensionsmethod signature follows the established patterns in the interface, accepting filters, sort, skip, and limit parameters and returning the result slice, count, and error.
114-121: Good: Default sort ensures consistent pagination.The default sort by
idwhen no sort is specified is a sensible default that ensures deterministic ordering for pagination. The comment clearly explains the reasoning.
123-129: Pagination implementation is correct.The LIMIT and OFFSET clauses are properly constructed using
%dformat specifier for integer values, which is safe from SQL injection.
135-144: Main query construction is well-structured.The SELECT query properly uses parameterized queries via
filter.paramsfor filter values, preventing SQL injection for user-provided values. The query components are cleanly organized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
80-81: Consider adding t.Parallel() for consistency and performance.The test
TestDatabase_GetGraphSchemaExtensionsis not marked witht.Parallel(), unlikeTestBloodhoundDB_SchemaNodeKind_CRUDat line 235. Based on learnings, integration tests can be parallelized when self-contained. However, if parallel tests create extensions with the same names ("adam", "bob", "charlie", "david"), they may conflict due to unique constraints on extension names.Consider either:
- Adding
t.Parallel()and using more unique names (e.g., with random suffixes or test-specific prefixes)- Documenting why parallelization was intentionally omitted
Based on learnings, table-driven tests in BloodHound can be parallelized when using isolated setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/graphschema.go(2 hunks)cmd/api/src/database/graphschema_integration_test.go(1 hunks)cmd/api/src/database/mocks/db.go(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/graphschema.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema.go
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
cmd/api/src/database/graphschema.go
🧬 Code graph analysis (2)
cmd/api/src/database/graphschema_integration_test.go (3)
packages/go/graphschema/common/common.go (1)
DisplayName(54-54)cmd/api/src/model/filter.go (5)
Filters(485-485)Sort(518-518)ApproximatelyEquals(47-47)AscendingSortDirection(491-491)DescendingSortDirection(492-492)packages/go/schemagen/csgen/models.go (1)
Operator(64-64)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (5)
SchemaNodeKind(45-55)SchemaNodeKind(58-60)GraphSchemaExtensions(19-19)SchemaEdgeKind(77-83)SchemaEdgeKind(85-87)
⏰ 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/database/graphschema.go (1)
98-160: LGTM!The implementation of
GetGraphSchemaExtensionsis well-structured:
- Proper use of
buildSQLFilterandbuildSQLSorthelpers for SQL safety- Default sort by id ensures consistent pagination (lines 115-117)
- COUNT query correctly avoids duplicate filters (lines 147-149)
- Appropriate error handling throughout
cmd/api/src/database/mocks/db.go (1)
1635-1649: LGTM!The new
GetGraphSchemaExtensionsmock correctly matches the interface signature and follows gomock conventions.
wes-mil
left a comment
wes-mil
left a comment
There was a problem hiding this comment.
Read through buildSQLFilter like 4 times and still a little confused... I'm just going to trust that it's behaving properly
It is pretty much a copy/paste from BuildSQLFilter() in model/filter.go. |
Description
Add a database handler to list extensions with filtering, sorting, and pagination parameters.
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-6778
How Has This Been Tested?
A unit test has been added that verifies functionality of the new function.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.