fix: getProviders recognizes application/json accept header #155

Nkovaturient · 2025-02-03T20:51:27Z

fix: content routing provider data handling and test cases

Description

Fixed the content routing implementation to ensure proper handling of provider data and JSON content types. All test cases are now passing across different environments (Node.js, Browser, WebWorker).

Key improvements:

Added proper Content-Type: application/json headers
Fixed provider data JSON formatting
Added routing initialization in tests
Fixed multiaddrs format handling in assertions

Test Results:

Node.js: 23 passing, 2 pending
Browser: 25 passing
WebWorker: 23 passing, 2 pending

Note: The 2 pending tests are related to cache TTL functionality, which is expected behavior.

Notes & open questions

None - all test cases are passing as expected across environments.

Change checklist

[✅] I have performed a self-review of my own code
[✅] I have made corresponding changes to the documentation (updated test cases with proper JSON formatting)
[✅] I have added tests that prove my fix is effective

achingbrain · 2025-02-03T21:21:07Z

Thanks for opening this. Please can you run ‘npm run lint -- --fix’ to reformat your changes according to the existing linting rules.

Nkovaturient · 2025-02-04T05:27:44Z

@achingbrain sure

Nkovaturient · 2025-02-04T13:20:57Z

@achingbrain Done✅

achingbrain · 2025-02-04T13:54:53Z

packages/client/.aegir.js has gained semicolons, trailing commas, and had it's ' characters swapped for ".

Several files have many whitespace changes also.

Please revert these changes, they just add noise to the PR and make it harder to review.

SgtPooki

there are multiple edits to code that doesn't seem like anything has changed other than style/lint fixes from a tool other than eslint/aegir and our eslint-config-ipfs rules.

those should definitely be addressed so we can determine what actual code changes were made.

SgtPooki · 2025-02-04T14:00:34Z

packages/client/test/routings.spec.ts

+    const providers = [
+      {
+        Protocol: 'transport-bitswap',
+        Schema: 'bitswap',
+        Metadata: 'gBI=',
+        ID: (await generateKeyPair('Ed25519')).publicKey.toString(),
+        Addrs: ['/ip4/41.41.41.41/tcp/1234']
+      },
+      {
+        Protocol: 'transport-bitswap',
+        Schema: 'peer',
+        Metadata: 'gBI=',
+        ID: (await generateKeyPair('Ed25519')).publicKey.toString(),
+        Addrs: ['/ip4/42.42.42.42/tcp/1234']
+      },
+      {
+        ID: (await generateKeyPair('Ed25519')).publicKey.toString(),
+        Addrs: ['/ip4/43.43.43.43/tcp/1234']
+      }
+    ]


It looks like this change is style only, and unnecessary.

I would highly recommend disabling editorconfig and auto styling that is not using eslint or aegir. If you disable auto-save, you should be able to run npm run lint -- --fix to fix issues until you get your editor to a good place.

FYI: my editor is vscode, and I have https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint installed

alryt thanks for the guidance. Will do it as desired

Nkovaturient · 2025-02-04T16:15:05Z

thanks @SgtPooki @achingbrain for the review and advice---
I have modified so far as advised.

[✅ ] Disabled Auto-save, auto-formatting
[✅ ] vscode/settings.json as per IPFS eslint implementation

kindly look into it and provide guidance.
Thank you for your time.

SgtPooki

partial review because it still seems like a lot of stylistic and unnecessary changes were made

SgtPooki · 2025-02-04T17:26:03Z

packages/client/.aegir.js

@@ -1,96 +1,200 @@
-import EchoServer from 'aegir/echo-server'
-import body from 'body-parser'
+import EchoServer from "aegir/echo-server";


need to remove stylistic changes here and in the rest of the PR.

You might need to git checkout main -- <pathToFile> and then discard everything except the changes that are needed.

okay! lemme try it out👍

this file still has a lot of single quotes changed to double quote, and semicolons added...

i cannot understand, despite disabling autosave, eslint, why extra spaces, semicolons and double quotes are added. can you please guide me how to configure my vs-setup--I have downloaded this vsc extension--https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint--and then ran npm run lint -- --fix, even did git checkout main --, and tried minimal code changes, yet

so, from the root of the project, because this is a monorepo, you actually have to run npm run lint -- -- --fix

SgtPooki · 2025-02-04T17:26:09Z

packages/client/.aegir.js

+        next();
+        lastCalledUrl = req.url;
+      });
+      // echo.polka.post("/add-providers/:cid", (req, res) => {


SgtPooki · 2025-02-04T17:28:36Z

packages/client/test/index.spec.ts

-    client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), { cacheTTL: 0 })
+    client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), {
+      cacheTTL: 0
+    })


unnecessary edit?

yes it is irrelevant, my apology

this one still needs addressed

SgtPooki · 2025-02-04T17:28:48Z

packages/client/test/index.spec.ts

-    await fetch(`${process.env.ECHO_SERVER}/add-providers/${cid.toString()}`, {
-      method: 'POST',
-      body: providers.map(prov => JSON.stringify(prov)).join('\n')
-    })
+    await fetch(
+      `${process.env.ECHO_SERVER}/add-providers/${cid.toString()}`,
+      {
+        method: 'POST',
+        headers: {
+          'Content-Type': 'application/json'
+        },
+        body: JSON.stringify({ Providers: providers })
+      }
+    )


unnecessary formatting change?

No, the test case was returning err when headers wasnt added here!

