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

fix(core/components/dag): make options in put API optional #1415

Merged
merged 2 commits into from
Jul 3, 2018

Conversation

0x-r4bbit
Copy link
Contributor

@0x-r4bbit 0x-r4bbit commented Jun 29, 2018

The dag.put interface
takes an options object which is required, but should be optional with
decent defaults.

See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316

This commit implements this behaviour.

Before:

ipfs.dag.put(obj, options, (err, cid) => {...});

^ Prior to this commit, without passing options, this call resulted in an error.

After:

ipfs.dag.put(obj, (err, cid) => {...});

^ This is now perfectly fine.

Fixes #1395

License: MIT
Signed-off-by: Pascal Precht [email protected]


const optionDefaults = {
format: 'dag-pb',
hashAlg: 'sha2-256'
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 you might wanna review those.

Copy link
Member

Choose a reason for hiding this comment

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

These should be the same as go-ipfs (https://ipfs.io/docs/commands/#ipfs-dag-put) i.e. dag-cbor and sha2-256

Note also that cid can be passed here instead of format & hashAlg that you need to take into account. See https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput

@0x-r4bbit
Copy link
Contributor Author

This is my first take on fixing #1395

I also wrote simple test for it in interface-ipfs-core, however, I wasn't able to actually run it within my local js-ipfs installation yet.

That's because as soon as interface-ipfs-core is linked as a dependency, test commands fail due to createCommon() not being a function.

Here's the error message I get:

~/projects/ipfs/js-ipfs fix/dag-put-api*                                                                  
❯ npm run test:node:core          
                                                                                                                            
> [email protected] test:node:core /Users/pascalprecht/projects/ipfs/js-ipfs                                                
> aegir test -t node -f test/core/**/*.js                                                                                    
                                                              
Test Node.js                                                                
/Users/pascalprecht/projects/ipfs/interface-ipfs-core/js/src/bitswap/stat.js:11
  const common = createCommon()                             
                 ^                                                 
                                                                      
TypeError: createCommon is not a function         
    at Object.module.exports [as stat] (/Users/pascalprecht/projects/ipfs/interface-ipfs-core/js/src/bitswap/stat.js:11:18)
    at Object.keys.forEach.t (/Users/pascalprecht/projects/ipfs/interface-ipfs-core/js/src/utils/suite.js:25:15)                                                                       at Array.forEach (<anonymous>)                                                                                                                                                     at Object.suite [as bitswap] (/Users/pascalprecht/projects/ipfs/interface-ipfs-core/js/src/utils/suite.js:5:24)                                                               
    at Object.<anonymous> (/Users/pascalprecht/projects/ipfs/js-ipfs/test/core/interface/bitswap.js:35:6)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)                                                                                                           at Module.load (internal/modules/cjs/loader.js:612:32)                                                                                                                             at tryModuleLoad (internal/modules/cjs/loader.js:551:12)                                                                                                                           at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at /Users/pascalprecht/projects/ipfs/js-ipfs/node_modules/aegir/node_modules/mocha/lib/mocha.js:231:27                   
    at Array.forEach (<anonymous>)                               
    at Mocha.loadFiles (/Users/pascalprecht/projects/ipfs/js-ipfs/node_modules/aegir/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/Users/pascalprecht/projects/ipfs/js-ipfs/node_modules/aegir/node_modules/mocha/lib/mocha.js:536:10)
    at Object.<anonymous> (/Users/pascalprecht/projects/ipfs/js-ipfs/node_modules/aegir/node_modules/mocha/bin/_mocha:573:18)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)                                      
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:240:19)                            
    at bootstrapNodeJSCore (internal/bootstrap/node.js:564:3)

I took a closer look and createCommon should've actually been passed down from test/core/interface/bitswap.js:

https://github.com/ipfs/js-ipfs/blob/master/test/core/interface/bitswap.js#L35

