chore: no tenant_id on similar search#82
Conversation
✱ Stainless preview buildsThis PR will update the Edit this comment to update it. It will appear in the SDK's changelogs. ✅ hub-openapi studio · code · diff
✅ hub-typescript studio · code · diff
This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
WalkthroughThis PR refactors the similarity search endpoint to derive tenant context from the source feedback record instead of requiring an explicit 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/handlers/search_handler.go (1)
164-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
service.ErrMissingTenantIDexplicitly.The service layer can return
ErrMissingTenantIDwhen the source feedback record has an emptytenant_id(persearch_service.go:223-225). This error is not handled in the current error chain (lines 166-176), so it falls through to the generic error handler at line 178, likely resulting in a 500 response.This is a data-integrity issue (feedback record exists but has invalid/empty tenant), and should return a more appropriate status:
- 404 Not Found with a message like "Source feedback record not found or has no tenant"
- OR 400 Bad Request if you want to surface the data inconsistency to the caller
🛡️ Suggested fix to handle missing tenant explicitly
res, err := h.service.SimilarFeedback(r.Context(), id, limit, minScore, cursor) if err != nil { + if errors.Is(err, service.ErrMissingTenantID) { + response.RespondNotFound(w, r, "Source feedback record has no tenant or was not found") + + return + } + if errors.Is(err, service.ErrEmbeddingNotFound) { response.RespondNotFound(w, r, "Feedback record has no embedding for the current model")Based on learnings: Treat
tenant_idas a security boundary. When a source record exists but has no tenant, explicitly handle it rather than allowing a generic error that could mask tenant-boundary violations.🤖 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 `@internal/api/handlers/search_handler.go` around lines 164 - 181, The error from h.service.SimilarFeedback can be ErrMissingTenantID and must be handled explicitly; add a branch checking errors.Is(err, service.ErrMissingTenantID) (alongside the existing ErrEmbeddingNotFound and ErrInvalidCursor checks) and call response.RespondNotFound(w, r, "Source feedback record not found or has no tenant") (or RespondInvalidParams if you prefer 400) before falling through to the generic response.RespondError; reference the service.ErrMissingTenantID symbol and the handler invocation h.service.SimilarFeedback to locate where to insert this check.internal/api/handlers/search_handler_test.go (1)
162-207: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for empty tenant scenario.
The tests verify the happy path (tenant derived successfully) and the embedding-not-found case, but don't cover the scenario where the source feedback record exists but has an empty
tenant_id. The service can returnservice.ErrMissingTenantIDin this case (persearch_service.go:223-225), which should be tested.🧪 Suggested test case for empty tenant
t.Run("source record with empty tenant returns appropriate error", func(t *testing.T) { id := uuid.MustParse("018e1234-5678-9abc-def0-123456789abc") mock := &mockSearchService{ similarFunc: func(_ context.Context, fid uuid.UUID, _ int, _ float64, _ string) (service.SearchResult, error) { assert.Equal(t, id, fid) return service.SearchResult{}, service.ErrMissingTenantID }, } handler := NewSearchHandler(mock) req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, similarURL, nil) req.SetPathValue("id", id.String()) rec := httptest.NewRecorder() handler.SimilarFeedback(rec, req) // Should return 404 or 400 depending on handler implementation // Currently falls through to generic error handler (likely 500) assert.Contains(t, []int{http.StatusNotFound, http.StatusBadRequest}, rec.Code) })Based on learnings: For any tenant-scoping change, include verification at the boundary where the leak could happen. This test would verify proper handling when the tenant boundary is absent from the source record.
🤖 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 `@internal/api/handlers/search_handler_test.go` around lines 162 - 207, Add a test in internal/api/handlers/search_handler_test.go that exercises the SimilarFeedback handler when the service returns service.ErrMissingTenantID: create a mockSearchService.similarFunc that asserts the incoming id (use the same uuid as other tests), returns (service.SearchResult{}, service.ErrMissingTenantID), call NewSearchHandler(mock).SimilarFeedback with a request having req.SetPathValue("id", id.String()), record the response, and assert the response code is an appropriate error (e.g., 400 or 404 depending on handler behavior); this covers the "source record with empty tenant" branch referenced by service.ErrMissingTenantID.
🤖 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 `@internal/api/handlers/search_handler_test.go`:
- Around line 162-207: Add a test in
internal/api/handlers/search_handler_test.go that exercises the SimilarFeedback
handler when the service returns service.ErrMissingTenantID: create a
mockSearchService.similarFunc that asserts the incoming id (use the same uuid as
other tests), returns (service.SearchResult{}, service.ErrMissingTenantID), call
NewSearchHandler(mock).SimilarFeedback with a request having
req.SetPathValue("id", id.String()), record the response, and assert the
response code is an appropriate error (e.g., 400 or 404 depending on handler
behavior); this covers the "source record with empty tenant" branch referenced
by service.ErrMissingTenantID.
In `@internal/api/handlers/search_handler.go`:
- Around line 164-181: The error from h.service.SimilarFeedback can be
ErrMissingTenantID and must be handled explicitly; add a branch checking
errors.Is(err, service.ErrMissingTenantID) (alongside the existing
ErrEmbeddingNotFound and ErrInvalidCursor checks) and call
response.RespondNotFound(w, r, "Source feedback record not found or has no
tenant") (or RespondInvalidParams if you prefer 400) before falling through to
the generic response.RespondError; reference the service.ErrMissingTenantID
symbol and the handler invocation h.service.SimilarFeedback to locate where to
insert this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f1a54ec-8669-4b55-b7b0-eb0775c58273
📒 Files selected for processing (6)
internal/api/handlers/search_handler.gointernal/api/handlers/search_handler_test.gointernal/repository/embeddings_repository.gointernal/service/search_service.gointernal/service/search_service_test.goopenapi.yaml
1 participant
What does this PR do?
Updates the similar feedback endpoint so callers no longer need to pass
tenant_idwhen looking up records similar to a source feedback record.Hub now derives the tenant boundary from the source feedback record itself, then uses that exact stored tenant ID to scope the nearest-neighbor embedding search. This removes a redundant public parameter while preserving tenant isolation internally.
Changes include:
tenant_idfromGET /v1/feedback-records/{id}/similarFixes #(issue)
How should this be tested?
go test ./internal/api/handlers ./internal/service ./internal/repository ./cmd/api ./cmd/worker/Users/xernobyl/go/bin/golangci-lint run ./...Checklist
Required
make buildmake tests(integration tests intests/)make fmtandmake lint; no new warningsgit pull origin mainmigrations/with goose annotations and ranmake migrate-validateAppreciated
make testsor API contract workflow)docs/if changes were necessarymake tests-coveragefor meaningful logic changes