Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: make "ipfs resolve" cli command recursive by default #3707

Merged
merged 11 commits into from
Jul 19, 2021
Merged

fix: make "ipfs resolve" cli command recursive by default #3707

merged 11 commits into from
Jul 19, 2021

Conversation

yusefnapora
Copy link
Contributor

@yusefnapora yusefnapora commented Jun 7, 2021

This changes the default behavior of the jsipfs resolve cli command to be recursive by default. Closes #3692.

# in packages/ipfs
node src/cli.js resolve /ipns/ipfs.io
/ipfs/bafybeiagozluzfopjadeigrjlsmktseozde2xc5prvighob7452imnk76a

BREAKING CHANGE: resolve is now recursive by default

@yusefnapora
Copy link
Contributor Author

cc @lidel / @olizilla since I haven't got my permissions sorted yet and can't request a review directly 😄

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks! CI is green, but it also means that we have no tests for this 😅

Perhaps you could add DNSLink test to safeguard of the default behavior in ipfs-cli/test/resolve.js?

@yusefnapora
Copy link
Contributor Author

I updated the tests a bit in ipfs-cli/test/resolve.js in the last commit, but it doesn't really cover the DNSLink issue. These tests just check that ipfs.resolve gets called with the right options for a given cli invocation, so there's no real lookup happening that we can validate.

@lidel Do you know if there's an end-to-end test for the resolve method elsewhere?

@achingbrain
Copy link
Member

@yusefnapora the intergration-style tests that would actually do DNS lookups are in the interface-ipfs-core tests - please add new tests for this there.

@yusefnapora
Copy link
Contributor Author

yusefnapora commented Jun 11, 2021

@achingbrain thanks for the pointer! There was already a test in there for the recursive case, but since recursive is the default, there was no test that covered the non-recursive case.

I added a test for recursive==false, which flushed out an issue. resolve was rewriting the "scheme" portion of the resolved name to /ipfs, even if it actually starts with e.g. /ipns.

So if e.g. you have a recursive IPNS record like:

/ipns/foo -> /ipns/bar -> /ipfs/bafyxzy...

calling resolve('/ipns/foo', { recursive: false }) was returning /ipfs/bar instead of /ipns/bar.

Yay for tests :)

@yusefnapora yusefnapora requested a review from lidel June 11, 2021 19:07
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Lgm, tests seem to fail for unrelated reasons, @achingbrain mind confirming and doing final review in spare time?

@BigLep BigLep added this to In Review in Maintenance Priorities - JS via automation Jun 14, 2021
@BigLep BigLep requested a review from achingbrain June 14, 2021 20:37
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

This is great, but the newly added test is failing in CI

packages/interface-ipfs-core/src/miscellaneous/resolve.js Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

Still failing:

ipfs:   1) interface-ipfs-core tests
ipfs:        .resolve
ipfs:          should resolve IPNS link non-recursively if recursive==false:
ipfs:      Error: record requested for QmYQ4diQzAKmE7PFGY7fbREgiuu4kCcqo88oR7TGrMgv53 was not found in the network
ipfs:       at IpnsResolver._resolveName (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/ipns/resolver.js:102:23)
ipfs:       at async IpnsResolver.resolver (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/ipns/resolver.js:74:17)
ipfs:       at async IpnsResolver.resolve (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/ipns/resolver.js:52:17)
ipfs:       at async IPNS.resolve (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/ipns/index.js:101:22)
ipfs:       at async resolve (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/components/name/resolve.js:84:19)
ipfs:       at async resolve (/home/travis/build/ipfs/js-ipfs/packages/ipfs-core/src/components/resolve.js:27:24)
ipfs:       at async Context.<anonymous> (/home/travis/build/ipfs/js-ipfs/packages/interface-ipfs-core/src/miscellaneous/resolve.js:112:21)

@lidel
Copy link
Member

lidel commented Jun 25, 2021

@yusefnapora will you have time to push this one over the finish line?

@lidel lidel removed this from In Review in Maintenance Priorities - JS Jun 25, 2021
@lidel lidel marked this pull request as draft July 2, 2021 14:22
@yusefnapora
Copy link
Contributor Author

Sorry for letting this one dangle folks. I'll pick it back up this week

@yusefnapora
Copy link
Contributor Author

The last few commits were me flailing to figure out a type check failure in ipfs-message-port-client that I wasn't able to reproduce locally. I finally got it to show up by installing a new version of node - I was on 14.17.0, while CI was running 14.17.1. I updated to the latest v14 release (14.17.3) and started seeing the error locally also.

The problem is that on node, setTimeout returns a NodeJS.Timeout instead of number, so typescript complains when trying to assign to a value of type number|null.

SO to the rescue with a trick to define the type of timerID as whatever the return type of setTimeout actually is.

@yusefnapora
Copy link
Contributor Author

BTW, I think the original test failure was happening because I used the same key ID (key-name) for both of the IPNS tests. I was running each test individually locally since running the whole suite takes a while. Once I ran them all I saw an error about key-name already being registered, so I changed the second test to use a different name. CI's still running, so we'll see :)

@yusefnapora yusefnapora marked this pull request as ready for review July 13, 2021 21:42
@yusefnapora
Copy link
Contributor Author

There's one more test failure, but this one is a module resolution failure in example code:

example-browser-browserify: > browserify src/index.js > public/bundle.js
example-browser-browserify: Error: Can't walk dependency graph: Cannot find module 'multiformats/hashes/sha2' from '/home/travis/build/ipfs/js-ipfs/node_modules/libp2p-crypto/src/keys/rsa-class.js'
example-browser-browserify:     required by /home/travis/build/ipfs/js-ipfs/node_modules/libp2p-crypto/src/keys/rsa-class.js
example-browser-browserify:     at /home/travis/build/ipfs/js-ipfs/node_modules/resolve/lib/async.js:137:35
example-browser-browserify:     at processDirs (/home/travis/build/ipfs/js-ipfs/node_modules/resolve/lib/async.js:290:39)
example-browser-browserify:     at isdir (/home/travis/build/ipfs/js-ipfs/node_modules/resolve/lib/async.js:297:32)
example-browser-browserify:     at /home/travis/build/ipfs/js-ipfs/node_modules/resolve/lib/async.js:25:69
example-browser-browserify:     at FSReqCallback.oncomplete (fs.js:192:21)

That sounds like something that fits into the "maintenance week" remit, so I'll check it out tomorrow :) That should probably go into its own PR though.

@yusefnapora
Copy link
Contributor Author

@achingbrain it looks like the last failing test should be fixed by the work you did on #3742 😄

@achingbrain
Copy link
Member

@yusefnapora awesome! I've restarted that bit of the build, it should pull down the updated modules and be good then.

@achingbrain
Copy link
Member

I'm going to land this after #3556

@achingbrain achingbrain changed the title make "ipfs resolve" cli command recursive by default fix: make "ipfs resolve" cli command recursive by default Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ipfs resolve fails when dnslink points to another dnslink
3 participants