Skip to content

Commit 62a5aad

Browse files
committed
fix: wire dht router to composableRouter and return error when DHT disabled
fixes two issues with the GetClosestPeers endpoint: 1. endpoint returned HTTP 200 with empty results instead of actual DHT peers 2. when DHT disabled, returned HTTP 200 with empty results instead of error root cause: during rebase of PR #124 (commit 0dad3f7) when integrating with autoconf refactor (PR #123 / ec76365), the dhtRouters initialization was accidentally omitted from server.go. the autoconf PR renamed getCombinedRouting to combineRouters and changed its signature, but the rebase failed to preserve the dhtRouters creation line that existed in commit 42bd221. changes: - server.go:201-208: add explicit dhtRouters creation with caching and sanitization, similar to original 42bd221 implementation - server.go:232: wire dhtRouters to composableRouter.dht field - server.go:338: update combineRouters signature to accept host.Host parameter needed for GetClosestPeers peerstore lookups - server_routers.go:73-77: return routing.ErrNotSupported instead of empty iterator when DHT is nil, resulting in HTTP 500 instead of misleading HTTP 200 - server_test.go:16,20,24: update combineRouters test calls with new signature - server_dht_test.go:355-379: add test verifying HTTP 500 when DHT disabled
1 parent 43188dd commit 62a5aad

File tree

4 files changed

+48
-10
lines changed

4 files changed

+48
-10
lines changed

server.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,18 @@ func start(ctx context.Context, cfg *config) error {
194194
}
195195

196196
// Combine HTTP routers with DHT and additional routers
197-
crRouters := combineRouters(dhtRouting, cachedAddrBook, providerHTTPRouters, blockProviderRouters)
198-
prRouters := combineRouters(dhtRouting, cachedAddrBook, peerHTTPRouters, nil)
199-
ipnsRouters := combineRouters(dhtRouting, cachedAddrBook, ipnsHTTPRouters, nil)
197+
crRouters := combineRouters(h, dhtRouting, cachedAddrBook, providerHTTPRouters, blockProviderRouters)
198+
prRouters := combineRouters(h, dhtRouting, cachedAddrBook, peerHTTPRouters, nil)
199+
ipnsRouters := combineRouters(h, dhtRouting, cachedAddrBook, ipnsHTTPRouters, nil)
200+
201+
// Create DHT router for GetClosestPeers endpoint
202+
var dhtRouters router
203+
if cachedAddrBook != nil && dhtRouting != nil {
204+
cachedRouter := NewCachedRouter(libp2pRouter{host: h, routing: dhtRouting}, cachedAddrBook)
205+
dhtRouters = sanitizeRouter{cachedRouter}
206+
} else if dhtRouting != nil {
207+
dhtRouters = sanitizeRouter{libp2pRouter{host: h, routing: dhtRouting}}
208+
}
200209

201210
_, port, err := net.SplitHostPort(cfg.listenAddress)
202211
if err != nil {
@@ -220,6 +229,7 @@ func start(ctx context.Context, cfg *config) error {
220229
providers: crRouters,
221230
peers: prRouters,
222231
ipns: ipnsRouters,
232+
dht: dhtRouters,
223233
}, handlerOpts...)
224234