Sure, but there are still formatting changes here that aren't necessary. e.g. we should still have await fetch(${process.env.ECHO_SERVER}/add-providers/${cid.toString()}, { on the initial line here AFAIK.

Nkovaturient · 2025-02-04T17:52:13Z

packages/client/.aegir.js has gained semicolons, trailing commas, and had it's ' characters swapped for ".

Several files have many whitespace changes also.

Please revert these changes, they just add noise to the PR and make it harder to review.

@achingbrain @SgtPooki
I humbly apologise for my unstructured code changes. It wont happen again. Thank you again for guiding.

Nkovaturient · 2025-02-04T19:30:18Z

@SgtPooki

I ran git checkout --main,
compared and subscribed to optimal code changes
ipfs/helia eslint practices
minimal reformatting and
worked afresh in carrying out the changes in Content-type header.

Kindly review it.

SgtPooki · 2025-02-05T15:16:50Z

packages/client/.aegir.js

@@ -1,96 +1,200 @@
-import EchoServer from 'aegir/echo-server'
-import body from 'body-parser'
+import EchoServer from "aegir/echo-server";


this file still has a lot of single quotes changed to double quote, and semicolons added...

packages/client/src/client.ts

SgtPooki · 2025-02-05T15:20:35Z

packages/client/test/index.spec.ts

+import {
+  createDelegatedRoutingV1HttpApiClient,
+  type DelegatedRoutingV1HttpApiClient
+} from '../src/index.js'


another unnecessary change

Suggested change

import {

createDelegatedRoutingV1HttpApiClient,

type DelegatedRoutingV1HttpApiClient

} from '../src/index.js'

import { createDelegatedRoutingV1HttpApiClient, type DelegatedRoutingV1HttpApiClient } from '../src/index.js'

SgtPooki · 2025-02-05T15:20:52Z

packages/client/test/index.spec.ts

-    client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), { cacheTTL: 0 })
+    client = createDelegatedRoutingV1HttpApiClient(new URL(serverUrl), {
+      cacheTTL: 0
+    })


this one still needs addressed

Nkovaturient · 2025-02-07T09:03:43Z

@SgtPooki
made desired changes,
is it upto the mark?🙂

Nkovaturient · 2025-02-09T19:59:26Z

Kindly guide further?

SgtPooki

I modified the code a bit.. lgtm now.

## [@helia/delegated-routing-v1-http-api-client-v4.2.2](https://github.com/ipfs/helia-delegated-routing-v1-http-api/compare/@helia/delegated-routing-v1-http-api-client-4.2.1...@helia/delegated-routing-v1-http-api-client-4.2.2) (2025-02-11) ### Bug Fixes * getProviders recognizes application/json accept header ([#155](#155)) ([8fd603b](8fd603b))

github-actions · 2025-02-11T19:05:00Z

🎉 This PR is included in version @helia/delegated-routing-v1-http-api-client-v4.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

modified-contentType&-added-testcases

29ccab6

Nkovaturient requested a review from a team as a code owner February 3, 2025 20:51

Nkovaturient mentioned this pull request Feb 3, 2025

getProviders returns empty array if delegated routing server returns Content-Type header that is not "application/json" exactly #154

Closed

fix: explicit Content-Type null check

5d10d98

SgtPooki requested changes Feb 4, 2025

View reviewed changes

SgtPooki linked an issue Feb 4, 2025 that may be closed by this pull request

getProviders returns empty array if delegated routing server returns Content-Type header that is not "application/json" exactly #154

Closed

SgtPooki changed the title ~~Perf & Enhancement: Content Routing~~ fix: getProviders recognizes application/json accept header Feb 4, 2025

fix-eslint-issues

6c6de7f

SgtPooki requested changes Feb 4, 2025

View reviewed changes

Nkovaturient mentioned this pull request Feb 4, 2025

fix: getProviders recognizes application/json accept header #156

Closed

SgtPooki requested changes Feb 5, 2025

View reviewed changes

Nkovaturient added 2 commits February 6, 2025 00:46

fixes-&-improve-code

16df06c

fix: cache ok responses

f9bce42

SgtPooki added 3 commits February 10, 2025 14:24

chore: lint fix

ebb9997

chore: reduce number of changes

b88cd64

chore: revert response cache change

bf2fa2d

SgtPooki approved these changes Feb 10, 2025

View reviewed changes

SgtPooki requested a review from achingbrain February 10, 2025 21:45

chore: remove unused package

e2860db

SgtPooki merged commit 8fd603b into ipfs:main Feb 11, 2025
19 checks passed

github-actions bot added the released label Feb 11, 2025

fix: getProviders recognizes application/json accept header #155

fix: getProviders recognizes application/json accept header #155

Uh oh!

Conversation

Nkovaturient commented Feb 3, 2025

fix: content routing provider data handling and test cases

Description

Notes & open questions

Change checklist

Uh oh!

achingbrain commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nkovaturient commented Feb 4, 2025

Uh oh!

Nkovaturient commented Feb 4, 2025

Uh oh!

achingbrain commented Feb 4, 2025

Uh oh!

SgtPooki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nkovaturient commented Feb 4, 2025

Uh oh!

SgtPooki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nkovaturient commented Feb 4, 2025

Uh oh!

Nkovaturient commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nkovaturient commented Feb 7, 2025

Uh oh!

Nkovaturient commented Feb 9, 2025

Uh oh!

SgtPooki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 11, 2025

Uh oh!

Uh oh!

achingbrain commented Feb 3, 2025 •

edited

Loading