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

Better error message when not initialized #1180

Closed
vmx opened this issue Jan 22, 2018 · 8 comments
Closed

Better error message when not initialized #1180

vmx opened this issue Jan 22, 2018 · 8 comments
Assignees
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/in-progress In progress

Comments

@vmx
Copy link
Member

vmx commented Jan 22, 2018

Type:

Bug

Severity:

Low

Description:

jsipfs block put errors with an unhandled exception when IPFS wasn't previously initialized. Instead it should print a nice error that no repository was initialized and calling jsipfs init first might make sense. Example:

$ jsipfs block put README.md
/home/vmx/src/pl/js-ipfs/node_modules/ipfs-block-service/src/index.js:64
    this._repo.blocks.put(block, callback)
                      ^

TypeError: Cannot read property 'put' of undefined
    at BlockService.put (/home/vmx/src/pl/js-ipfs/node_modules/ipfs-block-service/src/index.js:64:23)
    at waterfall (/home/vmx/src/pl/js-ipfs/src/core/components/block.js:51:43)
    at nextTask (/home/vmx/src/pl/js-ipfs/node_modules/async/waterfall.js:16:14)
    at next (/home/vmx/src/pl/js-ipfs/node_modules/async/waterfall.js:23:9)
    at /home/vmx/src/pl/js-ipfs/node_modules/async/internal/onlyOnce.js:12:16
    at waterfall (/home/vmx/src/pl/js-ipfs/src/core/components/block.js:31:20)
    at nextTask (/home/vmx/src/pl/js-ipfs/node_modules/async/waterfall.js:16:14)
    at exports.default (/home/vmx/src/pl/js-ipfs/node_modules/async/waterfall.js:26:5)
    at Function.put.promisify (/home/vmx/src/pl/js-ipfs/src/core/components/block.js:28:7)
    at Object.put (/home/vmx/src/pl/js-ipfs/node_modules/promisify-es6/index.js:32:27)

Steps to reproduce the error:

Make sure you don't have a repository (i.e. rm -Rf ~/.jsipfs). Then run:

jsipfs block put <some-file>
@daviddias daviddias added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked labels Jan 24, 2018
@paulogr
Copy link
Contributor

paulogr commented Jan 29, 2018

Hi @diasdavid I'm taking a look at this issue and found this

if (err.message.match(/not found/) || // indexeddb

There, you do not fail when a repo doesn't exists but just flag it.

A few lines after you check if hasRepo but check too if the IPFS has the option init: true

if (doInit && !hasRepo) {

But the IPFS class is always instaciated with init: false

init: false,

We have two options there

  1. change to init: true, so the check will evaluate and the repo will be initialized if it doesn't exists.
  2. do not just flag that repo was not initialized but throw the error to get caught and displayed like it happens on go ipfs implementations.

In go-ipfs if I try to run the same command I got this message:

Error: no IPFS repo found in /Users/paulogr/.ipfs.
please run: 'ipfs init'

Could you please instruct me what is the best way to accomplish that?

Will be glad to make a PR to fix it :)
Thank you!

@fsdiogo fsdiogo self-assigned this Mar 20, 2018
@fsdiogo fsdiogo added status/in-progress In progress and removed status/ready Ready to be worked labels Mar 20, 2018
@fsdiogo
Copy link
Contributor

fsdiogo commented Mar 21, 2018

I'm taking a look at this issue too, but I'm not sure if I'm taking the right path.

js-ipfs/src/core/boot.js

Lines 39 to 49 in c1e8db1

// If the error is that no repo exists,
// which happens when the version file is not found
// we just want to signal that no repo exist, not
// fail the whole process.
// TODO: improve datastore and ipfs-repo implemenations so this error is a bit more unified
if (err.message.match(/not found/) || // indexeddb
err.message.match(/ENOENT/) || // fs
err.message.match(/No value/) // memory
) {
return cb(null, false)
}

We could replace the line 48 with the following code to get the same behaviour of go-ipfs:

const repoPath = process.env.IPFS_PATH || os.homedir() + '/.jsipfs'
process.stdout.write('Error: no initialized ipfs repo found in ' + repoPath)
process.stdout.write('\nplease run: jsipfs init')
process.exit(1)

But I don't know if this should be in core/boot or a CLI only fix.

Any thoughts, @diasdavid @JonKrone?

@victorb
Copy link
Member

victorb commented Mar 21, 2018

@fsdiogo we should never do process.exit (edit: outside of the cli) as it might exit the process for the user when they want to handle the error and do something else.

The right implementation would be to know which commands needs to have a repository initialized before executing (not all commands needs a repository to work). For those commands, it should check the repository before running and if not initialized, return the callback with an error. This logic should hopefully be contained in one place and we can "tag" commands that needs a initialized repository to work.

In the CLI, if we get this error, we simply display it and then we can do process.exit as the CLI would/should not be reused by other projects.

@fsdiogo
Copy link
Contributor

fsdiogo commented Mar 21, 2018

@victorbjelkholm makes sense. That's why it didn't feel right doing that there.

But then it should be implemented both in the core and the CLI right? Abstracted and shared by both.

@daviddias
Copy link
Member

@fsdiogo the core is the module that either goes into the browser or it is used inside a daemon. It should avoid doing things like writing to stdout directly or exiting the process as @victorbjelkholm pointed out. Instead, it should return proper errors.

For the issue @vmx reported, the check can happen all in CLI, from within this func https://github.com/ipfs/js-ipfs/blob/master/src/cli/utils.js#L41

@victorb
Copy link
Member

victorb commented Mar 21, 2018

the check can happen all in CLI

Hm, then when you don't use the CLI (in browser for example), you won't get the error even if it's appropriate. I would say the error needs to come from core as a "repo not initialized" error, then printed in the CLI. The check should not just be in the CLI though, as if you don't use the CLI, you get the cryptic Cannot read property 'put' of undefined error that is mentioned here.

@fsdiogo fsdiogo removed their assignment Mar 29, 2018
@fsdiogo fsdiogo added status/ready Ready to be worked and removed status/in-progress In progress labels Mar 29, 2018
@hugomrdias hugomrdias self-assigned this Apr 9, 2018
@hugomrdias hugomrdias added status/in-progress In progress and removed status/ready Ready to be worked labels Apr 9, 2018
@uluhonolulu
Copy link

I'm getting this error from time to time using the package (v0.33.1) in a NodeJS project.

  put (block, callback) {
    if (this.hasExchange()) {
      this._bitswap.put(block, callback)
    } else {
      this._repo.blocks.put(block, callback)
    }
  }

The happy path in my case is choosing this._bitswap.put(block, callback), but sometimes this._repo.blocks.put(block, callback) is executed, which throws the error.

I'm on Windows 7.

@achingbrain
Copy link
Member

Closing as this issue is very stale and is no longer a problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/in-progress In progress
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants