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

fix: object.patch.rmLink not working #1508

Merged
merged 2 commits into from
Aug 15, 2018
Merged

fix: object.patch.rmLink not working #1508

merged 2 commits into from
Aug 15, 2018

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Aug 15, 2018 •
edited
Loading

This PR contains fixes for the test failures that are happening in master for object.patch.rmLink.

I think this change ipld/js-ipld-dag-pb#80 made DAGLink validate the hash better and caused the failures to start.

It also contains fixes for the pinSet module which was using the "private" _multihash property which was removed in ipld/js-ipld-dag-pb#81 and released 2 days ago - don't use private APIs kids.

@alanshaw
fix: object.patch.rmLink not working
75c1a7b 
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@ghost ghost assigned alanshaw Aug 15, 2018
@ghost ghost added the status/in-progress In progress label Aug 15, 2018
@alanshaw alanshaw mentioned this pull request Aug 15, 2018
Merged
achingbrain
achingbrain approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

I think we might want to make ipfs.object.patch.rmLink take a string arg (e.g. link name) in future.

@alanshaw
Copy link
Member Author

:( looks like there's now problems with pinSet

@achingbrain
Copy link
Member

achingbrain commented Aug 15, 2018 •
edited
Loading

I'm sure you've found it already but it looks like passing '' as the multihash is the cause:

https://github.com/ipfs/js-ipfs/blob/master/src/core/components/pin-set.js#L115
https://github.com/ipfs/js-ipfs/blob/master/src/core/components/pin-set.js#L121

  pinSet storeItems
Debug: internal, implementation, error 
    AssertionError [ERR_ASSERTION]: A link requires a multihash to point to
    at new DAGLink (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/ipld-dag-pb/src/dag-link/index.js:10:5)
    at new DAGLink (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/class-is/index.js:15:17)
    at pins.map.item (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:121:21)
    at Array.map (<anonymous>)
    at storePins (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:120:14)
    at Object.storeItems (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:102:14)
    at Object.storeSet (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:91:14)
    at createNode (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/test/core/pin-set.js:77:16)
    at multihashing (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/ipld-dag-pb/src/dag-node/create.js:53:7)
    at Multihashing.Multihashing.digest (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/index.js:33:5)
    at setImmediate (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/utils.js:8:7)
    at Immediate.<anonymous> (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/setImmediate.js:27:16)
    at runCallback (timers.js:696:18)
    at tryOnImmediate (timers.js:667:5)
    at processImmediate (timers.js:649:5)
    at process.topLevelDomainCallback (domain.js:121:23)
      1) generates a root node with links and hash

  Suite duration: 5.002 s, Tests: 1

  pinSet handles large sets
Debug: internal, implementation, error 
    Error: Invalid version, must be a number equal to 1 or 0
    at Function.validateCID (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/cids/src/index.js:235:13)
    at new CID (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/cids/src/index.js:105:9)
    at toB58String (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:20:10)
    at hash (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:53:22)
    at pins.reduce (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:148:23)
    at Array.reduce (<anonymous>)
    at storePins (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:147:29)
    at Object.storeItems (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:102:14)
    at Object.storeSet (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:91:14)
    at createNodes (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/test/core/pin-set.js:99:16)
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/parallel.js:39:9
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/once.js:12:16
    at iterateeCallback (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/eachOfLimit.js:48:24)
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/onlyOnce.js:12:16
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/parallel.js:36:13
    at createNode (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/test/core/pin-set.js:34:44)
    at multihashing (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/ipld-dag-pb/src/dag-node/create.js:53:7)
    at Multihashing.Multihashing.digest (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/index.js:33:5)
    at setImmediate (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/utils.js:8:7)
    at Immediate.<anonymous> (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/setImmediate.js:27:16)
    at runCallback (timers.js:696:18)
    at tryOnImmediate (timers.js:667:5)
    at processImmediate (timers.js:649:5)
    at process.topLevelDomainCallback (domain.js:121:23)
      1) handles storing items > maxItems
      - stress test: stores items > (maxItems * defaultFanout) + 1

  Suite duration: 19.001 s, Tests: 2

  pinSet walkItems
      ✓ fails if node doesn't have a pin-set protobuf header: 1ms
Debug: internal, implementation, error 
    AssertionError [ERR_ASSERTION]: A link requires a multihash to point to
    at new DAGLink (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/ipld-dag-pb/src/dag-link/index.js:10:5)
    at new DAGLink (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/class-is/index.js:15:17)
    at pins.map.item (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:121:21)
    at Array.map (<anonymous>)
    at storePins (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:120:14)
    at Object.storeItems (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:102:14)
    at Object.storeSet (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/src/core/components/pin-set.js:91:14)
    at createNodes (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/test/core/pin-set.js:175:16)
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/parallel.js:39:9
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/once.js:12:16
    at iterateeCallback (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/eachOfLimit.js:48:24)
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/onlyOnce.js:12:16
    at /tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/parallel.js:36:13
    at createNode (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/test/core/pin-set.js:34:44)
    at multihashing (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/ipld-dag-pb/src/dag-node/create.js:53:7)
    at Multihashing.Multihashing.digest (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/index.js:33:5)
    at setImmediate (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/multihashing-async/src/utils.js:8:7)
    at Immediate.<anonymous> (/tmp/jenkins/workspace/_js-ipfs_fix_object-rm-link-VLV2TBPORAS7C63UJSPGGK6BLAQUD4RI3ONWVTNAAKSYT5XGXBRQ/node_modules/async/internal/setImmediate.js:27:16)
    at runCallback (timers.js:696:18)
    at tryOnImmediate (timers.js:667:5)
    at processImmediate (timers.js:649:5)
    at process.topLevelDomainCallback (domain.js:121:23)
      1) visits all non-fanout links of a root node

@alanshaw
fix: pinSet DAG link was using private _multihash prop
ff102e6 
...which was removed in ipld/js-ipld-dag-pb#81

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@daviddias
Copy link
Member

I'm sure you've found it already but it looks like passing '' as the multihash is the cause:

Please add a test for this and give the user a good error if it happens.

@alanshaw
Copy link
Member Author

I'm sure you've found it already but it looks like passing '' as the multihash is the cause:

Please add a test for this and give the user a good error if it happens.

It was actually because the pinSet module was using the "private" _multihash property which was removed in ipld/js-ipld-dag-pb#81 and released 2 days ago. I've just switched this to use multihash and we're all good.

@alanshaw
Copy link
Member Author

CI green - am merge ✨

@alanshaw alanshaw merged commit afd3255 into master Aug 15, 2018
@alanshaw alanshaw deleted the fix/object-rm-link branch August 15, 2018 15:20
@ghost ghost removed the status/in-progress In progress label Aug 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achingbrain achingbrain achingbrain approved these changes

@daviddias daviddias Awaiting requested review from daviddias

Assignees

@alanshaw alanshaw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alanshaw @achingbrain @daviddias