@alanshaw maybe you have an idea what could cause this. I should add that this error does not occur when not linking interface-ipfs-core.

@0x-r4bbit
Copy link
Contributor Author

Also, I wasn't sure about the commit message scope, let me know if that needs to be updated.

@travisperson
Copy link
Member

@PascalPrecht PR #1389 updates js-ipfs to work with the newer version of interface-ipfs-core, which now requires the common to be a function stead of just an object.

To run new interface-ipfs-core (where your test is built), you will need to rebase your changes in this branch onto #1389.

PR #1389 should be merging soon, so we'll just need to wait for that to go in before merging this PR.

@0x-r4bbit
Copy link
Contributor Author

@travisperson thanks for clarifying!

I thought the #1389 was only crucial to enable support for the API to skip testsuites as @alanshaw mentioned in #1395.

Also I've just noticed that js-ipfs sits on interface-ipfs-core version 0.69.0 which explains the incompatibility between those to packages on latest master, as interface-ipfs-core has already evolved since then.

I'll rebase this PR on top of it then.

Thanks again for clarifying!

@0x-r4bbit
Copy link
Contributor Author

@travisperson @alanshaw Updated the PR accordingly.

I've also created another PR on interface-ipfs-core to introduce a dedicated test: ipfs-inactive/interface-js-ipfs-core#316

We might wanna merge that one, before this one can go in.

@0x-r4bbit
Copy link
Contributor Author

Looks like CI is failing due to use of object spread operators. Will update the PR once again to make CI happy.

@0x-r4bbit
Copy link
Contributor Author

Went with Object.assign() instead of object spread operators now. Hope that will satisfy CI.

0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jun 30, 2018
@0x-r4bbit
Copy link
Contributor Author

0x-r4bbit commented Jun 30, 2018

CI is still failing, but those failing tests are unrelated to my changes.

I've also noticed that this needs to be implemented in js-ipfs-api as well (https://github.com/ipfs/js-ipfs-api/blob/master/src/dag/put.js#L18). Will wait once we've decided on decent defaults and send a PR over there as well.


const optionDefaults = {
format: 'dag-pb',
hashAlg: 'sha2-256'
Copy link
Member

Choose a reason for hiding this comment

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

These should be the same as go-ipfs (https://ipfs.io/docs/commands/#ipfs-dag-put) i.e. dag-cbor and sha2-256

Note also that cid can be passed here instead of format & hashAlg that you need to take into account. See https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput

@0x-r4bbit
Copy link
Contributor Author

0x-r4bbit commented Jul 2, 2018

Note also that cid can be passed here instead of format & hashAlg that you need to take into account.

@alanshaw good point. The spec doesn't say anything about it, but I guess it'd make sense to throw an error in case someone uses the API wrong.

Or would you prefer if dag.put would silently chose an option with higher priority? For example, if cid is given, it simply ignores format and hashAlg.

@alanshaw
Copy link
Member

alanshaw commented Jul 2, 2018

The spec doesn't say anything about it

I'm looking here:

screen shot 2018-07-02 at 11 50 52

If the user passes a cid along with a format or hashAlg then I think it should callback with an error. It is likely developer error and we should inform them about it.

I'd also enforce format and hashAlg pair i.e. you shouldn't be able to pass one but not the other.

Does that sound sensible?

@0x-r4bbit
Copy link
Contributor Author

I'm looking here

Correct! I was referring to whether the API should throw an error or not :)

Does that sound sensible?

Alright, gonna update the PR once again!

} else if (options.cid && options.format && options.hashAlg) {
throw new Error('Can\'t put dag node. Please provide either `cid` OR `format` and `hashAlg` options.');
} else if ((options.format && !options.hashAlg) || (!options.format && options.hashAlg)) {
throw new Error('Can\'t put dag node. Please provide `format` AND `hashAlg` options.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a moment I was confused whether throwing synchronously inside promisify() makes sense, but I don't seem to have a handle to "reject" it either.

Looking at its testsuite, it seems to be the way to go: https://github.com/manuel-di-iorio/promisify-es6/blob/master/test.js#L79

Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace throw new ... with return callback(new ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richardschneider thanks for the hint! I assume that will automatically reject the promise?

I've updated the PR accordingly either way :)

0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 2, 2018
Copy link
Member

@alanshaw alanshaw 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 awesome 🚀 @PascalPrecht - thanks so much. I have just one small feedback point and we should be good to merge.

@@ -9,6 +9,21 @@ const flattenDeep = require('lodash.flattendeep')
module.exports = function dag (self) {
return {
put: promisify((dagNode, options, callback) => {
if (typeof options === 'function') {
callback = options
} else if (options.cid && options.format && options.hashAlg) {
Copy link
Member

Choose a reason for hiding this comment

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

If we change this to if (options.cid && (options.format || options.hashAlg)) we'll catch cid+format, cid+hashAlg AND cid+format+hashAlg. At the moment if I provide one of hashAlg or format as well as cid it'll fall through to the next check which is for a different restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea, right. Much better. Let me update really quick.

} else if (options.cid && options.format && options.hashAlg) {
throw new Error('Can\'t put dag node. Please provide either `cid` OR `format` and `hashAlg` options.')
} else if ((options.format && !options.hashAlg) || (!options.format && options.hashAlg)) {
throw new Error('Can\'t put dag node. Please provide `format` AND `hashAlg` options.')
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, yes I completely missed that - you must return callback(new Error(...)) for these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Sorry I didn't come up with callback(new Error()) myself. It wasn't really clear, looking at the (minimal) docs and tests of promisify-es6.

This should be aligned now.

@0x-r4bbit
Copy link
Contributor Author

Thanks @alanshaw I'm happy I managed to contribute to the IPFS project.

Hope there's gonna be more opportunities to help out! :)

The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface
takes an options object which is required, but should be optional with
decent defaults.

See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316

This commit implements this behaviour.

**Before**:

```js
ipfs.dag.put(obj, options, (err, cid) => {...});
```

^ Prior to this commit, without passing `options`, this call resulted in an error.

**After**:

```js
ipfs.dag.put(obj, (err, cid) => {...});
```

^ This is now perfectly fine.

Fixes ipfs#1395

License: MIT
Signed-off-by: Pascal Precht <[email protected]>
@0x-r4bbit
Copy link
Contributor Author

FYI: saw that latest master landed #1389, rebased this one on top of it therefore again.

@alanshaw
Copy link
Member

alanshaw commented Jul 2, 2018

I've also noticed that this needs to be implemented in js-ipfs-api as well (https://github.com/ipfs/js-ipfs-api/blob/master/src/dag/put.js#L18). Will wait once we've decided on decent defaults and send a PR over there as well.

BTW, that would be incredible ✨

@0x-r4bbit
Copy link
Contributor Author

Gotcha. WIll take care of that.

alanshaw pushed a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Jul 3, 2018
* test(dag): introduce test that ensure `dag.put` can be called without options

As discussed in ipfs/js-ipfs#1415 (comment)

* chore(SPEC/DAG): update DAG spec to cover optional `options` argument

License: MIT
Signed-off-by: Pascal Precht <[email protected]>

* test: add test to ensure defaults are set

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost assigned alanshaw Jul 3, 2018
@ghost ghost added the status/in-progress In progress label Jul 3, 2018
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs-api that referenced this pull request Jul 3, 2018
This is to align with API changes made in

ipfs-inactive/interface-js-ipfs-core@011c417

and

ipfs/js-ipfs#1415

License: MIT
Signed-off-by: Pascal Precht <[email protected]>
@0x-r4bbit
Copy link
Contributor Author

BTW, that would be incredible ✨

@alanshaw please find a WIP PR here ipfs-inactive/js-ipfs-http-client#801

@alanshaw alanshaw merged commit d299ed7 into ipfs:master Jul 3, 2018
@ghost ghost removed the status/in-progress In progress label Jul 3, 2018
@0x-r4bbit 0x-r4bbit deleted the fix/dag-put-api branch July 3, 2018 17:32
daviddias pushed a commit to ipfs-inactive/js-ipfs-http-client that referenced this pull request Jul 4, 2018
* fix(dag): ensure `dag.put()` allows for optional options

This is to align with API changes made in

ipfs-inactive/interface-js-ipfs-core@011c417

and

ipfs/js-ipfs#1415

License: MIT
Signed-off-by: Pascal Precht <[email protected]>

* fix(dag): fixes to allow options to be optional

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update ipfsd-ctl dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: increase timeout for addFromURL with wrap-with-directory

Sadly we can't guarantee ipfs.io will respond within 5s

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
daviddias pushed a commit to ipfs-inactive/js-ipfs-http-client that referenced this pull request Jul 4, 2018
* feat: uses modular interface tests

    Reduces code repetition, allows test skipping and running only some tests.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: skip unimplemented files tests and add ls* tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: adds skips for #339

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: adds skips for key and miscellaneous

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: add types and util tests (skipped as currently failing)

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: add pin tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix(pubsub): adds skips for tests on windows

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix(config): adds skip for config.replace

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: re-adds bitswap tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: move detect-node back to dependencies

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add skip reasons

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add reason for pubsub in browser skips

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add bitswap skips for offline errors

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: remove skip for test that was removed

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core version

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update deps and test on CI (#802)

* chore: update interface-ipfs-core

* fix(dag): `dag.put()` allows for optional options (#801)

* fix(dag): ensure `dag.put()` allows for optional options

This is to align with API changes made in

ipfs-inactive/interface-js-ipfs-core@011c417

and

ipfs/js-ipfs#1415

License: MIT
Signed-off-by: Pascal Precht <[email protected]>

* fix(dag): fixes to allow options to be optional

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update ipfsd-ctl dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: increase timeout for addFromURL with wrap-with-directory

Sadly we can't guarantee ipfs.io will respond within 5s

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: skip bitswap offline tests on all platforms

The offline tests create and stop a node. ipfs/kubo#4078 seems to not only be restricted to windows.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
danieldaf pushed a commit to danieldaf/js-ipfs-api that referenced this pull request Jul 21, 2018
* feat: uses modular interface tests

    Reduces code repetition, allows test skipping and running only some tests.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: skip unimplemented files tests and add ls* tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: adds skips for ipfs-inactive#339

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: adds skips for key and miscellaneous

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: add types and util tests (skipped as currently failing)

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* feat: add pin tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix(pubsub): adds skips for tests on windows

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix(config): adds skip for config.replace

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: re-adds bitswap tests

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: move detect-node back to dependencies

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add skip reasons

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add reason for pubsub in browser skips

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: add bitswap skips for offline errors

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: remove skip for test that was removed

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core version

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update deps and test on CI (ipfs-inactive#802)

* chore: update interface-ipfs-core

* fix(dag): `dag.put()` allows for optional options (ipfs-inactive#801)

* fix(dag): ensure `dag.put()` allows for optional options

This is to align with API changes made in

ipfs-inactive/interface-js-ipfs-core@011c417

and

ipfs/js-ipfs#1415

License: MIT
Signed-off-by: Pascal Precht <[email protected]>

* fix(dag): fixes to allow options to be optional

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update interface-ipfs-core dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* chore: update ipfsd-ctl dependency

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: increase timeout for addFromURL with wrap-with-directory

Sadly we can't guarantee ipfs.io will respond within 5s

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

* fix: skip bitswap offline tests on all platforms

The offline tests create and stop a node. ipfs/kubo#4078 seems to not only be restricted to windows.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
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.

4 participants