Conversation
|
Go test coverage
Total coverage: 56.1% |
There was a problem hiding this comment.
Pull request overview
Adds graph-backed RPC support in the reconciler service to list observed entities (and wires in graph DB access) as part of OBS-1811.
Changes:
- Extend reconciler server construction to accept a
GraphRepositorydependency and gate graph RPCs when unavailable. - Implement
ListEntitiesandCreateEntitygraph endpoints plusListGraphNodesrepository/query support. - Update reconciler-related tests and reconciler
mainwiring to pass the new graph repository parameter.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| diode-server/reconciler/server_test.go | Updates server construction in tests to provide a graph repository mock. |
| diode-server/reconciler/server.go | Adds graph repository field/dependency and wires ListEntities/CreateEntity to new handlers. |
| diode-server/reconciler/mocks/graphrepository.go | Extends the graph repository mock with ListGraphNodes. |
| diode-server/reconciler/graph_repository.go | Extends GraphRepository interface with ListGraphNodes. |
| diode-server/reconciler/graph_endpoints.go | New implementation for ListEntities and CreateEntity handlers and node→entity conversion. |
| diode-server/ingester/component_test.go | Updates reconciler server startup helper to provide graph repository mock. |
| diode-server/gen/dbstore/postgres/graph.sql.go | SQLC-generated code for ListGraphNodes. |
| diode-server/dbstore/postgres/queries/graph.sql | Adds the ListGraphNodes SQL query definition. |
| diode-server/cmd/reconciler/main.go | Wires SQLC queries as GraphRepository into reconciler server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Generate new UUID for external ID | ||
| externalID := uuid.New().String() | ||
|
|
There was a problem hiding this comment.
createEntity always generates a new UUID for externalID, so repeated calls with the same entity will create multiple rows (UpsertGraphNode only conflicts on (node_type, external_id)). This contradicts the RPC comment claiming idempotency and will inflate duplicates. Consider looking up an existing node first (e.g., by content hash/metadata) and reusing its external_id, or otherwise deriving a stable external_id for idempotency.
| // Parse page token (offset encoded as base64) | ||
| offset := int32(0) | ||
| if req.GetPageToken() != "" { | ||
| offsetBytes, err := base64.StdEncoding.DecodeString(req.GetPageToken()) | ||
| if err != nil { |
There was a problem hiding this comment.
Pagination token handling here duplicates logic and uses a different encoding (base64 of an ASCII integer) than the existing deviation pagination helpers (binary int32 + base64 in deviation.go). This inconsistency makes client implementations harder and increases maintenance cost. Consider reusing a shared helper or aligning token formats across endpoints.
| func listEntities(ctx context.Context, graphdb GraphRepository, req *reconcilerpb.ListEntitiesRequest) (*reconcilerpb.ListEntitiesResponse, error) { | ||
| // Parse pagination parameters | ||
| pageSize := defaultPageSize | ||
| if req.GetPageSize() > 0 { | ||
| pageSize = int(req.GetPageSize()) |
There was a problem hiding this comment.
New RPC behavior in createEntity/listEntities is added without unit tests. Please add tests using mocks.GraphRepository to cover pagination token parsing (valid/invalid), filter passthrough to ListGraphNodes, next page token generation, and create-entity idempotency/error paths.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
Per OBS-1811