225235
// Add CORS.
@@ -325,14 +335,14 @@ func newHost(cfg *config) (host.Host, error) {
325335

326336
// combineRouters combines delegated HTTP routers with DHT and additional routers.
327337
// It no longer creates HTTP clients (that's done in createDelegatedHTTPRouters).
328-
func combineRouters(dht routing.Routing, cachedAddrBook *cachedAddrBook, delegatedRouters, additionalRouters []router) router {
338+
func combineRouters(h host.Host, dht routing.Routing, cachedAddrBook *cachedAddrBook, delegatedRouters, additionalRouters []router) router {
329339
var dhtRouter router
330340

331341
if cachedAddrBook != nil {
332-
cachedRouter := NewCachedRouter(libp2pRouter{routing: dht}, cachedAddrBook)
342+
cachedRouter := NewCachedRouter(libp2pRouter{host: h, routing: dht}, cachedAddrBook)
333343
dhtRouter = sanitizeRouter{cachedRouter}
334344
} else if dht != nil {
335-
dhtRouter = sanitizeRouter{libp2pRouter{routing: dht}}
345+
dhtRouter = sanitizeRouter{libp2pRouter{host: h, routing: dht}}
336346
}
337347

338348
if len(delegatedRouters) == 0 && len(additionalRouters) == 0 {

server_dht_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,32 @@ func TestGetClosestPeersEndpoint(t *testing.T) {
351351
require.Equal(t, http.StatusOK, resp.StatusCode)
352352
require.Equal(t, mediaTypeJSON, resp.Header.Get("Content-Type"))
353353
})
354+
355+
t.Run("GET /routing/v1/dht/closest/peers/{cid} returns 500 when DHT is disabled", func(t *testing.T) {
356+
t.Parallel()
357+
358+
_, pid := makeEd25519PeerID(t)
359+
key := peer.ToCid(pid)
360+
361+
// Pass nil router to simulate DHT disabled via --dht=disabled
362+
handler := server.Handler(&composableRouter{dht: nil})
363+
srv := httptest.NewServer(handler)
364+
t.Cleanup(srv.Close)
365+
366+
urlStr := fmt.Sprintf("http://%s/routing/v1/dht/closest/peers/%s", srv.Listener.Addr().String(), key.String())
367+
req, err := http.NewRequest(http.MethodGet, urlStr, nil)
368+
require.NoError(t, err)
369+
req.Header.Set("Accept", mediaTypeJSON)
370+
371+
resp, err := http.DefaultClient.Do(req)
372+
require.NoError(t, err)
373+
374+
// Returns 500 Internal Server Error instead of misleading 200 with empty results
375+
require.Equal(t, http.StatusInternalServerError, resp.StatusCode)
376+
body, err := io.ReadAll(resp.Body)
377+
require.NoError(t, err)
378+
require.Contains(t, string(body), "not supported")
379+
})
354380
}
355381

356382
// mockDHTRouter implements the router interface for testing

server_routers.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@ func (r composableRouter) FindPeers(ctx context.Context, pid peer.ID, limit int)
7171

7272
func (r composableRouter) GetClosestPeers(ctx context.Context, key cid.Cid) (iter.ResultIter[*types.PeerRecord], error) {
7373
if r.dht == nil {
74-
return iter.ToResultIter(iter.FromSlice([]*types.PeerRecord{})), nil
74+
// Return ErrNotSupported when no DHT is available (e.g., disabled via --dht=disabled CLI param).
75+
// This returns HTTP 501 Not Implemented instead of misleading HTTP 200 with empty results.
76+
return nil, routing.ErrNotSupported
7577
}
7678
return r.dht.GetClosestPeers(ctx, key)
7779
}

server_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ func TestCombineRouters(t *testing.T) {
1313
mockRouter := composableRouter{}
1414

1515
// Check that combineRouters with DHT only returns sanitizeRouter
16-
v := combineRouters(&bundledDHT{}, nil, nil, nil)
16+
v := combineRouters(nil, &bundledDHT{}, nil, nil, nil)
1717
require.IsType(t, sanitizeRouter{}, v)
1818

1919
// Check that combineRouters with delegated routers only returns parallelRouter
20-
v = combineRouters(nil, nil, []router{mockRouter}, nil)
20+
v = combineRouters(nil, nil, nil, []router{mockRouter}, nil)
2121
require.IsType(t, parallelRouter{}, v)
2222

2323
// Check that combineRouters with both DHT and delegated routers returns parallelRouter
24-
v = combineRouters(&bundledDHT{}, nil, []router{mockRouter}, nil)
24+
v = combineRouters(nil, &bundledDHT{}, nil, []router{mockRouter}, nil)
2525
require.IsType(t, parallelRouter{}, v)
2626
}

0 commit comments

Comments
 (0)