feat(OpenGraph): Add OpenGraph nodes to shortest-path endpoint BED-6718 #2241
Conversation
sirisjo
added
enhancement
WalkthroughAdds an only_traversable query parameter and OpenGraph-aware pathfinding: new request-scoped context usage, traversable-kind filtering, OpenGraph node-fetch fallback, new graph query methods, fixtures/mocks, OpenAPI/schema updates, and expanded tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as GetShortestPath (API)
participant Flags as FlagsStore
participant Filter as FilterBuilder
participant GraphQ as GraphQuery
participant Analysis as Analysis (Node Fetch)
Client->>API: GET /api/v2/graphs/shortest-path?relationship_kinds=&only_traversable
API->>Flags: GetFlagByKey(FeatureOpenGraphSearch)
Flags-->>API: flag value
API->>Filter: createRelationshipKindFilterCriteria(relationship_kinds, only_traversable, validKinds)
alt OpenGraph enabled
API->>GraphQ: GetAllShortestPathsWithOpenGraph(start,end,filter)
GraphQ->>Analysis: FetchNodeByObjectIDIncludeOpenGraph(...)
Analysis-->>GraphQ: Node (may be OpenGraph kind)
GraphQ-->>API: PathSet
else OpenGraph disabled
API->>GraphQ: GetAllShortestPaths(start,end,filter)
GraphQ->>Analysis: FetchNodeByObjectID(...)
Analysis-->>GraphQ: Node
GraphQ-->>API: PathSet
end
API-->>Client: HTTP Response (PathSet or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
sirisjo
changed the title
/shortest-path endpoint BED-6718shortest-path endpoint BED-6718
sirisjo
changed the title
shortest-path endpoint BED-6718![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cmd/api/src/api/v2/pathfinding.go:
- Around line 146-148: Rename the local variable named `context` to `ctx` to
avoid shadowing the imported `context` package: change the declaration `context
= request.Context()` to `ctx := request.Context()` (or `var ctx context.Context
= request.Context()` if you prefer explicit typing) and update every use of
`context` in this function (the occurrences at the previously noted call sites)
to `ctx` (e.g., replace `context` with `ctx` in calls that pass the request
context into graph/path functions and error handling).
In @packages/go/openapi/doc/openapi.json:
- Around line 7246-7253: Update the "description" for the query parameter
"only_traversable" to explicitly state (1) when only_traversable=true and
relationship_kinds is omitted the API will restrict to all traversable
relationship kinds by default, (2) when only_traversable=false the API will
consider both traversable and non‑traversable kinds (i.e., no traversability
filtering) even if relationship_kinds is provided, and (3) when the parameter is
omitted document the default behavior (specify true or false as the default).
Keep the text concise and concrete so callers know exact behavior in the three
scenarios and include the default value.
🧹 Nitpick comments (6)
cmd/api/src/services/saml/mocks/saml.go (1)
1-1: Auto-generated file: consider regenerating instead of manual updates.This file is auto-generated by MockGen (as indicated by the comment on line 17). Rather than manually updating the copyright header, consider regenerating this file using the mockgen command shown in the comments (line 22) to ensure consistency with the generation tooling.
For bulk license/header year updates across generated files, adding the
copyright_fileparameter to the mockgen invocation (already present:-copyright_file=../../../../../LICENSE.header) ensures headers stay in sync automatically on future regenerations.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx (1)
176-176: Minor: copyright year inconsistency with other updated files.The file still shows copyright 2025 (line 1) while other generated files in the PR have been updated to 2026. For consistency with the broader year-bump pattern, consider updating the copyright header if this file is part of the 2026 update cycle. The trailing newline addition is fine.
cmd/api/src/queries/graph_integration_test.go (1)
507-776: Consider using subtests for better test organization.The test provides excellent coverage of OpenGraph pathfinding scenarios. However, it would benefit from wrapping each test case in
t.Run(testCase.name, func(t *testing.T) {...}). This would:
- Show which specific scenario failed in test output
- Allow running individual test cases with
-run- Follow Go testing best practices
♻️ Suggested refactor
defer teardownIntegrationTestSuite(t, &testSuite) for _, testCase := range testTable { - actual, err := graphQuery.GetAllShortestPathsWithOpenGraph(testSuite.Context, testCase.startNodeID, testCase.endNodeID, testCase.filter) - require.Nil(t, err) - require.Equal(t, testCase.expected, actual) + t.Run(testCase.name, func(t *testing.T) { + actual, err := graphQuery.GetAllShortestPathsWithOpenGraph(testSuite.Context, testCase.startNodeID, testCase.endNodeID, testCase.filter) + require.Nil(t, err) + require.Equal(t, testCase.expected, actual) + }) } }cmd/api/src/queries/graph.go (1)
284-315: Implementation is correct; consider reducing duplication.The method correctly implements OpenGraph-aware pathfinding by using
FetchNodeByObjectIDIncludeOpenGraph. The logic mirrorsGetAllShortestPathswith only the node fetching function differing. While functional, you could optionally refactor both methods to share the common path-fetching logic through an internal helper function that accepts a node-fetching strategy.♻️ Optional refactor to reduce duplication
Consider extracting shared logic into a helper:
func (s *GraphQuery) getAllShortestPathsInternal(ctx context.Context, startNodeID string, endNodeID string, filter graph.Criteria, nodeFetcher func(tx graph.Transaction, objectID string) (*graph.Node, error)) (graph.PathSet, error) { var paths graph.PathSet return paths, s.Graph.ReadTransaction(ctx, func(tx graph.Transaction) error { if startNode, err := nodeFetcher(tx, startNodeID); err != nil { return err } else if endNode, err := nodeFetcher(tx, endNodeID); err != nil { return err } else { criteria := []graph.Criteria{ query.Equals(query.StartID(), startNode.ID), query.Equals(query.EndID(), endNode.ID), } if filter != nil { criteria = append(criteria, filter) } return tx.Relationships().Filter(query.And(criteria...)).FetchAllShortestPaths(func(cursor graph.Cursor[graph.Path]) error { for path := range cursor.Chan() { if len(path.Edges) > 0 { paths.AddPath(path) } } return cursor.Error() }) } }) } func (s *GraphQuery) GetAllShortestPaths(ctx context.Context, startNodeID string, endNodeID string, filter graph.Criteria) (graph.PathSet, error) { defer measure.ContextMeasure(ctx, slog.LevelInfo, "GetAllShortestPaths")() return s.getAllShortestPathsInternal(ctx, startNodeID, endNodeID, filter, analysis.FetchNodeByObjectID) } func (s *GraphQuery) GetAllShortestPathsWithOpenGraph(ctx context.Context, startNodeID string, endNodeID string, filter graph.Criteria) (graph.PathSet, error) { defer measure.ContextMeasure(ctx, slog.LevelInfo, "GetAllShortestPathsWithOpenGraph")() return s.getAllShortestPathsInternal(ctx, startNodeID, endNodeID, filter, analysis.FetchNodeByObjectIDIncludeOpenGraph) }cmd/api/src/api/v2/pathfinding_test.go (1)
821-823: Minor: Redundant parentheses aroundad.Contains.The extra parentheses don't affect functionality but are unnecessary.
Suggested fix
- GetAllShortestPathsWithOpenGraph(gomock.Any(), "someID", "someOtherID", query.KindIn(query.Relationship(), traversableKindsWithOpenGraph.Exclude(graph.Kinds{}.Add((ad.Contains)))...)). + GetAllShortestPathsWithOpenGraph(gomock.Any(), "someID", "someOtherID", query.KindIn(query.Relationship(), traversableKindsWithOpenGraph.Exclude(graph.Kinds{}.Add(ad.Contains))...)).cmd/api/src/api/v2/pathfinding.go (1)
114-124: Confusing logic: consider adding a clarifying comment or refactoring.The function name
relationshipKindExistsInGraphsuggests it checks whether a kind exists, but it returnsfalseunconditionally whenonlyIncludeTraversableKindsisfalse. The actual purpose appears to be: "when requesting only traversable kinds, check if a non-traversable kind is still valid (exists in the full relationship set) so it can be silently filtered out instead of erroring."Also note this function only checks built-in AD/Azure relationships and won't recognize OpenGraph edge kinds—this appears intentional since OpenGraph kinds would already be in
acceptableKinds, but a comment would clarify the design.Suggested documentation
-// Determines if the relationship kind is technically a valid built-in kind that exists in the graph +// relationshipKindExistsInGraph checks if a kind is a valid built-in relationship kind. +// This is used when onlyIncludeTraversableKinds=true to allow users to request +// non-traversable kinds without error—those kinds are silently excluded from the filter +// rather than returning a validation error. When onlyIncludeTraversableKinds=false, +// all valid kinds should already be in acceptableKinds, so this fallback isn't needed. +// Note: This only checks AD/Azure built-in kinds; OpenGraph kinds should already be +// included in acceptableKinds when OpenGraph is enabled. func relationshipKindExistsInGraph(kind graph.Kind, onlyIncludeTraversableKinds bool) bool {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
LICENSE.headercmd/api/src/api/constant.gocmd/api/src/api/mocks/authenticator.gocmd/api/src/api/v2/pathfinding.gocmd/api/src/api/v2/pathfinding_internal_test.gocmd/api/src/api/v2/pathfinding_test.gocmd/api/src/daemons/datapipe/mocks/cleanup.gocmd/api/src/database/mocks/auth.gocmd/api/src/database/mocks/db.gocmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.jsoncmd/api/src/queries/graph.gocmd/api/src/queries/graph_integration_test.gocmd/api/src/queries/mocks/graph.gocmd/api/src/services/agi/mocks/mock.gocmd/api/src/services/dataquality/mocks/mock.gocmd/api/src/services/fs/mocks/fs.gocmd/api/src/services/graphify/mocks/ingest.gocmd/api/src/services/oidc/mocks/oidc.gocmd/api/src/services/saml/mocks/saml.gocmd/api/src/services/upload/mocks/mock.gocmd/api/src/utils/validation/mocks/validator.gocmd/api/src/vendormocks/dawgs/graph/mock.gocmd/api/src/vendormocks/io/fs/mock.gocmd/api/src/vendormocks/neo4j/neo4j-go-driver/v5/neo4j/mock.gopackages/csharp/graphschema/PropertyNames.cspackages/go/analysis/analysis.gopackages/go/crypto/mocks/digest.gopackages/go/graphschema/ad/ad.gopackages/go/graphschema/azure/azure.gopackages/go/graphschema/common/common.gopackages/go/graphschema/graph.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/paths/graph.graphs.shortest-path.yamlpackages/javascript/bh-shared-ui/src/graphSchema.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
💤 Files with no reviewable changes (1)
- packages/go/openapi/src/paths/graph-schema.edge-kinds.yaml
🧰 Additional context used
🧠 Learnings (7)
📚 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:
packages/go/graphschema/ad/ad.gopackages/go/graphschema/azure/azure.gopackages/go/graphschema/graph.gopackages/go/graphschema/common/common.gocmd/api/src/api/v2/pathfinding_test.go
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsxpackages/javascript/bh-shared-ui/src/graphSchema.ts
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/queries/graph.go
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound 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.
Applied to files:
cmd/api/src/queries/graph.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/queries/graph_integration_test.gocmd/api/src/api/v2/pathfinding_test.go
📚 Learning: 2025-05-29T18:42:23.137Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:77-86
Timestamp: 2025-05-29T18:42:23.137Z
Learning: Copyright years in license headers should typically preserve the original year when the work was created, not be updated to the current year when the license is added later. The engineering team (#bhe-engineering, bhe-ec) should provide guidance on the copyright year policy for the BloodHound project.
Applied to files:
LICENSE.header
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
packages/go/openapi/doc/openapi.json
🧬 Code graph analysis (4)
packages/go/analysis/analysis.go (3)
packages/go/graphschema/ad/ad.go (2)
Entity(28-28)Nodes(1146-1148)packages/go/graphschema/azure/azure.go (1)
Entity(28-28)packages/go/graphschema/common/common.go (2)
Nodes(33-35)ObjectID(52-52)
cmd/api/src/queries/graph.go (2)
packages/go/analysis/analysis.go (1)
FetchNodeByObjectIDIncludeOpenGraph(110-119)packages/go/graphschema/graph.go (1)
Path(35-39)
cmd/api/src/queries/graph_integration_test.go (2)
packages/go/graphschema/graph.go (1)
Path(35-39)packages/go/lab/generic/graph.go (1)
Node(36-40)
cmd/api/src/api/v2/pathfinding_test.go (6)
cmd/api/src/database/mocks/db.go (1)
NewMockDatabase(55-59)cmd/api/src/queries/graph.go (1)
GraphQuery(161-169)cmd/api/src/model/graphschema.go (1)
GraphSchemaEdgeKindsWithNamedSchema(146-146)cmd/api/src/model/appcfg/flag.go (2)
FeatureOpenGraphSearch(44-44)FeatureFlag(50-70)packages/go/graphschema/ad/ad.go (2)
GenericAll(45-45)GenericWrite(46-46)packages/go/graphschema/azure/azure.go (1)
ServicePrincipalEndpointReadWriteAll(84-84)
⏰ 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: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (36)
packages/csharp/graphschema/PropertyNames.cs (1)
2-2: Copyright year update looks good.The copyright year has been correctly updated from 2025 to 2026. No functional changes.
packages/javascript/bh-shared-ui/src/graphSchema.ts (1)
1-1: LGTM!Copyright year update only. The file correctly exports all public schema enums, display functions, and pathfinding helpers for downstream consumers.
cmd/api/src/services/dataquality/mocks/mock.go (1)
1-1: LGTM!Auto-generated mock file with copyright year update. No functional changes.
cmd/api/src/services/upload/mocks/mock.go (1)
1-1: LGTM!Auto-generated mock file with copyright year update. No functional changes.
packages/go/graphschema/graph.go (1)
1-1: LGTM!Auto-generated file from CUE schemas with copyright year update. No functional changes. Based on learnings, these changes are legitimate outputs of the generation process.
cmd/api/src/api/mocks/authenticator.go (1)
1-1: LGTM!Auto-generated mock file with copyright year update. No functional changes.
cmd/api/src/vendormocks/neo4j/neo4j-go-driver/v5/neo4j/mock.go (1)
1-1: LGTM!Auto-generated mock file with copyright year update. No functional changes.
LICENSE.header (1)
1-1: Copyright year update noted.The copyright year has been updated to 2026. Based on learnings, there may be a project-level policy consideration regarding whether copyright years should preserve the original year or be updated annually. If this hasn't been formalized, the engineering team may want to document the preferred approach.
cmd/api/src/services/graphify/mocks/ingest.go (1)
1-1: LGTM!Auto-generated mock file with copyright year update. No functional changes.
packages/go/graphschema/azure/azure.go (1)
1-1: Routine copyright update in generated file.cmd/api/src/vendormocks/io/fs/mock.go (1)
1-1: Routine copyright update in generated mock file.packages/go/crypto/mocks/digest.go (1)
1-1: Routine copyright update in generated mock file.cmd/api/src/database/mocks/auth.go (1)
1-1: Copyright year updated as part of repository maintenance.This auto-generated mock file has been updated with the 2026 copyright year. No functional changes.
cmd/api/src/services/oidc/mocks/oidc.go (1)
1-1: Copyright year updated as part of repository maintenance.This auto-generated mock file has been updated with the 2026 copyright year. No functional changes.
cmd/api/src/database/mocks/db.go (1)
1-1: Copyright year updated as part of repository maintenance.This auto-generated mock file has been updated with the 2026 copyright year. No functional changes to the mock interface or methods.
cmd/api/src/daemons/datapipe/mocks/cleanup.go (1)
1-1: Skip: auto-generated mock file.This is machine-generated code marked "DO NOT EDIT" (line 17). The copyright year bump is routine and expected for auto-generated mocks.
cmd/api/src/utils/validation/mocks/validator.go (1)
1-1: Skip: auto-generated mock file.This is machine-generated code marked "DO NOT EDIT" (line 17). The copyright year bump is routine and expected for auto-generated mocks.
cmd/api/src/vendormocks/dawgs/graph/mock.go (1)
1-1: LGTM: Copyright year updated.The copyright year has been appropriately updated to 2026 for this generated mock file.
packages/go/graphschema/ad/ad.go (1)
1-1: LGTM: Copyright year updated in generated file.The copyright year has been appropriately updated to 2026. This file is generated from CUE schemas, and the change is part of the standard code generation process.
Based on learnings, files in
packages/go/graphschema/*/are automatically generated and such updates are expected.cmd/api/src/api/constant.go (1)
42-51: LGTM: New query parameter constant added.The new
QueryParameterIncludeOnlyTraversableKindsconstant follows the established naming convention and aligns with the OpenGraph pathfinding feature introduced in this PR. The alignment improvements to existing constants also enhance readability.packages/go/analysis/analysis.go (2)
88-93: LGTM: OpenGraph node filtering criteria added.The
openGraphNodeByIndexedKindPropertyfunction correctly creates a query criteria that matches nodes by property while excluding AD and Azure base entities (ad.Entityandazure.Entity), enabling OpenGraph-specific node lookups.
110-119: LGTM: OpenGraph-inclusive node lookup implemented correctly.The
FetchNodeByObjectIDIncludeOpenGraphfunction properly implements a fallback pattern:
- First attempts standard AD/Azure entity lookup via
FetchNodeByObjectID- Falls back to OpenGraph-aware query only when the node is not found
- Correctly propagates non-NotFound errors immediately
This design enables OpenGraph node retrieval while maintaining backward compatibility with existing node lookup logic.
packages/go/openapi/src/paths/graph.graphs.shortest-path.yaml (2)
41-41: LGTM: Clear error behavior documented.The updated description for
relationship_kindsappropriately documents that invalid kinds will result in an error, setting clear expectations for API consumers.
45-50: LGTM: New parameter documented with clear interaction behavior.The
only_traversableparameter is well-documented with:
- Clear description of its purpose
- Explanation of interaction with
relationship_kindsparameter- Appropriate schema definition (boolean, optional)
The documentation makes it clear that when enabled, non-traversable kinds specified in
relationship_kindswill be filtered out, which is the expected behavior.cmd/api/src/api/v2/pathfinding_internal_test.go (1)
30-69: LGTM! Comprehensive test coverage for the new parameter.The test additions properly validate the
onlyTraversableparameter behavior, including error cases and filtering of non-traversable relationship kinds.cmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.json (1)
1-168: LGTM! Clear fixture structure for OpenGraph testing.The restructuring with explicit edge definitions and numeric node IDs provides a clean foundation for the OpenGraph pathfinding tests.
cmd/api/src/queries/graph_integration_test.go (1)
139-139: LGTM! ObjectID expectations updated to match fixture.The test expectations correctly reflect the fixture changes from "TEST-1" to numeric string IDs.
Also applies to: 226-226
cmd/api/src/queries/graph.go (1)
139-139: LGTM! Clean interface extension.The new method signature appropriately extends the Graph interface to support OpenGraph pathfinding.
cmd/api/src/queries/mocks/graph.go (1)
1-1: LGTM! Mock properly implements the new interface method.The auto-generated mock correctly supports the new
GetAllShortestPathsWithOpenGraphmethod and follows the established pattern. Copyright year appropriately updated to 2026.Also applies to: 199-212
cmd/api/src/api/v2/pathfinding_test.go (2)
100-118: LGTM! Comprehensive test setup for OpenGraph pathfinding scenarios.The test variables are well-organized, covering both standard and OpenGraph-enabled pathfinding with proper kind filters. The setup correctly initializes built-in kinds concatenated with OpenGraph kinds for testing various filter scenarios.
415-478: LGTM! Good coverage for theonly_traversableparameter behavior.These test cases properly verify that:
- Invalid boolean values default to
falsewithout erroring- The traversable filter restricts to pathfinding relationships
- Combining
only_traversablewithrelationship_kindscorrectly intersects the filtersThe test at line 470 correctly expects only
ad.ContainssinceAZScopedTois not a traversable relationship.cmd/api/src/api/v2/pathfinding.go (4)
80-112: LGTM! Parameter parsing logic is well-structured.The function correctly:
- Validates format with regex
- Parses the operator and kinds
- Validates each kind against acceptable kinds or checks graph existence
- Returns appropriate defaults when no parameter is provided
126-138: LGTM! Clean filter criteria construction.The function properly handles both inclusion (
in) and exclusion (nin) operators, and correctly defaults to including all valid kinds whenonlyIncludeTraversableKindsis true with no explicit parameter.
152-155: Intentional: invalidonly_traversabledefaults tofalse.The error is logged but not returned, allowing requests with invalid boolean values to proceed with the default. This is verified by the test case "InvalidOnlyTraversableParamDefaultsToFalse".
Consider whether a 400 Bad Request would be more appropriate for invalid parameter values, or if the silent fallback is the desired UX.
194-217: LGTM! OpenGraph path retrieval logic is sound.The function correctly:
- Builds filters for traversability when requested
- Fetches OpenGraph edge kinds from the database
- Merges OpenGraph kinds with built-in kinds
- Creates appropriate filter criteria and queries the graph
Minor nit: inconsistent blank line at line 211 before
} else {.packages/go/openapi/doc/openapi.json (1)
7237-7254: Reminder: This file is generated from YAML sources.Based on learnings,
packages/go/openapi/doc/openapi.jsonis generated from YAML underpackages/go/openapi/src/schemas. Ensure that the source YAML files (graph.graphs.shortest-path.yaml) have been updated and the spec regenerated.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cmd/api/src/api/v2/pathfinding.go:
- Around line 184-192: In getAllShortestPaths change the parameter named context
to ctx to avoid shadowing the imported context package: update the function
signature in Resources.getAllShortestPaths to use ctx, and replace all uses of
the parameter (e.g., in the call to s.GraphQuery.GetAllShortestPaths) to pass
ctx instead of context; ensure any callers or references to this function
(within the same diff) are adjusted if they use the old name inside this
function body, keeping the rest of logic unchanged.
- Around line 194-217: The function getAllShortestPathsWithOpenGraph uses a
parameter named context which shadows the imported context package; rename the
parameter to ctx in the function signature and update all uses inside the
function (e.g., calls to s.DB.GetGraphSchemaEdgeKindsWithSchemaName(ctx, ...),
s.GraphQuery.GetAllShortestPathsWithOpenGraph(ctx, ...)) so the package import
remains accessible and naming matches other functions like getAllShortestPaths;
no other logic changes needed.
🧹 Nitpick comments (2)
cmd/api/src/api/v2/pathfinding.go (2)
114-124: Confusing function semantics and logic.The function name
relationshipKindExistsInGraphsuggests it checks whether a kind exists in the graph, but the actual logic returnsfalsewhenonlyIncludeTraversableKindsisfalse(line 117-118). This is counterintuitive—when not filtering to traversable-only, a valid built-in kind should still "exist."The function effectively answers: "Is this a valid built-in kind AND are we in traversable-only mode?" Consider renaming for clarity, e.g.,
isValidBuiltInKindForTraversableMode.Additionally, the nested if-else can be simplified:
♻️ Suggested simplification
func relationshipKindExistsInGraph(kind graph.Kind, onlyIncludeTraversableKinds bool) bool { validBuiltInKinds := graph.Kinds(ad.Relationships()).Concatenate(azure.Relationships()) - if !onlyIncludeTraversableKinds { - return false - } else if !validBuiltInKinds.ContainsOneOf(kind) { - return false - } else { - return true - } + return onlyIncludeTraversableKinds && validBuiltInKinds.ContainsOneOf(kind) }
152-155: Consider improving error handling for malformed boolean parameter.When
ParseOptionalBoolfails, the error is logged but the request continues with the default value (false). While this is reasonable for optional parameters, returning a400 Bad Requestfor a malformed boolean value would be more explicit and help API consumers identify their mistake.♻️ Optional: Return 400 for malformed boolean
onlyIncludeTraversableKinds, err := api.ParseOptionalBool(request.URL.Query().Get(api.QueryParameterIncludeOnlyTraversableKinds), false) if err != nil { - slog.ErrorContext(ctx, err.Error()) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf("Invalid value for '%s': expected boolean", api.QueryParameterIncludeOnlyTraversableKinds), request), response) + return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/api/v2/pathfinding.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/graph.graphs.shortest-path.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/openapi/doc/openapi.json
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/api/src/api/v2/pathfinding.go (7)
cmd/api/src/api/error.go (4)
ErrorWrapper(111-116)BuildErrorResponse(134-145)HandleDatabaseError(149-158)FormatDatabaseError(162-169)cmd/api/src/ctx/ctx.go (1)
Get(75-85)cmd/api/src/api/constant.go (1)
QueryParameterIncludeOnlyTraversableKinds(51-51)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/model/appcfg/flag.go (1)
FeatureOpenGraphSearch(44-44)cmd/api/src/api/v2/model.go (1)
Resources(105-118)cmd/api/src/model/filter.go (3)
Filters(485-485)Filter(479-483)Sort(507-507)
⏰ 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: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (4)
packages/go/openapi/src/paths/graph.graphs.shortest-path.yaml (1)
40-50: LGTM - Well-documented new parameter.The
only_traversableparameter is well-documented with clear explanations of its behavior in different scenarios (true with/without relationship_kinds, false/omitted). The updatedrelationship_kindsdescription appropriately clarifies the error behavior for invalid kinds.cmd/api/src/api/v2/pathfinding.go (3)
98-105: Potential silent filtering of valid relationship kinds.When a kind is not in
acceptableKindsbut passesrelationshipKindExistsInGraph(returns true whenonlyTraversableis true and kind is a valid built-in), it's silently dropped fromparameterKindswithout being added or causing an error. This means user-specified built-in non-traversable kinds would be silently ignored whenonlyTraversable=true.Is this the intended behavior? If so, consider documenting this in the OpenAPI description. If not, the kind should likely be added to
parameterKindswhen it exists in the graph.
126-138: LGTM - Filter criteria logic is correct.The function properly handles both
inandninoperations, with thenincase correctly excluding specified kinds from the valid set. The early return for traversable-only mode without specified kinds is a clean optimization.
162-181: LGTM - Feature flag integration and routing logic.The feature flag retrieval with proper database error handling, the conditional assignment of
validBuiltInKindsbased ononlyIncludeTraversableKinds, and the clean branching between OpenGraph and standard pathfinding are well-structured.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/api/src/queries/graph_integration_test.go (1)
507-518: Minor: Test function name uses singular "Path" vs method name uses "Paths".The test function is named
TestGetAllShortestPathWithOpenGraphbut callsGetAllShortestPathsWithOpenGraph(plural). Consider renaming for consistency:-func TestGetAllShortestPathWithOpenGraph(t *testing.T) { +func TestGetAllShortestPathsWithOpenGraph(t *testing.T) {cmd/api/src/api/v2/pathfinding.go (1)
146-149: Consider consistent error handling for malformed boolean parameters.The
ParseOptionalBoolerror is currently logged but execution continues with the default value offalse. While the test explicitly validates this behavior (InvalidOnlyTraversableParamDefaultsToFalsein pathfinding_test.go confirms malformed input likeonly_traversable=notABoolsilently defaults tofalse), this approach differs from other endpoints. For example,ad_entity.goreturns HTTP 400 when the same function encounters an error (lines 67-68). Consider standardizing this behavior across the API—either validate and reject malformed booleans consistently, or document the intentional lenient defaulting pattern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/pathfinding.gocmd/api/src/api/v2/pathfinding_test.gocmd/api/src/queries/graph.gocmd/api/src/queries/graph_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/queries/graph.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/api/v2/pathfinding_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/api/v2/pathfinding_test.gocmd/api/src/queries/graph_integration_test.go
🧬 Code graph analysis (2)
cmd/api/src/queries/graph_integration_test.go (3)
cmd/api/src/queries/graph.go (1)
NewGraphQuery(171-181)packages/go/graphschema/graph.go (1)
Path(35-39)packages/go/lab/generic/graph.go (1)
Node(36-40)
cmd/api/src/api/v2/pathfinding.go (2)
cmd/api/src/api/constant.go (1)
QueryParameterIncludeOnlyTraversableKinds(51-51)cmd/api/src/model/appcfg/flag.go (1)
FeatureOpenGraphSearch(44-44)
⏰ 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 (12)
cmd/api/src/queries/graph_integration_test.go (3)
139-146: LGTM: Test value updated to match fixture data.The change from
"TEST-1"to"2"aligns with the updated fixture data in the OpenGraph JSON files where object IDs are now numeric strings.
224-232: LGTM: Consistent with fixture updates.
771-777: LGTM: Comprehensive integration tests for OpenGraph pathfinding.The test cases provide good coverage of:
- OpenGraph-to-OpenGraph paths
- AD-to-AD paths
- Cross-graph (OpenGraph-to-AD) paths
- Kind filtering with exclusion and inclusion
The table-driven approach with explicit expected results ensures precise validation of the pathfinding behavior.
cmd/api/src/api/v2/pathfinding_test.go (4)
100-118: LGTM: Well-structured test setup with comprehensive filter definitions.The test setup properly defines:
- Mock database for feature flag retrieval
- OpenGraph edge kinds for testing
- Multiple filter combinations (all kinds, traversable kinds, with/without OpenGraph)
This allows thorough testing of the various pathfinding scenarios.
144-161: LGTM: Proper error handling test for feature flag retrieval failure.The test correctly verifies that a database error during feature flag lookup results in a 500 status code with a generic error message, avoiding information leakage.
415-478: LGTM: Thorough testing ofonly_traversableparameter behavior.The tests properly verify:
- Invalid boolean values default to
false(line 416-435)only_traversable=truerestricts to traversable relationships only (line 437-456)- Combining
only_traversable=truewithrelationship_kindsfilters correctly intersects the constraints (line 458-478)
762-830: LGTM: Comprehensive OpenGraph traversable relationship tests.The tests properly verify:
only_traversable=truepasses the correct filter toGetGraphSchemaEdgeKindsWithSchemaName(line 773, 796, 820)- Both IN and NIN operators work correctly with OpenGraph and traversable filtering
- The filter
{"is_traversable": [{Operator: model.Equals, Value: "true"}]}is correctly appliedcmd/api/src/api/v2/pathfinding.go (5)
80-112: LGTM: Enhanced relationship kinds parsing with traversable filtering.The updated
parseRelationshipKindsParamfunction properly:
- Validates the format with regex
- Filters kinds based on
acceptableKinds- Silently ignores non-traversable but valid kinds when
onlyTraversable=true(line 102-104)- Returns an error for completely invalid kinds
The silent filtering behavior for non-traversable kinds is reasonable since it allows users to specify kinds without needing to know which are traversable.
114-118: LGTM: Helper correctly identifies non-traversable valid kinds.The logic correctly determines if a kind exists in the built-in graph but should be silently ignored because it's not traversable. This enables the silent filtering behavior documented in the PR.
120-132: LGTM: Clean filter criteria creation logic.The function properly handles:
onlyTraversable=truewithoutrelationship_kinds→ returns all traversable kindsin:Kind1,Kind2→ includes only specified kindsnin:Kind1,Kind2→ excludes specified kinds from valid set
178-186: LGTM: Clean implementation of standard shortest paths retrieval.Proper error handling with distinct status codes:
- 400 for invalid filter criteria
- 500 for graph query errors
188-211: LGTM: OpenGraph-aware pathfinding implementation.The method correctly:
- Applies
is_traversablefilter when retrieving OpenGraph edge kinds (line 190-192)- Extends
validKindswith OpenGraph edge kinds (line 200)- Uses the combined kinds for filter creation and path retrieval
Minor formatting: there are extra blank lines at 205 and 210-211, but this doesn't affect functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/api/src/queries/graph_integration_test.go (1)
507-778: Consider making edge ID assertions more resilient.The test cases hardcode expected edge IDs (e.g.,
ID: 1,ID: 4,ID: 7) which are typically auto-assigned by the database. If insertion order changes or the database implementation varies, these tests could become flaky.Consider one of these approaches:
- Assert on path structure (node sequence, edge kinds, start/end IDs) without checking the edge
IDfield.- Use a custom comparison helper that ignores auto-generated IDs.
- Zero out the edge IDs in both expected and actual results before comparison.
Example helper approach:
func assertPathSetEquivalent(t *testing.T, expected, actual graph.PathSet) { require.Equal(t, len(expected), len(actual)) for i := range expected { require.Equal(t, len(expected[i].Nodes), len(actual[i].Nodes)) require.Equal(t, len(expected[i].Edges), len(actual[i].Edges)) for j, edge := range actual[i].Edges { require.Equal(t, expected[i].Edges[j].StartID, edge.StartID) require.Equal(t, expected[i].Edges[j].EndID, edge.EndID) require.Equal(t, expected[i].Edges[j].Kind, edge.Kind) } // Similar for nodes... } }If edge IDs are guaranteed stable due to controlled fixture loading, please add a comment explaining why the hardcoded IDs are reliable.
packages/go/openapi/doc/openapi.json (1)
7252-7260: Consider clarifying the edge case behavior.The description is comprehensive. However, it might be worth clarifying what happens when
only_traversable=trueand all specifiedrelationship_kindsare non-traversable. Would this result in an empty result set, an error, or default to searching all traversable kinds?If this edge case is already handled gracefully (e.g., returns empty or falls back to all traversable), explicitly documenting this would help API consumers avoid unexpected behavior.
cmd/api/src/api/v2/pathfinding.go (2)
146-149: Silent default tofalseon parse error is intentional but consider explicit behavior.The error is logged but execution continues with
onlyIncludeTraversableKinds = false. While this is validated by the test case "InvalidOnlyTraversableParamDefaultsToFalse", consider whether silently defaulting is the desired UX. An alternative would be to return a 400 error for malformed boolean values.Alternative: Return 400 for invalid boolean parameter
onlyIncludeTraversableKinds, err := api.ParseOptionalBool(request.URL.Query().Get(api.QueryParameterIncludeOnlyTraversableKinds), false) if err != nil { - slog.ErrorContext(ctx, err.Error()) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf("Invalid value for %s: must be 'true' or 'false'", api.QueryParameterIncludeOnlyTraversableKinds), request), response) + return }
188-211: OpenGraph path retrieval correctly extends valid kinds.The implementation:
- Builds appropriate filters for
is_traversablewhen needed- Fetches OpenGraph edge kinds from database
- Concatenates with built-in kinds before filter creation
- Uses the OpenGraph-aware graph query method
Minor formatting: there are extra blank lines (205-206, 210-211) that could be cleaned up, but this doesn't affect functionality.
Optional: Clean up extra blank lines
} else if paths, err := s.GraphQuery.GetAllShortestPathsWithOpenGraph(ctx, startNode, endNode, kindFilter); err != nil { return nil, api.BuildErrorResponse(http.StatusInternalServerError, err.Error(), request) - } else { return paths, nil } - } -
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cmd/api/src/api/constant.gocmd/api/src/api/v2/pathfinding.gocmd/api/src/api/v2/pathfinding_internal_test.gocmd/api/src/api/v2/pathfinding_test.gocmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.jsoncmd/api/src/queries/graph.gocmd/api/src/queries/graph_integration_test.gocmd/api/src/queries/mocks/graph.gocmd/ui/public/mockServiceWorker.jspackages/go/analysis/analysis.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/paths/graph.graphs.shortest-path.yaml
💤 Files with no reviewable changes (1)
- packages/go/openapi/src/paths/graph-schema.edge-kinds.yaml
✅ Files skipped from review due to trivial changes (1)
- cmd/ui/public/mockServiceWorker.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/go/openapi/src/paths/graph.graphs.shortest-path.yaml
- cmd/api/src/queries/mocks/graph.go
- packages/go/analysis/analysis.go
- cmd/api/src/api/v2/pathfinding_internal_test.go
🧰 Additional context used
🧠 Learnings (4)
📚 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/queries/graph_integration_test.gocmd/api/src/api/v2/pathfinding_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/api/v2/pathfinding_test.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/queries/graph.go
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound 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.
Applied to files:
cmd/api/src/queries/graph.go
🧬 Code graph analysis (3)
cmd/api/src/queries/graph_integration_test.go (3)
cmd/api/src/queries/graph.go (1)
NewGraphQuery(171-181)packages/go/graphschema/graph.go (1)
Path(35-39)packages/go/lab/generic/graph.go (1)
Node(36-40)
cmd/api/src/api/v2/pathfinding_test.go (4)
cmd/api/src/database/mocks/db.go (1)
NewMockDatabase(55-59)cmd/api/src/model/graphschema.go (1)
GraphSchemaEdgeKindsWithNamedSchema(158-158)cmd/api/src/model/appcfg/flag.go (2)
FeatureOpenGraphSearch(44-44)FeatureFlag(50-70)packages/go/graphschema/azure/azure.go (1)
ServicePrincipalEndpointReadWriteAll(84-84)
cmd/api/src/queries/graph.go (1)
packages/go/analysis/analysis.go (2)
FetchNodeByObjectID(98-108)FetchNodeByObjectIDIncludeOpenGraph(110-119)
⏰ 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: build-ui
- GitHub Check: run-tests
- GitHub Check: run-go-unit-tests
🔇 Additional comments (13)
cmd/api/src/api/constant.go (1)
42-51: LGTM!The new
QueryParameterIncludeOnlyTraversableKindsconstant follows existing naming conventions and the value aligns with the API documentation. The alignment padding improves readability.cmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.json (1)
1-167: LGTM!The fixture provides well-structured test data for OpenGraph pathfinding scenarios. The graph topology with
Knows,IsParent, andContainsedges enables testing shortest paths between OpenGraph nodes, AD nodes, and cross-graph paths—aligning with the integration test cases.cmd/api/src/queries/graph_integration_test.go (2)
139-139: LGTM!The change from the previous ObjectID to
"2"aligns with the updated fixture where node IDs are now numeric strings.
226-226: LGTM!Consistent with the fixture update.
packages/go/openapi/doc/openapi.json (1)
7245-7250: LGTM!The updated description clarifies error behavior when invalid relationship kinds are provided, which improves API documentation quality.
cmd/api/src/api/v2/pathfinding_test.go (4)
100-118: LGTM! Well-structured test setup with comprehensive test data.The test data setup cleanly defines all the necessary kinds and filters for testing both standard and OpenGraph pathfinding scenarios. The separation of
validBuiltInKindsvsvalidBuiltInTraversableKindsand their OpenGraph counterparts enables precise mock assertions.
415-456: Good coverage ofonly_traversableparameter behavior.These tests verify:
- Invalid boolean values default to
false(line 416-435)- When
true, only traversable relationships are used in the filter (line 437-456)- The mock expectations correctly assert the filter contains only traversable kinds
457-478: Correctly tests filtering of non-traversable kinds from explicitinlist.This test verifies that when
only_traversable=trueand the user requestsin:Contains,AZScopedTo, onlyad.Containsis included (sinceAZScopedTois valid but not traversable). This aligns with the implementation's silent-ignore behavior for valid-but-non-traversable kinds.
762-830: Comprehensive OpenGraph + only_traversable tests.These tests properly verify:
- The
is_traversablefilter is passed toGetGraphSchemaEdgeKindsWithSchemaName(line 773, 796, 820)- Both
inandninoperations work correctly with OpenGraph and traversable filtering- OpenGraph edge kinds are included in the filter criteria
cmd/api/src/queries/graph.go (2)
139-139: Good interface extension for OpenGraph support.Adding
GetAllShortestPathsWithOpenGraphto the interface maintains clean separation while enabling OpenGraph-aware pathfinding. The method signature mirrorsGetAllShortestPathsfor consistency.
251-289: Clean refactoring using strategy pattern for node fetching.The introduction of
getAllShortestPathsInternalwith anodeFetcherfunction parameter is a solid approach:
- Eliminates code duplication between standard and OpenGraph path retrieval
- The
nodeFetchersignaturefunc(tx graph.Transaction, objectID string) (*graph.Node, error)matches bothanalysis.FetchNodeByObjectIDandanalysis.FetchNodeByObjectIDIncludeOpenGraph- Error handling properly propagates node fetch failures
- Logging via
measure.ContextMeasureis appropriately placed in the public methodscmd/api/src/api/v2/pathfinding.go (2)
80-112: Well-designed parameterized kind validation.The refactored
parseRelationshipKindsParamcleanly separates validation logic:
- Validates format via regex
- Filters kinds against
acceptableKinds(which varies based ononlyTraversableand OpenGraph state)- Silently ignores valid-but-non-traversable kinds when appropriate (line 102-104)
The logic at lines 100-104 is correct:
- If kind is in acceptableKinds → include it
- Else if kind is valid but non-traversable AND onlyTraversable=true → silently skip
- Else → return error (truly invalid kind)
114-118: Correct helper for traversability check.This helper correctly determines if a kind exists in the built-in graph but is not in the traversable subset. The short-circuit with
onlyIncludeTraversableKindsensures the function only performs the check when relevant.
AD7ZJ
left a comment
AD7ZJ
left a comment
There was a problem hiding this comment.
Couple minor q's but overall looks good! This is a big ticket, nice work.
cmd/api/src/api/v2/pathfinding.go
Outdated
| if onlyIncludeTraversableKinds { | ||
| edgeKindFilters["is_traversable"] = append(edgeKindFilters["is_traversable"], model.Filter{Operator: model.Equals, Value: "true"}) | ||
| } | ||
| if openGraphEdges, _, err := s.DB.GetGraphSchemaEdgeKindsWithSchemaName(ctx, edgeKindFilters, model.Sort{}, 0, 0); err != nil { |
There was a problem hiding this comment.
If you don't need the schema name here, you could save a JOIN with GetGraphSchemaEdgeKinds()
cmd/api/src/api/v2/pathfinding.go
Outdated
| validKinds := graph.Kinds(ad.Relationships()).Concatenate(azure.Relationships()) | ||
| // kindIsValidButIsNotTraversable determines if a kind exists in the built-in graph, but is not traversable. | ||
| func kindIsValidButIsNotTraversable(kind graph.Kind, onlyIncludeTraversableKinds bool) bool { | ||
| validBuiltInKinds := graph.Kinds(ad.Relationships()).Concatenate(azure.Relationships()) |
There was a problem hiding this comment.
I'm confused by this part, is validBuiltInKinds a list of only traversable rels or all builtin rels? It looks like all builtin rels to me. A comment explaining would be good here.
There was a problem hiding this comment.
Thanks for working through this with me!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/api/v2/pathfinding.go (1)
104-136: UnusedonlyTraversableparameter.The
onlyTraversableparameter is passed toparseRelationshipKindsParambut never used within the function body. Either remove it or document its intended purpose.🔧 Suggested fix: remove unused parameter
-func parseRelationshipKindsParam(acceptableKinds graph.Kinds, relationshipKindsParam string, onlyTraversable bool) (graph.Kinds, string, error) { +func parseRelationshipKindsParam(acceptableKinds graph.Kinds, relationshipKindsParam string) (graph.Kinds, string, error) {Then update callers at lines 147 and elsewhere to not pass the third argument.
🧹 Nitpick comments (3)
packages/cue/bh/ad/ad.cue (1)
1915-1977: Add documentation explaining the intentional differences betweenPathfindingRelationshipsandPathfindingRelationshipsMatchFrontend.
PathfindingRelationshipsMatchFrontendintentionally differs fromPathfindingRelationshipsby:
- Excluding
ContainsIdentity,PropagatesACEsTo,GPOAppliesTo,CanApplyGPO(which are inSharedRelationshipKinds)- Including
ProtectAdminGroups(which is not inSharedRelationshipKinds)- Not including the additional edges from
PathfindingRelationships(Contains,DCFor,SameForestTrust,SpoofSIDHistory,AbuseTGTDelegation)Consider adding a comment explaining why these relationships are excluded from the frontend's traversable edges. Using an explicit list means future updates to
SharedRelationshipKindswon't automatically propagate here, so documenting the rationale will help maintainers understand whether new relationships should be added to this list.cmd/api/src/queries/graph_integration_test.go (1)
507-778: Comprehensive test coverage for OpenGraph pathfinding.The test covers key scenarios well:
- OpenGraph-to-OpenGraph paths
- AD-to-AD paths
- Cross-graph (OpenGraph-to-AD) paths
- Edge kind filtering (exclude/include)
Minor formatting nit: Lines 672 and 741 have unnecessary blank lines after the
Edges: []*graph.Relationship{opening brace.♻️ Optional: Remove extra blank lines
Edges: []*graph.Relationship{ - { ID: 1,cmd/api/src/api/v2/pathfinding.go (1)
138-141: Consider caching the built-in kinds slice.
kindIsValidBuiltIncreates a new concatenated slice on every call. While the impact is minimal for typical usage, consider caching this at package level for efficiency.♻️ Optional optimization
+var allBuiltInKinds = graph.Kinds(ad.Relationships()).Concatenate(azure.Relationships()) + // kindIsValidBuiltIn determines if a kind exists in the built-in graph func kindIsValidBuiltIn(kind graph.Kind) bool { - return graph.Kinds(ad.Relationships()).Concatenate(azure.Relationships()).ContainsOneOf(kind) + return allBuiltInKinds.ContainsOneOf(kind) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/api/src/api/constant.gocmd/api/src/api/v2/pathfinding.gocmd/api/src/api/v2/pathfinding_internal_test.gocmd/api/src/api/v2/pathfinding_test.gocmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.jsoncmd/api/src/queries/graph.gocmd/api/src/queries/graph_integration_test.gocmd/api/src/queries/mocks/graph.gopackages/cue/bh/ad/ad.cuepackages/cue/bh/bh.cuepackages/go/analysis/analysis.gopackages/go/graphschema/ad/ad.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/paths/graph.graphs.shortest-path.yamlpackages/go/schemagen/generator/golang.gopackages/go/schemagen/model/schema.go
💤 Files with no reviewable changes (1)
- packages/go/openapi/src/paths/graph-schema.edge-kinds.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/api/src/api/constant.go
- packages/go/openapi/src/paths/graph.graphs.shortest-path.yaml
- packages/cue/bh/bh.cue
- cmd/api/src/api/v2/pathfinding_internal_test.go
- packages/go/openapi/doc/openapi.json
- packages/go/schemagen/model/schema.go
- packages/go/schemagen/generator/golang.go
🧰 Additional context used
🧠 Learnings (6)
📚 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/queries/graph_integration_test.gocmd/api/src/api/v2/pathfinding_test.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/queries/graph.go
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound 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.
Applied to files:
cmd/api/src/queries/graph.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/api/v2/pathfinding.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/api/v2/pathfinding_test.gopackages/cue/bh/ad/ad.cue
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/api/v2/pathfinding_test.go
🧬 Code graph analysis (5)
packages/go/graphschema/ad/ad.go (2)
packages/go/graphschema/azure/azure.go (3)
Owns(72-72)MemberOf(55-55)Contains(49-49)packages/go/ein/incoming_models.go (1)
GPLink(249-252)
packages/go/analysis/analysis.go (3)
packages/go/graphschema/ad/ad.go (2)
Entity(28-28)Nodes(1146-1148)packages/go/graphschema/common/common.go (2)
Nodes(33-35)ObjectID(52-52)cmd/api/src/model/filter.go (1)
Filter(479-483)
cmd/api/src/queries/graph_integration_test.go (3)
cmd/api/src/queries/graph.go (1)
NewGraphQuery(171-181)packages/go/graphschema/graph.go (1)
Path(35-39)packages/go/lab/generic/graph.go (1)
Node(36-40)
cmd/api/src/queries/graph.go (2)
packages/go/lab/generic/graph.go (2)
Node(36-40)Graph(54-57)packages/go/analysis/analysis.go (2)
FetchNodeByObjectID(98-108)FetchNodeByObjectIDIncludeOpenGraph(110-119)
cmd/api/src/api/v2/pathfinding_test.go (3)
packages/go/graphschema/ad/ad.go (11)
Relationships(1149-1151)PathfindingRelationshipsMatchFrontend(1161-1163)PathfindingRelationships(1158-1160)Contains(54-54)Owns(44-44)GenericAll(45-45)GenericWrite(46-46)Nodes(1146-1148)Entity(28-28)Computer(30-30)User(29-29)cmd/api/src/model/graphschema.go (1)
GraphSchemaEdgeKinds(86-86)packages/go/graphschema/azure/azure.go (1)
ServicePrincipalEndpointReadWriteAll(84-84)
⏰ 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 (19)
packages/go/analysis/analysis.go (2)
88-93: LGTM - Well-designed predicate for OpenGraph node filtering.The predicate correctly excludes both
ad.Entityandazure.Entitykinds to identify OpenGraph nodes while matching by the indexed property.
110-119: LGTM - Clean fallback pattern for OpenGraph node lookup.The function properly:
- Attempts the standard AD/Azure lookup first
- Distinguishes "not found" errors from actual failures
- Falls back to OpenGraph query only when the node isn't found in standard entities
This maintains backward compatibility while extending support for OpenGraph nodes.
packages/cue/bh/ad/ad.cue (1)
31-31: LGTM - Export declaration for new pathfinding relationship set.packages/go/graphschema/ad/ad.go (1)
1161-1163: LGTM - Generated code matches CUE schema.This generated function correctly reflects the
PathfindingRelationshipsMatchFrontenddeclaration from the CUE schema. Based on learnings, this file is auto-generated from CUE schemas whenjust prepare-for-codereviewis run.cmd/api/src/queries/graph.go (3)
139-139: LGTM - Interface extended for OpenGraph support.The new method signature matches the existing
GetAllShortestPathspattern, maintaining API consistency.
251-279: LGTM - Clean refactoring with pluggable node fetcher.The internal helper effectively abstracts the node-fetching strategy via the
nodeFetcherparameter, enabling code reuse between standard and OpenGraph-aware path lookups. The implementation correctly:
- Applies the filter criteria when provided
- Handles errors from node fetching
- Collects only paths with edges (maintaining existing behavior)
281-289: LGTM - Public methods correctly delegate to internal helper.Both methods appropriately use the measurement decorator and delegate to
getAllShortestPathsInternalwith the correct node fetcher:
GetAllShortestPaths: Usesanalysis.FetchNodeByObjectID(AD/Azure only)GetAllShortestPathsWithOpenGraph: Usesanalysis.FetchNodeByObjectIDIncludeOpenGraph(includes OpenGraph nodes)cmd/api/src/queries/mocks/graph.go (1)
199-212: LGTM - Generated mock matches interface.The mock implementation and recorder method correctly follow the established pattern and match the new
GetAllShortestPathsWithOpenGraphinterface method signature.cmd/api/src/queries/graph_integration_test.go (2)
139-139: LGTM!The change from
"TEST-1"to"2"aligns the test with the updated object-id-based identifiers used in the fixture data.
226-226: LGTM!Consistent with the previous change, using numeric object IDs for exact-match scenarios.
cmd/api/src/api/v2/pathfinding.go (4)
143-155: LGTM!The helper correctly centralizes filter criteria creation. Minor nit: extra blank line at line 154.
169-172: Verify error handling for invalid boolean parameter.When
ParseOptionalBoolfails, the error is logged but execution continues withfalseas the default. Confirm this is the intended behavior—it means clients passing invalid values like"notABool"get silent fallback tofalserather than a 400 response.The test at line 573-597 in pathfinding_test.go confirms this is intentional behavior.
214-222: LGTM!Clean helper function with proper context usage and error handling.
224-247: LGTM!The OpenGraph path retrieval logic correctly:
- Builds traversable filters when
onlyIncludeTraversableKindsis true- Fetches OpenGraph edge kinds from the database
- Extends valid kinds with OpenGraph edges
- Creates the appropriate filter criteria
Minor formatting nit: extra blank lines at 241 and 246-247.
cmd/api/src/api/v2/pathfinding_test.go (3)
107-122: Well-organized test fixtures.The test setup clearly defines the different kind sets and filters needed for various scenarios:
- Built-in kinds (all vs traversable)
- OpenGraph kinds
- Combined kinds with OpenGraph
- Pre-built filter criteria for assertions
This makes the test cases more readable and maintainable.
573-648: Good coverage foronly_traversableparameter behavior.These tests verify:
- Invalid boolean values default to
false(all kinds filter)only_traversable=trueuses traversable kinds filteronly_traversable=truewith relationship_kinds filters to intersection of traversable and requested kindsThe test at line 639 correctly expects only
ad.ContainssinceAZScopedTois not in the traversable set.
956-1036: Comprehensive OpenGraph traversable filtering tests.These tests verify the interaction between
only_traversableand OpenGraph:
- Line 970: DB is queried with
is_traversablefilter- Line 972: Path query uses traversable kinds with OpenGraph
- Lines 982-1008: IN operator with traversable filtering
- Lines 1010-1036: NIN operator with traversable filtering
The mock expectations correctly verify the implementation behavior.
cmd/api/src/queries/fixtures/OpenGraphJSON/raw/base.json (2)
88-165: Edges fixture looks consistent with node IDs.
match_by: "id"references align with existing node IDs and the edge kinds should exercise pathfinding well.
6-85: No action needed. The fixture is correctly structured. Node IDs in the fixture ("id": "7","id": "2", etc.) are properly converted toobjectidproperties when written to the database viaWriteGraphToDatabase, which explicitly setsobjectid = node.ID. Test assertions expecting"objectid": "7"match the fixture's"id": "7"and will work as designed. No consumer updates required.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
AD7ZJ
left a comment
AD7ZJ
left a comment
There was a problem hiding this comment.
One thought, otherwise it looks good! It's working nicely on my local environment.
packages/cue/bh/ad/ad.cue
Outdated
| PathfindingRelationships: list.Concat([SharedRelationshipKinds,[Contains, DCFor, SameForestTrust, SpoofSIDHistory, AbuseTGTDelegation]]) | ||
|
|
||
| // Edges that are used in Shortest Path and match the frontend's list of traversable edges | ||
| PathfindingRelationshipsMatchFrontend: [ |
There was a problem hiding this comment.
Is there a way with CUE to define this list in relation to the existing PathfindingRelationships list? Possibly using the list.Concat method in conjunction with a filter for the 4 edges we don't want?
There was a problem hiding this comment.
I'll try that, thanks for the suggestion!
Do not merge until after RC
Description
shortest-pathendpoint.onlyTraversable, to the endpoint. If this parameter is included, the resulting path will contain its search to only traversable edges. Users will no longer have to pass in a hard-coded list of all the traversable kinds torelationship_kinds🎉NOTE: this work is gated behind a feature-flag, AND is dependent on BED-6723 to populate edge kind data in the
schema-edge-kindstable. This endpoint will not return opengraph data if the opengraph edge kinds are not present in theschema-edge-kindstable. You can manually insert your schema edge kinds in the table if you want to play around with the endpoint with opengraph data in the meantime.Motivation and Context
Resolves BED-6718
This addition will enable users to search for paths between opengraph nodes just like any other AD/AZ node.
How Has This Been Tested?
API tests and graph integration tests added.
Tested locally with some OpenGraph Nodes.
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Documentation
Data
Tests
✏️ Tip: You can customize this high-level summary in your review settings.