feat: add routing filtering to delegated routing server IPIP-484 #671
Conversation
2color
changed the title
routing/http/server/server_test.go
Outdated
| }) | ||
|
|
||
| t.Run("NDJSON Response", func(t *testing.T) { | ||
| runTest(t, mediaTypeNDJSON, false, true, `{"Addrs":[],"ID":"12D3KooWM8sovaEGU1bmiWGWAzvs47DEcXKZZTuJnpQyVTkRs2Vn","Protocols":["transport-bitswap"],"Schema":"peer"}`+"\n"+`{"Schema":"bitswap","Protocol":"transport-bitswap","ID":"12D3KooWM8sovaEGU1bmiWGWAzvs47DEcXKZZTuJnpQyVTkRs2Vz"}`+"\n") | ||
| runTest(t, mediaTypeNDJSON, "", "", false, true, `{"Addrs":["/ip4/127.0.0.1/tcp/4001","/ip4/127.0.0.1/udp/4001/quic-v1","/ip4/127.0.0.1/tcp/4001/ws","/ip4/102.101.1.1/tcp/4001/p2p/12D3KooWEjsGPUQJ4Ej3d1Jcg4VckWhFbhc6mkGunMm1faeSzZMu/p2p-circuit","/ip4/102.101.1.1/udp/4001/quic-v1/webtransport/p2p/12D3KooWEjsGPUQJ4Ej3d1Jcg4VckWhFbhc6mkGunMm1faeSzZMu/p2p-circuit","/ip4/8.8.8.8/udp/4001/quic-v1/webtransport"],"ID":"12D3KooWM8sovaEGU1bmiWGWAzvs47DEcXKZZTuJnpQyVTkRs2Vn","Protocols":["transport-bitswap"],"Schema":"peer"}`+"\n"+`{"Schema":"bitswap","Protocol":"transport-bitswap","ID":"12D3KooWM8sovaEGU1bmiWGWAzvs47DEcXKZZTuJnpQyVTkRs2Vz"}`+"\n") |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actually the tests were wrong (I forgot to update the assertions after changing the test data. This was not a normalisation problem.
The last commits should fix this.
| }) | ||
| } | ||
| func (s *server) findProvidersNDJSON(w http.ResponseWriter, provIter iter.ResultIter[types.Record], filterAddrs, filterProtocols []string) { |
There was a problem hiding this comment.
@2color Any reason why we duplicate this code other than just having filtering on /providers and not /peers?
Thoughts on adding filtering support to /peers and not having this duplicated handler? It feels useful for peer lookups too, at least filter-addrs one ("give me only /wss and /webrtc of this peer"), but supporting both for consistency allows us to be more DRY
There was a problem hiding this comment.
Yeah that's a good idea. One challenge was doing so with generics so I just focused on making it work first.
I'll give it another look.
There was a problem hiding this comment.
I agree we should enable filtering to the /peers.
I had another look at this and haven't been able to find a good solution to reuse the code, due to the combination of generics and the different return types (types.Record and types.PeerRecord)
The main open question I have is why FindProviders and FindPeers have a different return type if they both return a list of PeerRecord:
FindProviders(ctx context.Context, cid cid.Cid, limit int) (iter.ResultIter[types.Record], error)
FindPeers(ctx context.Context, pid peer.ID, limit int) (iter.ResultIter[*types.PeerRecord], error)
Do you have an idea @lidel?
There was a problem hiding this comment.
/routing/v1/peerswill always return results that use Peer schema./routing/v1/providersmay return different schemas, Peer schema is not the only one, so a more generic type is used to allow pass-through in places like https://delegated-ipfs.dev/ which proxy results from other systems.- fysa
cid.contact/routing/v1switched to Peer schema some time ago, making it now confusing why we dont use Peer schema everywhere, and why we waste space for Schema field in every response, but this is [probably both true] (a) a way of having open-ended API that can be evolved by community without breaking existing clients (b) reality of different teams working on different things with different priorities.
- fysa
Personally I see some value in (a), if we keep FindProviders(ctx context.Context, cid cid.Cid, limit int) (iter.ResultIter[types.Record], error) and return generic Record we ensure Schema field is not waste of space, and allow others to experiment without having to invent yet another API.
There was a problem hiding this comment.
@2color To address your task at hand, something we could do:
- keep interface as-is, write reusable things so they return Record, and in FindPeers have a wrapper that translates Records to PeerRecord for now (this way we dont break API)
- this one is preferable, unless we have a good reason to break api (and we dont atm)
- make a breaking change and document in release notes: make
FindPeersreturn genericRecordas a way of deduplicating code boilerplate. Unlikely we will see different schema on/peers, but would not hurt to have that ability.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #671 +/- ##
==========================================
+ Coverage 60.18% 60.24% +0.05%
==========================================
Files 241 242 +1
Lines 30707 30854 +147
==========================================
+ Hits 18482 18587 +105
- Misses 10574 10605 +31
- Partials 1651 1662 +11
|
What's in this PR