Skip to content
This repository was archived by the owner on Mar 10, 2020. It is now read-only.

fix: Support api endpoint with no port defined #744

Closed
wants to merge 1 commit into from
Closed

fix: Support api endpoint with no port defined #744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"detect-node": "^2.0.3",
"flatmap": "0.0.3",
"glob": "^7.1.2",
"ip-regex": "^3.0.0",
"ipfs-block": "~0.7.1",
"ipfs-unixfs": "~0.1.14",
"ipld-dag-cbor": "~0.12.0",
Expand Down
15 changes: 14 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'

const ipRegex = require('ip-regex')
const multiaddr = require('multiaddr')
const loadCommands = require('./utils/load-commands')
const getConfig = require('./utils/default-config')
Expand All @@ -15,7 +16,19 @@ function IpfsAPI (hostOrMultiaddr, port, opts) {
} catch (e) {
if (typeof hostOrMultiaddr === 'string') {
config.host = hostOrMultiaddr
config.port = port && typeof port !== 'object' ? port : config.port
const addrParts = hostOrMultiaddr.split(':')
if (addrParts.length === 1 && ipRegex.v4({ exact: true }).test(addrParts[0]) &&
(!port || typeof port === 'object')) {
// IPv4 Host with no port specified
config.port = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel as though there might be a bunch of cases here that aren't being taken into account. For example:

  1. I pass a host as an IP with no port and want the default port
  2. I pass a host as a domain name with no port and want the default port
  3. I pass a host as a domain name with a port

I'm not sure this is quite the right solution for the problem. If you're creating a new IpfsApi and you don't pass a port you should expect to get the default port. In #581 the request is to pass a path as well as a host. This PR would allow the user to pass 1.2.3.4/ipfs for their host, but this isn't a valid "host" - host being a hostname and port and might also break the default port.

I was going to suggest adding a path or pathname property to the config to allow people to customise the path, but I just found this (api-path):

https://github.com/ipfs/js-ipfs-api/blob/faa51b4c26abc047fd94b78535e77ca2dc95630f/src/utils/send-request.js#L159

Can this be used with #581 and does this mitigate the need for this PR? If so, it needs to be documented and we can close out #581 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your analyze @alanshaw ! I agree with you about using api-path for this 🙂

Copy link
Contributor

@alanshaw alanshaw May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Rad! Would you check that it works (perhaps add a test!?) and send a PR for updating the docs?

Shall we close this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to test this later today! Then, I close this PR and create the new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanshaw , I had tested this and. If I instantiate the ipfsAPI this way:

const api = ipfsAPI({
      'api-path': 'http://127.0.0.1/api/v0/',
      'host': '',
      'port': ''
    })

The host and port default are used as well. A request example is:

http://localhost:3000http://127.0.0.1/api/v0/add?wrapWithDirectory=true&wrap-with-directory=true&stream-channels=true

Therefore, we need another property for ignoring the default config. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but for #581 I was hoping you'd be able to do this:

const api = ipfsAPI({
  'api-path': '/ipfs/api/v0/',
  host: '1.1.1.1',
  port: 80
})

To get:

http://1.1.1.1:80/ipfs/api/v0/add?wrapWithDirectory=true&wrap-with-directory=true&stream-channels=true

...to solve the issue.

Is this the case? If so then there's no work to do here other than to send a PR to document api-path and respond to #581.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it worked using

const api = ipfsAPI({
      'api-path': '/ipfs/api/v0/',
      'host': '127.0.0.1',
      'port': '80'
    })

I will add a test to the constructor and add the proper docs.

Thanks @alanshaw

} else if (addrParts.length === 2 && ipRegex.v4({ exact: true }).test(addrParts[0]) &&
(!port || typeof port === 'object')) {
// IPv4 Host with port specified
config.host = addrParts[0]
config.port = addrParts[1]
} else {
config.port = port
}
}
}

Expand Down
6 changes: 6 additions & 0 deletions test/constructor.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ describe('ipfs-api constructor tests', () => {
clientWorks(ipfsAPI(apiAddr, { protocol: 'http' }), done)
})

it('host:port', (done) => {
const splitted = apiAddr.split('/')

clientWorks(ipfsAPI(`${splitted[2]}:${splitted[4]}`), done)
})

it('host, port', (done) => {
const splitted = apiAddr.split('/')

Expand Down