feat: Add KindInfo to api/v2/nodes/node_id - BED-8762#2982
Conversation
brandonshearin
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe node retrieval path accepts an optional ChangesNode kind-information persistence
Conditional node aggregation
HTTP query and response rendering
Node response API contract
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetNodeByID
participant Service.GetNode
participant Database.GetKindInfos
Client->>GetNodeByID: Request node with include-info=true
GetNodeByID->>Service.GetNode: GetNode(id, true)
Service.GetNode->>Database.GetKindInfos: Fetch kind information by name
Database.GetKindInfos-->>Service.GetNode: Return kind information
Service.GetNode-->>GetNodeByID: Return node with KindInfos
GetNodeByID-->>Client: Return NodeView with info
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
server/graphdb/internal/services/node.go (2)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the underlying error with
%wto preserve the error chain.
fmt.Errorf("failed to get kind info for %s", nodeKind.Name)discards the original error fromGetKindInfos. This impairs debugging and preventserrors.Is/errors.Asfrom matching any sentinel errors the DB layer might return.🔧 Proposed fix
- return Node{}, fmt.Errorf("failed to get kind info for %s", nodeKind.Name) + return Node{}, fmt.Errorf("failed to get kind info for %s: %w", nodeKind.Name, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/services/node.go` at line 76, Update the error return in the GetKindInfos handling within the node service to capture the underlying error and wrap it with %w, while retaining nodeKind.Name in the contextual message so errors.Is and errors.As can traverse the original error.
69-80: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider batching
GetKindInfoscalls to avoid sequential per-kind DB round-trips.Each kind triggers a separate
GetKindInfosquery. For nodes with many kinds, this adds latency on the request path. A batch query accepting multiple kind names would reduce this to a single round-trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/services/node.go` around lines 69 - 80, Replace the sequential per-kind GetKindInfos calls in the node processing loop with a batched database call that accepts all non-nil nodeKind.Name values and returns their kind information in one round-trip. Preserve the existing filtering and error context, then append the batched results to allKindInfos.server/graphdb/internal/services/node_test.go (1)
164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeriving
includeKindInfofromwantResult.KindInfoslimits test coverage.This coupling means the test can't verify the
includeKindInfo=truepath when no kind infos are found (empty result). Consider adding an explicitincludeKindInfofield to the test struct for independent control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/services/node_test.go` at line 164, The GetNode test currently derives includeKindInfo from wantResult.KindInfos, preventing coverage of the true path with empty kind information. Add an explicit includeKindInfo field to the test cases and pass it to svc.GetNode, updating existing cases to preserve their intended behavior and adding coverage for includeKindInfo=true with no kind infos.server/graphdb/internal/handlers/node.go (1)
98-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildMarkdownViewsilently swallows unmarshal errors.Returning an empty
MarkdownView{}on failure is reasonable for graceful degradation, but a log at debug/warn level would help diagnose malformedKindInfo.Contentin the database without affecting the response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/handlers/node.go` around lines 98 - 106, Add debug- or warn-level logging in buildMarkdownView when json.Unmarshal fails, including the unmarshal error and enough context to identify the malformed content, while preserving the existing graceful fallback of returning an empty MarkdownView{}.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/graphdb/internal/appdb/kindinfo.go`:
- Line 89: Update the OrderBy call in the relevant query builder to qualify both
sort columns with the ki. alias, using ki.position and ki.title, consistent with
the existing SELECT expressions.
In `@server/graphdb/internal/handlers/node_test.go`:
- Around line 103-105: Cover the nil NodeKindID case in the test fixture and add
an assertion for the resulting node view. Update BuildNodeView to check
kindInfo.NodeKindID before dereferencing it, handling nil safely while
preserving the existing behavior for non-nil IDs.
In `@server/graphdb/internal/handlers/node.go`:
- Line 89: Guard against nil KindInfo.NodeKindID values in the service-layer
merge logic before entries reach the handler; filter out relationship kind infos
with nil IDs. Verify the handler’s NodeKindID conversion is only performed for
entries passing this guard.
In `@server/graphdb/internal/services/node_test.go`:
- Around line 60-126: The commented-out cases in the node test table leave error
and edge-case behavior untested. Update their mocks and expectations to match
the current GetNode signature, then re-enable tests for ErrNodeNotFound,
unexpected database errors, single-kind nodes, and mixed resolved/unresolved
kinds; remove them only if equivalent replacement coverage exists.
In `@server/graphdb/internal/services/node.go`:
- Around line 82-90: Prevent nil NodeKindID dereferences in both the
allKindInfos sorting logic and the downstream handler conversion. Filter out
KindInfo entries with nil NodeKindID before sorting in the relevant node service
function, and ensure handlers/node.go only converts validated non-nil IDs,
preserving valid node kinds while avoiding relationship-kind entries.
---
Nitpick comments:
In `@server/graphdb/internal/handlers/node.go`:
- Around line 98-106: Add debug- or warn-level logging in buildMarkdownView when
json.Unmarshal fails, including the unmarshal error and enough context to
identify the malformed content, while preserving the existing graceful fallback
of returning an empty MarkdownView{}.
In `@server/graphdb/internal/services/node_test.go`:
- Line 164: The GetNode test currently derives includeKindInfo from
wantResult.KindInfos, preventing coverage of the true path with empty kind
information. Add an explicit includeKindInfo field to the test cases and pass it
to svc.GetNode, updating existing cases to preserve their intended behavior and
adding coverage for includeKindInfo=true with no kind infos.
In `@server/graphdb/internal/services/node.go`:
- Line 76: Update the error return in the GetKindInfos handling within the node
service to capture the underlying error and wrap it with %w, while retaining
nodeKind.Name in the contextual message so errors.Is and errors.As can traverse
the original error.
- Around line 69-80: Replace the sequential per-kind GetKindInfos calls in the
node processing loop with a batched database call that accepts all non-nil
nodeKind.Name values and returns their kind information in one round-trip.
Preserve the existing filtering and error context, then append the batched
results to allKindInfos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cc451e67-3e68-40f5-8856-e55d00483624
📒 Files selected for processing (9)
server/graphdb/internal/appdb/kindinfo.goserver/graphdb/internal/handlers/handlers.goserver/graphdb/internal/handlers/mocks/graphdb.goserver/graphdb/internal/handlers/node.goserver/graphdb/internal/handlers/node_test.goserver/graphdb/internal/services/mocks/database.goserver/graphdb/internal/services/node.goserver/graphdb/internal/services/node_test.goserver/graphdb/internal/services/services.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/go/openapi/doc/openapi.json`:
- Around line 23220-23230: Update the InfoMarkdown schema so its markdown
property and additionalProperties: false constraint are defined in the same
object schema; avoid placing the constraint in a separate allOf branch. Preserve
the InfoMarkdownContent fields, either by inlining them or merging the object
definition, so KindInfoContent and NodeDetailsWithInfo validate real markdown
payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 25ff9e96-5607-48fa-83b0-9c1f831256e6
📒 Files selected for processing (3)
packages/go/openapi/doc/openapi.jsonserver/graphdb/internal/handlers/node.goserver/graphdb/internal/handlers/node_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/graphdb/internal/handlers/node_test.go
- server/graphdb/internal/handlers/node.go
brandonshearin
changed the title
coderabbitai
Bot
added
the
infrastructure
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/graphdb/internal/services/node_test.go (1)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
includeKindInfofield and error-path test cases.Deriving
includeKindInfofromtt.wantResult.KindInfos != nilmakes it impossible to testincludeKindInfo=truewhen the expected result has nilKindInfos— precisely the scenario forGetKindInfoserror propagation. TheGetNodeimplementation has two untested error paths:GetNodeKindsByNamesfailures andGetKindInfosfailures.Note: the
GetKindInfoserror wrapping innode.gousesfmt.Errorf("failed to get kind info for %s", nodeKind.Name)without%w, soerrors.Iswon't match the original error. Adding the test below would expose this — the wrapping should use%wto preserve the error chain.♻️ Proposed refactor
tests := []struct { name string setupMock func(databaseMock *mocks.MockDatabase) wantResult services.Node wantErr error + includeKindInfo bool }{- result, err := svc.GetNode(ctx, nodeID, tt.wantResult.KindInfos != nil) + result, err := svc.GetNode(ctx, nodeID, tt.includeKindInfo)Add these test cases to cover the missing error paths:
+ { + name: "error_-_propagates_get_node_kinds_by_names_error", + setupMock: func(databaseMock *mocks.MockDatabase) { + databaseMock.EXPECT().GetNode(ctx, nodeID).Return(baseNode, nil) + databaseMock.EXPECT().GetNodeKindsByNames(ctx, []string{"User", "Group"}).Return(nil, unexpectedErr) + }, + wantErr: unexpectedErr, + }, + { + name: "error_-_propagates_get_kind_infos_error", + setupMock: func(databaseMock *mocks.MockDatabase) { + databaseMock.EXPECT().GetNode(ctx, nodeID).Return(baseNode, nil) + databaseMock.EXPECT().GetNodeKindsByNames(ctx, []string{"User", "Group"}).Return(resolvedKinds, nil) + databaseMock.EXPECT().GetKindInfos(ctx, "User").Return(nil, unexpectedErr) + }, + wantErr: unexpectedErr, + includeKindInfo: true, + },Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/services/node_test.go` around lines 54 - 59, Add an explicit includeKindInfo boolean to the node test table and use it when calling GetNode, rather than deriving it from wantResult.KindInfos. Add cases covering GetNodeKindsByNames and GetKindInfos failures, including the scenario where includeKindInfo is true but expected KindInfos is nil. Update GetKindInfos error wrapping in GetNode to use %w so errors.Is preserves the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/graphdb/internal/services/node_test.go`:
- Around line 54-59: Add an explicit includeKindInfo boolean to the node test
table and use it when calling GetNode, rather than deriving it from
wantResult.KindInfos. Add cases covering GetNodeKindsByNames and GetKindInfos
failures, including the scenario where includeKindInfo is true but expected
KindInfos is nil. Update GetKindInfos error wrapping in GetNode to use %w so
errors.Is preserves the original error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0b1d9293-0129-4f18-855b-d81fe7d5f577
📒 Files selected for processing (1)
server/graphdb/internal/services/node_test.go
coderabbitai
Bot
removed
the
infrastructure
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/go/openapi/doc/openapi.json (1)
23066-23230: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInline
InfoMarkdownbefore closing the schemapackages/go/openapi/doc/openapi.json:InfoMarkdownstill wrapsInfoMarkdownContentin a separateallOfbranch withadditionalProperties: false, somarkdownis treated as an extra property. Inline the content into the same object or switch tounevaluatedProperties: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/openapi/doc/openapi.json` around lines 23066 - 23230, Fix the InfoMarkdown schema so its inherited markdown field is not rejected as an extra property: update InfoMarkdown to inline the InfoMarkdownContent fields into one object, or replace additionalProperties: false with unevaluatedProperties: false while preserving the existing required markdown/content structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/go/openapi/doc/openapi.json`:
- Around line 23066-23230: Fix the InfoMarkdown schema so its inherited markdown
field is not rejected as an extra property: update InfoMarkdown to inline the
InfoMarkdownContent fields into one object, or replace additionalProperties:
false with unevaluatedProperties: false while preserving the existing required
markdown/content structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7df9ec56-d9f0-4f61-a51e-811275e7c72a
📒 Files selected for processing (1)
packages/go/openapi/doc/openapi.json
|
does this still need openapi doc updates? |
|
@seanjSO i don't think so |
|
Just a heads up, I think we will want to update the open api docs, it didnt include the kind info object in the original implementation. |
| // GetKindInfos returns all KindInfo's associated with the given kind name, | ||
| // ordered by position then title. An empty slice is returned when no rows match. | ||
| func (s *Store) GetKindInfos(ctx context.Context, kindID int32) ([]services.KindInfo, error) { | ||
| func (s *Store) GetKindInfos(ctx context.Context, kindName string) ([]services.KindInfo, error) { |
There was a problem hiding this comment.
Not blocking: Any reason why we are using name instead of the kind id?
There was a problem hiding this comment.
Yes -- i waffled between a couple approaches to this.
The reason I picked kind name: in order to avoid a GetKindInfos() method signature that had nullable *nodeKindID, *relationshipKindID parameters.
More background: The consumer of this function is services.GetNode. That caller does not have a dawgs ID in scope, but it does have a 1. kind name or 2. schema_node_kind ID. Either would work, but the latter would require the services.GetRelationship function to pass its ID into a different slot, which was too messy.
@LawsonWillard I updated the docs -- the response definition and examples include kind infos. is there something else i left out? |
3 participants
Description
KindInfoViewto handlers/node.goKindInfosMotivation and Context
Resolves BED-8762
We want to extend the existing
api/v2/nodes/{node_id}endpoint with a query param namedinclude-info. When set to true, it pulls the associated entity panels for the node's kinds.How Has This Been Tested?
You can see the API shape in the Swagger UI.
schema_node_kindtable with a few records that correspond to a kind on the node you picked in step 2.Screenshots (optional):
Example API response:
Types of changes
Checklist:
Summary by CodeRabbit
GET /api/v2/nodes/{node_id}can now optionally include per-kind “info” metadata (with markdown content) wheninclude-info=true.include-infovalues now return HTTP 400.GET /api/v2/nodes/{node_id}to document the new optional response shape and examples.