feat[Go]: implement GET /dify/retrieval/health (issue #15240)#15571
feat[Go]: implement GET /dify/retrieval/health (issue #15240)#15571dripsmvcp wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
size:M
|
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:
📝 WalkthroughWalkthroughAdds an unauthenticated Dify retrieval health endpoint: implements DifyHandler.RetrievalHealth with tests, registers GET /api/v1/dify/retrieval/health in the router, and initializes the handler during server startup. ChangesDify Retrieval Health Check
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/handler/dify.go (1)
47-53: 💤 Low valueOptional: reuse the shared
jsonResponsehelper.
internal/handler/kb.goalready exposesjsonResponse(c, code, data, message)for the standard envelope. Adopting it keeps the response shape consistent across handlers. Note the hardcoded lowercase"success"is intentional and must be preserved—common.CodeSuccess.Message()returns"Success"(capitalized), so pass the literal explicitly rather than deriving it.🤖 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/handler/dify.go` around lines 47 - 53, Replace the direct c.JSON response in RetrievalHealth with the shared jsonResponse helper: call jsonResponse(c, common.CodeSuccess, true, "success") so the envelope is consistent with other handlers; keep the literal lowercase "success" string (do not use common.CodeSuccess.Message()). Ensure you update the RetrievalHealth method to use jsonResponse rather than constructing gin.H directly.
🤖 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 `@internal/handler/dify_test.go`:
- Around line 59-79: TestDifyRetrievalHealthDoesNotRequireAuth currently
attaches NewDifyHandler().RetrievalHealth to a bare gin.New() so it doesn't
exercise the AuthMiddleware; change the test to create the application's router
via Router.Setup (so AuthMiddleware is registered) and then register the dify
retrieval health route through the public/no-auth group used by your
Router.Setup to confirm the route bypasses auth, and also handle the
json.Unmarshal error instead of discarding it (fail the test if unmarshalling
the response body returns an error) so a malformed body doesn't mask failures.
In `@internal/handler/dify.go`:
- Around line 36-46: Update the Swagger/method comment in the RetrievalHealth
handler so the documented path matches the actual mounted route; change the
method comment and the `@Router` annotation that currently reference
"/v1/dify/retrieval/health" to the full mounted path
"/api/v1/dify/retrieval/health" (modify the comment block around the
RetrievalHealth function and its `@Router` tag) to ensure generated Swagger
reflects the real endpoint.
---
Nitpick comments:
In `@internal/handler/dify.go`:
- Around line 47-53: Replace the direct c.JSON response in RetrievalHealth with
the shared jsonResponse helper: call jsonResponse(c, common.CodeSuccess, true,
"success") so the envelope is consistent with other handlers; keep the literal
lowercase "success" string (do not use common.CodeSuccess.Message()). Ensure you
update the RetrievalHealth method to use jsonResponse rather than constructing
gin.H directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5af1182-54bb-4ca9-81fd-f1c59bb7360e
📒 Files selected for processing (4)
cmd/server_main.gointernal/handler/dify.gointernal/handler/dify_test.gointernal/router/router.go
| func TestDifyRetrievalHealthDoesNotRequireAuth(t *testing.T) { | ||
| // Public probe: must succeed even with no user attached to the context. | ||
| gin.SetMode(gin.TestMode) | ||
|
|
||
| r := gin.New() | ||
| r.GET("/api/v1/dify/retrieval/health", NewDifyHandler().RetrievalHealth) | ||
|
|
||
| resp := httptest.NewRecorder() | ||
| req := httptest.NewRequest(http.MethodGet, "/api/v1/dify/retrieval/health", nil) | ||
| r.ServeHTTP(resp, req) | ||
|
|
||
| if resp.Code != http.StatusOK { | ||
| t.Fatalf("status=%d body=%s", resp.Code, resp.Body.String()) | ||
| } | ||
|
|
||
| var body map[string]interface{} | ||
| _ = json.Unmarshal(resp.Body.Bytes(), &body) | ||
| if code, _ := body["code"].(float64); int(code) != int(common.CodeSuccess) { | ||
| t.Errorf("unauthenticated probe should succeed, got code=%v", body["code"]) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestDifyRetrievalHealthDoesNotRequireAuth doesn't exercise the no-auth contract.
This test mounts the handler on a bare gin.New() router with no auth middleware, so it's functionally identical to TestDifyRetrievalHealthReturnsTrueEnvelope and proves nothing about authentication. To actually validate that the probe is public, the route should be reached through a router that has AuthMiddleware() applied (i.e., via Router.Setup) confirming the no-auth group bypasses it—otherwise this is a duplicate.
Also, the json.Unmarshal error is silently discarded at Line 75; on a malformed body the assertion would read a zeroed code and could mask a real failure.
🤖 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/handler/dify_test.go` around lines 59 - 79,
TestDifyRetrievalHealthDoesNotRequireAuth currently attaches
NewDifyHandler().RetrievalHealth to a bare gin.New() so it doesn't exercise the
AuthMiddleware; change the test to create the application's router via
Router.Setup (so AuthMiddleware is registered) and then register the dify
retrieval health route through the public/no-auth group used by your
Router.Setup to confirm the route bypasses auth, and also handle the
json.Unmarshal error instead of discarding it (fail the test if unmarshalling
the response body returns an error) so a malformed body doesn't mask failures.
dripsmvcp
force-pushed
the
feat/go-api-dify-health
branch
from
5c76c8e to
06aceaf
Compare
|
This PR looks well-scoped and correctly aligned with the Python implementation. A few observations: Looks good:
Things to verify:
Overall this is a clean, minimal addition. The main risk is getting the route path exactly right so Dify's connector probe hits it without a prefix mismatch. To reply, just mention @dosu. Docs are dead. Just use Dosu.
|
|
@Hz-186 Would you check this PR too? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15571 +/- ##
=======================================
Coverage 93.16% 93.16%
=======================================
Files 10 10
Lines 717 717
Branches 118 118
=======================================
Hits 668 668
Misses 29 29
Partials 20 20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dripsmvcp
force-pushed
the
feat/go-api-dify-health
branch
from
ed1908e to
5dcff85
Compare
dripsmvcp
force-pushed
the
feat/go-api-dify-health
branch
from
5dcff85 to
bf8f260
Compare
dosubot
Bot
added
size:L
dripsmvcp
force-pushed
the
feat/go-api-dify-health
branch
from
bf8f260 to
775194c
Compare
dosubot
Bot
added
size:M
|
@Haruko386 Would you double check this review? |
|
@dripsmvcp Would you please fix the conflicts? |
|
Will do it @JinHai-CN |
|
Closing as superseded by #15704 ("Dify-compatible retrieval API endpoint"), which merged into This PR's |
3 participants
Summary
Port the Dify external-knowledge-base reachability probe to the Go API server. Listed in the Go-API port checklist of #15240.
Dify calls this endpoint to verify that the RAGFlow external-knowledge connector is reachable before the user configures any credentials, so it returns a constant success envelope and stays on the public route group.
What
internal/handler/dify.go— newDifyHandler.RetrievalHealthreturning the same{"code":0, "message":"success", "data":true}envelope the Python endpoint emits.internal/router/router.go— wireapiNoAuth.GET("/dify/retrieval/health", ...). Public by design, matching the undecorated Python route.cmd/server_main.go— instantiate and injectDifyHandler.internal/handler/dify_test.go— unit tests for the envelope shape and confirmation that the probe succeeds without an authenticated user.Python source
api/apps/restful_apis/dify_retrieval_api.py:Test plan
go vet ./internal/handler ./internal/routercleango test ./internal/handlerwith cgo binding linkedRelated