Skip to content

Conversation

@aschmahmann
Copy link

Description

Switched the UnixFS resolver code to do resolve only the relevant part of a HAMT rather than potentially enumerating all of it.

Fixes: ipfs/service-worker-gateway#18

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

@aschmahmann aschmahmann requested a review from a team as a code owner February 23, 2024 17:55
Comment on lines 72 to 76
if (result.unixfs?.type === 'hamt-sharded-directory') {
// special case - unixfs v1 hamt shards
dirCid = await findShardCid(result.node, part, blockstore)
} else {
dirCid = findLinkCid(result.node, part)
Copy link
Author

@aschmahmann aschmahmann Feb 23, 2024

Choose a reason for hiding this comment

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

This code is straight-up copy-pasted from https://github.com/ipfs/js-ipfs-unixfs/blob/4749d9a7c1eddd86b8fc42c3fa47f88c7b1b75ae/packages/ipfs-unixfs-exporter/src/resolvers/unixfs-v1/index.ts#L59 which makes me wonder why we can't just use walkPath from ipfs-unixfs-exporter directly and pass a path

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 using ipfs-unixfs-exporter instead of copy-pasting the code is a better idea.

@@ -1,12 +1,14 @@
import { logger } from '@libp2p/logger'
import { exporter } from 'ipfs-unixfs-exporter'
import findShardCid from 'ipfs-unixfs-exporter/dist/src/utils/find-cid-in-shard.js'
Copy link
Author

Choose a reason for hiding this comment

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

This isn't right. We could copy-paste or export the code if needed, but wonder if we could use more of the resolver code from ipfs-unixfs-exporter as-is

Copy link
Member

@achingbrain achingbrain Feb 26, 2024

Choose a reason for hiding this comment

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

FWIW you can't do deep imports of files like this any more, it's prevented by the exports map in the module's package.json which defines what's importable from a module and what isn't.

SgtPooki added a commit that referenced this pull request Feb 23, 2024
@achingbrain
Copy link
Member

I have opened a PR that uses the unixfs-exporter to traverse the DAG here - #455

It should also solve the issue reported in ipfs/service-worker-gateway#18

@SgtPooki
Copy link
Member

Closing in favor of #455

@SgtPooki SgtPooki closed this Feb 27, 2024
@SgtPooki SgtPooki deleted the fix/hamt-overqueried branch February 27, 2024 17:08
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.

Enumerating HAMT directory retrieves too many blocks?

4 participants