-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: recursive dnslink lookups #1935
Changes from 2 commits
74c3a49
5e02108
0518da3
aed2f53
d5fa74e
fb114f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,29 @@ | |||||||||||||||||
|
|
||||||||||||||||||
| const dns = require('dns') | ||||||||||||||||||
| const _ = require('lodash') | ||||||||||||||||||
| const isIPFS = require('is-ipfs') | ||||||||||||||||||
| const errcode = require('err-code') | ||||||||||||||||||
|
|
||||||||||||||||||
| const maxRecursiveDepth = 32 | ||||||||||||||||||
niinpatel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
|
|
||||||||||||||||||
| module.exports = (domain, opts, callback) => { | ||||||||||||||||||
| resolveDnslink(domain) | ||||||||||||||||||
| // recursive is true by default, it's set to false only if explicitly passed as argument in opts | ||||||||||||||||||
| const recursive = opts.recursive === undefined || opts.recursive.toString() === 'true' | ||||||||||||||||||
|
||||||||||||||||||
| const recursive = opts.recursive === undefined || opts.recursive.toString() === 'true' | |
| const recursive = opts.recursive == null ? true : Boolean(opts.recursive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean("false") actually returns true not false. Arguments passed via query parameters are of string type and we need to parse those too. Once easy solution I just now came up with is this:
const recursive = opts.recursive.toString() !== 'false'
If recursive is false or "false" then only we parse it as false. true when passed anything else, including null and undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: That wouldn't work either because error will be thrown if opts.recursive is undefined.
The original one is probably the best solution after all, with slight modifications:
js-ipfs/src/core/runtime/dns-nodejs.js
Line 12 in d6b2ca3
| const recursive = opts.recursive == null || opts.recursive.toString() !== 'false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguments passed via query parameters are of string type and we need to parse those too
It should be passed as a boolean - this is the documented type for the value. If a string is being passed then we need to fix our HTTP endpoint to properly parse query strings to the expected types for the calls to core.
We can't guarantee the passed value is a boolean but we can ensure that the value is a boolean for the rest of our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Right now, I don't think query parameters are being parsed and "type-casted" before passing to the core API. For this PR, I've just now add it here:
js-ipfs/src/http/api/resources/dns.js
Lines 14 to 18 in fb114f1
| // query parameters are passed as strings and need to be parsed to expected type | |
| let recursive = request.query.recursive || request.query.r | |
| recursive = !(recursive && recursive === 'false') | |
| const path = await request.server.app.ipfs.dns(domain, { recursive, format }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| /* eslint-env mocha */ | ||
| 'use strict' | ||
|
|
||
| const chai = require('chai') | ||
| const dirtyChai = require('dirty-chai') | ||
| const expect = chai.expect | ||
| chai.use(dirtyChai) | ||
|
|
||
| const IPFSFactory = require('ipfsd-ctl') | ||
| const IPFS = require('../../src/core') | ||
|
|
||
| describe('.dns', () => { | ||
| let ipfsd, ipfs | ||
|
|
||
| before(function (done) { | ||
| this.timeout(20 * 1000) | ||
|
|
||
| const factory = IPFSFactory.create({ type: 'proc' }) | ||
|
|
||
| factory.spawn({ | ||
| exec: IPFS, | ||
| initOptions: { bits: 512 }, | ||
| config: { Bootstrap: [] } | ||
| }, (err, _ipfsd) => { | ||
| expect(err).to.not.exist() | ||
| ipfsd = _ipfsd | ||
| ipfs = _ipfsd.api | ||
| done() | ||
| }) | ||
| }) | ||
|
|
||
| after((done) => { | ||
| if (ipfsd) { | ||
| ipfsd.stop(done) | ||
| } else { | ||
| done() | ||
| } | ||
| }) | ||
|
|
||
| // skipping because there is an error in https://ipfs.io/api/v0/dns?arg=ipfs.io | ||
| // unskip once this is resolved: https://github.com/ipfs/go-ipfs/issues/6086 | ||
| it.skip('should non-recursively resolve ipfs.io', () => { | ||
| return ipfs.dns('ipfs.io', { recursive: false }).then(res => { | ||
| // matches pattern /ipns/<ipnsaddress> | ||
| expect(res).to.match(/\/ipns\/.+$/) | ||
| }) | ||
| }) | ||
|
|
||
| it('should recursively resolve ipfs.io', () => { | ||
| return ipfs.dns('ipfs.io', { recursive: true }).then(res => { | ||
| // matches pattern /ipfs/<hash> | ||
| expect(res).to.match(/\/ipfs\/.+$/) | ||
| }) | ||
| }) | ||
|
|
||
| it('should resolve subdomain docs.ipfs.io', () => { | ||
| return ipfs.dns('docs.ipfs.io').then(res => { | ||
| // matches pattern /ipfs/<hash> | ||
| expect(res).to.match(/\/ipfs\/.+$/) | ||
| }) | ||
| }) | ||
niinpatel marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.