Skip to content

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 11, 2025

Description

Implements IPIP-513 spec changes for delegated routing v1 endpoints to return 200 with empty results instead of 404 + some extra fixes to ensure client interop with broad type of servers.

changes

  • server returns 200 instead of 404 for empty results
    • providers: empty JSON/NDJSON array
    • peers: empty JSON/NDJSON array
    • ipns: text/plain "Record not found"
  • client handles both 200 and 404 responses for backward compatibility
  • client handles null/undefined Providers/Peers fields

test coverage

  • added tests for 404 backward compatibility (old servers)
  • added tests for null/undefined field handling
  • added tests for IPNS content-type validation
  • test fixtures use special CIDs to trigger edge cases

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

changes:
- server returns 200 instead of 404 for empty results
  - providers: empty JSON/NDJSON array
  - peers: empty JSON/NDJSON array
  - ipns: text/plain "Record not found"
- client handles both 200 and 404 responses for backward compatibility
- client handles null/undefined Providers/Peers fields

test coverage:
- added tests for 404 backward compatibility (old servers)
- added tests for null/undefined field handling
- added tests for IPNS content-type validation
- test fixtures use special CIDs to trigger edge cases
@lidel lidel changed the title feat: implement IPIP-513 (200 instead of 404) feat(routing/http): return 200 for empty results per IPIP-513 Sep 11, 2025
@lidel lidel force-pushed the feat/ipip-513-empty-200s branch from 53b7481 to 1459f1d Compare September 11, 2025 05:00
@lidel lidel requested a review from achingbrain September 11, 2025 06:17
@lidel lidel marked this pull request as ready for review September 11, 2025 06:17
@lidel lidel requested a review from a team as a code owner September 11, 2025 06:17
const peerCid = CID.parse(cidStr)
peerId = peerIdFromCID(peerCid)
} catch (err) {
} catch (err: any) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think : any has been added here to mask a TypeScript error. The error is valid - fastify's pino logger doesn't log additional arguments without formatters and the types have been updated to reflect this, causing the error where there wasn't one before.

The correct solution is either to add a formatter:

fastify.log.error('could not parse CID from params - %s', err)

or pass the error as a merging object:

fastify.log.error({ err }, 'could not parse CID from params')

The latter is probably better as depending on the Error implementation we can lose information during stringification.

Docs link: https://github.com/pinojs/pino/blob/main/docs/api.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't return 404s from routing calls

2 participants