BloodHound icon indicating copy to clipboard operation
BloodHound copied to clipboard
verified publisher icon SpecterOps

BED-6778: OG DB - List/Query Extension Entries

Open AD7ZJ opened this issue 1 month ago • 1 comments

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

  • New feature (non-breaking change which adds functionality)

Checklist:

  • [x] I have met the contributing prerequisites
    • Assigned myself to this PR
    • Added the appropriate labels
    • Associated an issue: https://github.com/SpecterOps/BloodHound/issues/672
    • Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
  • [x] I have ensured that related documentation is up-to-date
    • Open API docs
    • Code comments (GoDocs / JSDocs)
  • [x] I have followed proper test practices
    • Added/updated tests to cover my changes
    • All new and existing tests passed

AD7ZJ avatar Nov 20 '25 17:11 AD7ZJ

Walkthrough

This PR introduces a new GetGraphSchemaExtensions method for retrieving and filtering graph schema extensions from the database. The change adds SQL-level filtering, sorting, and pagination capabilities through new helper functions (buildSQLFilter, buildSQLSort) and extends the database interface, implementation, mocks, model types, and integration tests.

Changes

Cohort / File(s) Change Summary
Database Interface & Implementation
cmd/api/src/database/graphschema.go		 Added public method GetGraphSchemaExtensions to OpenGraphSchema interface and implemented in BloodhoundDB with SQL-level filtering, sorting (defaulting to id), optional pagination, and total row count calculation.
Database Mocks
cmd/api/src/database/mocks/db.go		 Added mock method GetGraphSchemaExtensions and corresponding recorder for testing support.
Model Types
cmd/api/src/model/graphschema.go		 Added new exported type GraphSchemaExtensions as a slice of GraphSchemaExtension.
Database Helpers
cmd/api/src/database/helper.go		 Added ErrInvalidSortDirection error variable; introduced sqlFilter struct and helper functions buildSQLFilter (constructs PostgreSQL-compatible WHERE clauses with operator and null handling) and buildSQLSort (generates ORDER BY clauses with validation). Modified CheckError to return nil for non-NotFound errors.
Integration Tests
cmd/api/src/database/graphschema_integration_test.go		 Added comprehensive TestDatabase_GetGraphSchemaExtensions covering unfiltered queries, single/multiple filters, fuzzy filtering, sorting (ascending/descending), pagination with skip/limit, and error cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring extra attention:
    • buildSQLFilter logic in helper.go: verify PostgreSQL AST construction, operator handling, null semantics, and OR/AND grouping correctness
    • buildSQLSort validation for sort direction and edge cases
    • CheckError behavior change: confirm nil return for non-NotFound errors doesn't break existing callers
    • Integration test coverage: ensure all filter combinations and edge cases are properly validated

Possibly related PRs

  • SpecterOps/BloodHound#2065: Adds create and get-by-id functionality for GraphSchemaExtension; this PR builds on that foundation with a listing/filtering method.
  • SpecterOps/BloodHound#2083: Modifies the OpenGraphSchema interface and related database/mocks layers; overlapping surface area for schema extension database operations.

Suggested reviewers

  • wes-mil
  • LawsonWillard
  • superlinkx

Poem

🐰 A query builder hops through fields, Filtering extensions the database yields, Sorting and paging with SQL delight, New methods blooming, shining bright! Tests guard the path, all corners checked— Graph schemas perfectly intersected! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding a database handler to list/query extension entries, with the ticket reference providing appropriate context.
Description check ✅ Passed The description covers all critical sections: it explains what was added, references the Jira ticket, describes testing approach, identifies the change type, and confirms checklist completion.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch BED-6778--OG-db-list-extensions
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca3f2c55d6d82d8ceb96385619fc195ce73c9cba and 88bd82b2f9885860e272bfd0ac64035bd279859e.

📒 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 (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/api/src/database/graphschema_integration_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 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-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.go
  • cmd/api/src/database/mocks/db.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 (3)
cmd/api/src/model/filter.go (4)
  • Filters (485-485)
  • Sort (507-507)
  • SortItem (502-505)
  • AscendingSortDirection (491-491)
cmd/api/src/model/graphschema.go (3)
  • GraphSchemaExtensions (19-19)
  • GraphSchemaExtension (21-28)
  • GraphSchemaExtension (30-32)
cmd/api/src/database/helper.go (1)
  • CheckError (39-45)
cmd/api/src/database/mocks/db.go (3)
cmd/api/src/auth/model.go (1)
  • Context (174-178)
cmd/api/src/model/filter.go (2)
  • Filters (485-485)
  • Sort (507-507)
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). (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 (3)
cmd/api/src/database/mocks/db.go (1)

1649-1663: LGTM! Generated mock matches the interface.

The generated mock method and recorder correctly match the GetGraphSchemaExtensions interface signature defined in cmd/api/src/database/graphschema.go:30.

cmd/api/src/database/graphschema.go (2)

30-30: LGTM! Interface method addition is well-defined.

The new GetGraphSchemaExtensions method signature appropriately uses model.Filters and model.Sort types for filtering and sorting, with pagination parameters skip and limit, and returns both the results and total count for proper pagination support.

101-163: LGTM! Implementation handles filtering, sorting, and pagination correctly.

The implementation properly:

  • Defaults to sorting by id when no sort is specified (lines 118-120), ensuring consistent pagination
  • Conditionally builds WHERE, ORDER BY, and LIMIT/OFFSET clauses
  • Executes a separate COUNT query only when pagination is requested (lines 149-156)
  • Uses parameterized queries for filter values via filter.params
  • Returns appropriate error types via CheckError

The COUNT query correctly uses only whereClauseString (line 151), avoiding the SQL duplication issue noted in past review comments.

Note: This implementation relies on buildSQLFilter and buildSQLSort helper functions (lines 112, 121) which are not included in this review. Based on past review comments, those helpers should validate column names and operators against an allowlist to prevent SQL injection. Assuming those validations are properly implemented in cmd/api/src/database/helper.go.

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 20 '25 17:11 coderabbitai[bot]

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. But yeah I agree it's hard to wrap your brain around these!

AD7ZJ avatar Dec 08 '25 22:12 AD7ZJ