fix: add support for resolving links by name #78

daviddias · 2018-07-19T13:52:57Z

This is just a stab at fixing the resolve links by name on dag-pb nodes. This PR is not finished (at all) but it shows what is missing.

Short story: go-ipfs and js-ipfs historically have supported the ability to do jsipfs dag get CID/a where a is a named link in a dag-pb.

So a dag-pb node like this.

{
  Data: <some data>
  Links: [
    {
       Name: a
       Hash: b58...
       Size: 1000
    }
  ]
}

Should support resolving the link through name so that both jsipfs dag get CID/a and jsipfs dag get CID/Links/0/Hash are equivalent.

codecov · 2018-07-19T13:56:16Z

Codecov Report

Merging #78 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master     #78     +/-   ##
========================================
+ Coverage    90.6%   90.8%   +0.2%     
========================================
  Files          13      13             
  Lines         266     272      +6     
========================================
+ Hits          241     247      +6     
  Misses         25      25

Impacted Files	Coverage Δ
src/resolver.js	`95.65% <100%> (+0.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda23c4...bfd2886. Read the comment docs.

daviddias · 2018-07-19T17:42:59Z

@vmx can you check on this with @olizilla and @alanshaw. They are looking for this feature in for the WebUI.

@victorbjelkholm can we make sure to disable codecov comments?

vmx · 2018-07-19T20:24:38Z

@diasdavid Do you mean that I should finish this PR?

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

alanshaw · 2018-07-19T21:30:30Z

I hope you don't mind, I made some fixes and added a test - should be good to go now.

vmx

I did a review. If I should take this PR over (I'd prefer not to), please let me know an I'd fix those things myself.

vmx · 2018-07-20T09:09:07Z

src/resolver.js

+          // TODO by enabling something to resolve through link name, we are
+          // applying a transformation (a view) to the data, confirm if this
+          // is exactly what we want
+          values[i] = values[link.name] = {


I don't think values[link.name] is needed here. The case is handled below.

Please also remove the "TODO".

This was already in the code base although I don't think it was working correctly:

js-ipld-dag-pb/src/resolver.js

Lines 54 to 57 in fda23c4

// TODO by enabling something to resolve through link name, we are

// applying a transformation (a view) to the data, confirm if this

// is exactly what we want

values[link.name] = link.multihash

It allows you to reference links by name via Links e.g. /ipfs/QmHash/Links/myNamedLink

Got it. So this is kind of the IPFS vs. IPLD like traversal.

Yeah, I don't know if it's really necessary to be able to /ipfs/QmHash/Links/myNamedLink as well as /ipfs/QmHash/myNamedLink but the former was already there and we're adding the latter.

/ipfs/QmHash/Links/myNamedLink is what I call IPLD-like traversal. It will then be use if you use the /ipld prefix.

vmx · 2018-07-20T09:09:46Z

src/resolver.js

@@ -74,6 +73,31 @@ exports.resolve = (binaryBlob, path, callback) => {
      } else if (split[0] === 'Data') {
        cb(null, { value: node.data, remainderPath: '' })
      } else {
+        const values = {}


Can you please add a comment saying that this is the case of a named link? It took me a while to figure that out.

vmx · 2018-07-20T09:10:43Z

src/resolver.js

+          // TODO by enabling something to resolve through link name, we are
+          // applying a transformation (a view) to the data, confirm if this
+          // is exactly what we want
+          values[i] = values[link.name] = {


The assignment to values[i] is not needed here and I find it rather confusing.

Please remote the comments/change them to make the saying the right thing.

vmx · 2018-07-20T09:14:45Z

test/resolver.spec.js

+          done()
+        })
+      })
+


Could you please add tests for:

named link and a remainder path

error case for non-existent named link

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

alanshaw · 2018-07-20T09:58:51Z

Is good now @vmx? macOS tests are npm failure not test failure.

vmx

Thanks!

Please merge it (I normally let the author merge). Please squash them up into a single commit as it is one unit of change.

vmx · 2018-07-20T10:10:00Z

@alanshaw yes it's good to be merged.

fix: add support for resolving links by name

a5d7640

daviddias added kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP exp/expert Having worked on the specific codebase is important labels Jul 19, 2018

ipld deleted a comment from codecov bot Jul 19, 2018

fix: resolver links by name

ed9af60

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

alanshaw changed the title ~~wip - fix: add support for resolving links by name~~ fix: add support for resolving links by name Jul 19, 2018

alanshaw mentioned this pull request Jul 19, 2018

feat: add partial implementation for ipfs.resolve ipfs/js-ipfs#1455

Merged

vmx suggested changes Jul 20, 2018

View reviewed changes

alanshaw added 2 commits July 20, 2018 10:52

test: add tests for missing link and named link remainderPath

f1816c9

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

fix: tweaks from PR review

bfd2886

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

vmx approved these changes Jul 20, 2018

View reviewed changes

alanshaw merged commit 3f6f094 into master Jul 20, 2018

alanshaw deleted the resolve-by-name branch July 20, 2018 10:38

victorb mentioned this pull request Jul 20, 2018

Make sure CodeCov comments are disabled ipfs-inactive/dev-team-enablement#57

Open

alanshaw mentioned this pull request Jul 23, 2018

[WIP] feat(dag): add IPLD to dag.get ipfs-inactive/js-ipfs-http-client#755

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add support for resolving links by name #78

fix: add support for resolving links by name #78

daviddias commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading

daviddias commented Jul 19, 2018

vmx commented Jul 19, 2018

alanshaw commented Jul 19, 2018

vmx left a comment

vmx Jul 20, 2018

alanshaw Jul 20, 2018

vmx Jul 20, 2018

alanshaw Jul 20, 2018

vmx Jul 20, 2018

vmx Jul 20, 2018

vmx Jul 20, 2018

vmx Jul 20, 2018

alanshaw commented Jul 20, 2018

vmx left a comment

vmx commented Jul 20, 2018

	// TODO by enabling something to resolve through link name, we are
	// applying a transformation (a view) to the data, confirm if this
	// is exactly what we want
	values[link.name] = link.multihash

fix: add support for resolving links by name #78

fix: add support for resolving links by name #78

Conversation

daviddias commented Jul 19, 2018 • edited Loading

codecov bot commented Jul 19, 2018 • edited Loading

Codecov Report

daviddias commented Jul 19, 2018

vmx commented Jul 19, 2018

alanshaw commented Jul 19, 2018

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

alanshaw commented Jul 20, 2018

vmx left a comment

Choose a reason for hiding this comment

vmx commented Jul 20, 2018

daviddias commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading