feat: Add option to specify chunking algorithm when adding files #1469

dordille · 2018-07-26T16:43:59Z

This allows the chunking algorithm, and options to be specified when using the adding files.

Specifying chunker and options are identical to go-ipfs and support the following formats:

default
size-{size}
rabin
rabin-{avg}
rabin-{min}-{avg}-{max}

Example usage via command line as follows

$ ./src/cli/bin.js add --chunker rabin-48-96-192 LICENSE
added QmZH1VRMjDD48A7uqzaEU6qdfk4ddNVKZLnBMN1Lc1iEic LICENSE
$ ./src/cli/bin.js object links QmZH1VRMjDD48A7uqzaEU6qdfk4ddNVKZLnBMN1Lc1iEic
QmamX3HDpMkE6NNun2pvz1UmBubA1nsuEzMEnBqUSVXy5E 180
QmdH3L1zu7hWu7nrpoCe33zfn3hDm6baYpTSENbRitpcFu 140
Qmaw7roFAKSj3V3QxmFUfpqU3dqAEMPMqHynqXxJmhhGhB 64
Qmewm77gB4V7cgnZLsuikXgrG8Q38M1TX8qPRBcKrw4yJ4 90
QmRd6EoLAyp6TXZmSRTByaAgEn5632J8u9NwFSiHfXurLx 95
QmeLhozoTP1z3RiLRBRUuLJ7xpmDh5oi1ShN91B4UPjRwA 172
QmdxhoXpK74gGbm2G8bAoCiuqDfbq44rN6RZKefCAFEhoZ 99
QmZNEhqU6uVjMc7p7xfqfwQK5YGxe1bdUNdG6Hq81149iw 139
Qmd1yFj2ZuNd28r5MaQaWkr3vcUwPF61i8oBdfRjaUC4V5 64
QmeJhcwipWbjXA5DdXVGe6Sh8kwBzADNnw3Zd3HKcPtPkm 82
Qme1G8Mk1PLThzCr11NzChQVy1FJoHCQkjK7rB2iTWKkaJ 58

Fixes #1283

vmx · 2018-08-06T14:28:44Z

This PR depends on ipfs-inactive/js-ipfs-unixfs-engine#223.

vmx

Thanks a lot. Most of my comments are pretty minor ones.

vmx · 2018-08-06T15:31:03Z

src/cli/commands/files/add.js

@@ -135,6 +135,10 @@ module.exports = {
      default: false,
      describe: 'Only chunk and hash, do not write'
    },
+    chunker: {
+      default: 'default',
+      describe: 'Chunking algorithm to use, formatted like [default, size-{size}, rabin, rabin-{avg}, rabin-{min}-{avg}-{max}]'


The Go IPFS CLI uses size-262144 as default. Would it make sense to remove the default value from here?

I think that makes sense, will make that change.

vmx · 2018-08-06T15:32:34Z

src/core/utils.js

+    const sizeStr = chunker.split('-')[1]
+    const size = parseInt(sizeStr)
+    if (isNaN(size)) {
+      throw new Error('Parameter avg must be an integer')


I think this message is wrong. The parameter isn't the average, but the fixed size.

vmx · 2018-08-06T16:57:56Z

src/core/utils.js

+      }
+      break
+    case 4:
+      options.minChunkSize = parseSub(parts[1].split(':'), 'min')


This looks like a port of the Go implementations. It seems to support something like rabin-min:123-avg:456-max:789. This isn't documented (also not in the Go version). I lean towards not supporting it, i.e. less code, less docs, less testing, less bugs :)

Though it would be good to check with the Go team, what the original reason is for having support for that.

I talked with @whyrusleeping over irc and he said that we didn't have to worry too much about the alternative format, so I will remove.

vmx · 2018-08-06T17:02:31Z

src/core/utils.js

+      }
+      break
+    case 4:
+      options.minChunkSize = parseSub(parts[1].split(':'), 'min')


This looks like a port of the Go implementations. It seems to support something like rabin-min:123-avg:456-max:789. This isn't documented (also not in the Go version). I lean towards not supporting it, i.e. less code, less docs, less testing, less bugs :)

Though it would be good to check with the Go team, what the original reason is for having support for that.

vmx · 2018-08-06T17:10:20Z

test/core/utils.js

@@ -157,4 +157,51 @@ describe('utils', () => {
      })
    })
  })
+
+  describe('parseChunkerString', () => {
+    it('handles an empty string', () => {


Please add test cases for error cases like an unsupported chunker.

vmx · 2018-08-08T00:09:29Z

src/cli/commands/files/add.js

@@ -135,6 +135,10 @@ module.exports = {
      default: false,
      describe: 'Only chunk and hash, do not write'
    },
+    chunker: {
+      default: 'size-262144',
+      describe: 'Chunking algorithm to use, formatted like [default, size-{size}, rabin, rabin-{avg}, rabin-{min}-{avg}-{max}]'


This is really a minor thing: I've seen that the Go implementation can parse default as value, but also the CLI help of ipfs add --help doesn't show this option. I'd remove default from this list and also the code below. Less options, less bugs :)

vmx · 2018-08-08T16:26:14Z

src/core/utils.js

+ * @return {Object}   Chunker options for DAGBuilder
+ */
+function parseChunkerString (chunker) {
+  if (!chunker || chunker === '') {


Do we ever pass in an empty string (or undefined)? If not, this if case could be removed (and also removed from the JSDocs).

Looks safe to remove

vmx

Thanks a lot! LGTM, I think it's ready to merged. I'd prefer if someone from the js-ipfs would do the merge and hence confirm that it's good to go.

alanshaw · 2018-08-09T15:28:26Z

This looks great @dordille. Do you have time to submit a PR to https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#filesadd to document this new option?

dordille · 2018-08-09T16:54:10Z

Yeah, I’ll update the docs

alanshaw

This is great. Thank you! ✨

There's just a few issues to iron out before we can merge this. Comments inline.

Additionally, the option needs to also be added to the HTTP API here https://github.com/ipfs/js-ipfs/blob/master/src/http/api/resources/files.js#L152

alanshaw · 2018-08-10T11:35:56Z

src/core/components/files.js

@@ -133,12 +134,13 @@ class AddHelper extends Duplex {
 }

 module.exports = function files (self) {
-  function _addPullStream (options) {
+  function _addPullStream (options = {}) {
+    const chunkerOptions = parseChunkerString(options.chunker)


parseChunkerString can throw. add*Stream methods should never throw when called, but should return a stream that immediately errors. You need to do something like:

let chunkerOptions try { chunkerOptions = parseChunkerString(options.chunker) } catch (err) { return pull.map(() => { throw err }) }

alanshaw · 2018-08-10T12:58:39Z

src/http/api/resources/files.js

@@ -233,7 +233,8 @@ exports.add = {
      onlyHash: request.query['only-hash'],
      hashAlg: request.query['hash'],
      wrapWithDirectory: request.query['wrap-with-directory'],
-      pin: request.query.pin
+      pin: request.query.pin,
+      chunker: request.query['chunker']


request.query['chunker'] => request.query.chunker

dordille · 2018-08-13T16:03:38Z

@alanshaw Updated that http spec to include the option, can't retrigger a build though, looks like there wasn't enough space to run the build.

alanshaw · 2018-08-15T15:41:31Z

I've rebased this against master - there were test failures due to changes in dependencies that have now been resolved. Lets see what CI says now 🤞

This allows the chunking algorithm, and options to be specified when using the adding files. Specifying chunker and options are identical to go-ipfs and support the following formats: default, size-{size}, rabin, rabin-{avg}, rabin-{min}-{avg}-{max} This is required to achieve parity with go-ipfs. Fixes ipfs#1283 License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

…ull stream License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

alanshaw · 2018-08-24T10:53:12Z

Tests all passing, only commitlint that failed - am merging! Thanks @dordille ❤️

dordille mentioned this pull request Jul 26, 2018

Allow chunking algorithm to be specified when adding a file #1465

Closed

dordille force-pushed the rabin-chunker branch from 562e302 to 8a15c0e Compare July 26, 2018 17:49

dordille changed the title ~~Add option to specify chunking algorithm when adding files~~ (WIP) feat: Add option to specify chunking algorithm when adding files Jul 26, 2018

vmx suggested changes Aug 6, 2018

View reviewed changes

alanshaw changed the title ~~(WIP) feat: Add option to specify chunking algorithm when adding files~~ [WIP] feat: Add option to specify chunking algorithm when adding files Aug 7, 2018

vmx reviewed Aug 8, 2018

View reviewed changes

vmx approved these changes Aug 8, 2018

View reviewed changes

alanshaw self-requested a review August 9, 2018 11:20

alanshaw changed the title ~~[WIP] feat: Add option to specify chunking algorithm when adding files~~ feat: Add option to specify chunking algorithm when adding files Aug 9, 2018

alanshaw mentioned this pull request Aug 9, 2018

⚡️ v0.32.0 RELEASE 🚀 #1497

Closed

22 tasks

dordille mentioned this pull request Aug 9, 2018

docs: add chunker option to files.add documentation ipfs-inactive/interface-js-ipfs-core#350

Merged

alanshaw suggested changes Aug 10, 2018

View reviewed changes

alanshaw force-pushed the rabin-chunker branch from 582d880 to 30ea824 Compare August 15, 2018 15:40

ghost assigned alanshaw Aug 15, 2018

ghost added the status/in-progress In progress label Aug 15, 2018

alanshaw force-pushed the rabin-chunker branch 2 times, most recently from a5eb1e7 to f9c9d13 Compare August 23, 2018 19:19

dordille added 7 commits August 23, 2018 20:19

test: add tests for invalid chunker types and parameters

5867d52

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

perf(core): remove alternative rabin chunker format

f9cdfd5

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

perf(core): remove default as an accepted chunker format

d236470

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

perf(core): remove empty string check in parseChunkerString method

ee070f6

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

fix(api): add chunker to validated query keys in files.add

7b38101

License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

fix(core): catch errors when parsing chunkerstring and return error p…

f9c9d13

…ull stream License: MIT Signed-off-by: Dan Ordille <dordille@gmail.com>

alanshaw merged commit 4f805d3 into ipfs:master Aug 24, 2018

ghost removed the status/in-progress In progress label Aug 24, 2018

This was referenced Sep 15, 2021

[Snyk] Fix for 1 vulnerabilities rtpro/js-ipfs#45

Open

[Snyk] Fix for 1 vulnerabilities awaisab172/js-ipfs#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add option to specify chunking algorithm when adding files #1469

feat: Add option to specify chunking algorithm when adding files #1469

dordille commented Jul 26, 2018

vmx commented Aug 6, 2018 •

edited

Loading

vmx left a comment

vmx Aug 6, 2018

dordille Aug 7, 2018

vmx Aug 6, 2018

vmx Aug 6, 2018

dordille Aug 7, 2018

vmx Aug 6, 2018

vmx Aug 6, 2018

vmx Aug 8, 2018

vmx Aug 8, 2018

dordille Aug 8, 2018

vmx left a comment

alanshaw commented Aug 9, 2018

dordille commented Aug 9, 2018

alanshaw left a comment

alanshaw Aug 10, 2018

alanshaw Aug 10, 2018

dordille commented Aug 13, 2018 •

edited

Loading

alanshaw commented Aug 15, 2018

alanshaw commented Aug 24, 2018

feat: Add option to specify chunking algorithm when adding files #1469

feat: Add option to specify chunking algorithm when adding files #1469

Conversation

dordille commented Jul 26, 2018

vmx commented Aug 6, 2018 • edited Loading

vmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

alanshaw commented Aug 9, 2018

dordille commented Aug 9, 2018

alanshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dordille commented Aug 13, 2018 • edited Loading

alanshaw commented Aug 15, 2018

alanshaw commented Aug 24, 2018

vmx commented Aug 6, 2018 •

edited

Loading

dordille commented Aug 13, 2018 •

edited

Loading