Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError while opening Links/ #279

Closed
lidel opened this issue Sep 21, 2020 · 9 comments · Fixed by #360
Closed

TypeError while opening Links/ #279

lidel opened this issue Sep 21, 2020 · 9 comments · Fixed by #360
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) need/analysis Needs further analysis before proceeding P2 Medium: Good to have, but can wait until someone steps up

Comments

@lidel
Copy link
Member

lidel commented Sep 21, 2020

Explorer returns an error while traversing some DAGs.

Steps to reproduce:

  1. Open QmWYmE8fzNRYg48E5PaDhMfHqfbPjyQVmUjGQfgAsa4NYq
  2. Click on Links/0, you will be moved to QmY4HSz1oVGdUzb8poVYPLsoqBZjH6LZrtgnme9wWn2Qko
  3. Then click on Links/0 again, you will see error looking similar to this:

    Screenshot_2020-09-21 Exploring IPLD

Demo: https://webui.ipfs.io/#/explore/QmWYmE8fzNRYg48E5PaDhMfHqfbPjyQVmUjGQfgAsa4NYq/Links/0

Additional notes:

  • afaik this happens only when trying to open leaf nodes that are not raw leaves (dag-pb without any children) – perhaps something in IPLD APIs changed and we now need to check if Links array is present?
@lidel lidel added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Sep 21, 2020
@ipfs ipfs deleted a comment from welcome bot Sep 21, 2020
@lidel
Copy link
Member Author

lidel commented Sep 21, 2020

I have no bandwidth to dig into this, but if someone wants to pick this up, comments by @rvagg in ipld/specs#297 sound relevant.

I suspect the current JS implementation expects [] but gets undefined instead, and that causes the error in IPLD Explorer.

@jessicaschilling jessicaschilling added exp/intermediate Prior experience is likely helpful effort/days Estimated to take multiple days, but less than a week 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 and removed need/triage Needs initial labeling and prioritization labels Sep 21, 2020
@rvagg
Copy link
Member

rvagg commented Sep 22, 2020

I'm trying to understand this but finding I don't have nearly enough background in how all of these components work. I'm poking around in js-ipfs to no avail but maybe I'm not even looking in the right place.

The work in the specs repo are for a newer implementation of dag-pb that this shouldn't impact. I don't believe there's been any major changes that should impact this in a while, this was probably the biggest thing (aside from Buffer -> Uint8Array): ipld/js-ipld-dag-pb#184

I'm pretty sure that there will always be a Links array in the current ipld-dag-pb, regardless of what's in the protobuf (this will likely change for the new version - it'll either have elements or not be there at all, although Eric was hinting today that he might prefer an empty array to be the no-links case, we'll discuss further). That terminal without links comes out like this when running in the repl:

> ipldDagPb.util.deserialize(fs.readFileSync('./QmRk1rduJvo5DfEYAaLobS2za9tDszk35hzaNSDCJ74DA7'))
DAGNode {
  Data: <Buffer 08 02 12 80 80 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 262104 more bytes>,
  Links: []
}

(there is no Links in the raw pb, but it's created anyway).

Can you tell me how /Links/0/Links/0 is supposed to work? According to the data model form that we put out from ipld-dag-pb it should be /Links/0/Hash/Links/0/Hash and I can run this on the js-ipfs CLI and have it work: js-ipfs dag get QmWYmE8fzNRYg48E5PaDhMfHqfbPjyQVmUjGQfgAsa4NYq/Links/0/Hash/Links/0/Hash. I don't think we have a way to transparently resolve a Link to a CID, it has to go through Hash, but maybe there was a historical time when this worked? It doesn't work in go-ipfs which has its own pathing mechanism for DAG-PB (one of the things that I'm trying to get properly unified for the new JS and Go implementations). Maybe there's some hard-wiring of this Links array that needs to be undone so that they display the contents of each Link element instead?

@rvagg
Copy link
Member

rvagg commented Sep 22, 2020

Ah, I think I have it!

https://github.com/ipfs-shipyard/ipld-explorer-components/blob/07dc1837ee8ff6f791db312ad00f4a2aef1441d6/src/lib/normalise-dag-node.js#L63

extracting { name, size, cid } from a DAGLink object to support this pathing. That was changed in ipld/js-ipld-dag-pb#127

It should now be { Name, Tsize, Hash }, which will map to what you need. The set of links displayed should probably look like these and be navigable by /Links/x/Hash/... rather than the /Links/x/... magic. I don't know how painful that would be to change but it'll be the "correct" and future-proof version for DAG-PB.

@rvagg
Copy link
Member

rvagg commented Sep 22, 2020

oh, and this: path: name || `Links/${index}`, maybe could change too - this magic named link stuff is going away too. Only go-ipfs supports this method, it doesn't work in JS and it's being removed in the newer JS & Go libraries entirely. So it's just pure: { Data: Uint8Array, Links: [ { Hash: CID, Name: string, Tsize: number }, ... ] } (where most of the properties can be omitted, but we're making Hash mandatory for links, but an entirely empty block is still possible).

@lidel lidel added need/triage Needs initial labeling and prioritization need/analysis Needs further analysis before proceeding and removed status/ready Ready to be worked labels Oct 19, 2020
@jessicaschilling

This comment has been minimized.

@lidel lidel removed the need/triage Needs initial labeling and prioritization label Jun 18, 2021
@lidel lidel assigned olizilla and unassigned olizilla Jun 18, 2021
@vasco-santos
Copy link
Member

Cannot assign me to this issue, I will try @rvagg 's suggestions

@vasco-santos
Copy link
Member

extracting { name, size, cid } from a DAGLink object to support this pathing. That was changed in ipld/js-ipld-dag-pb#127
It should now be { Name, Tsize, Hash }, which will map to what you need.

I tried this, but I had no success. Looking at what actually the object contains, it seems that the previous code is correct, we iterate over this type of objects:

cid: "QmRk1rduJvo5DfEYAaLobS2za9tDszk35hzaNSDCJ74DA7"
name: ""
size: 262158

I found a potential solution, that I will PR shortly

@lidel
Copy link
Member Author

lidel commented Jun 25, 2021

For what its worth, we have a bunch of technical debt in this repo (was created way before we had proper IPLD in js-ipfs) and in go-ipfs (it still does not use ipld-prime).

There may not be an easy fix that works with goth js-ipfs and js-ipfs-http-client in webui – I feel a bigger refactor is needed after ipld prime work lands everywhere.

@BigLep BigLep assigned olizilla and unassigned olizilla Jul 2, 2021
@SgtPooki
Copy link
Member

I believe this is fixed on my branch where i'm fixing #359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) need/analysis Needs further analysis before proceeding P2 Medium: Good to have, but can wait until someone steps up
Projects
No open projects
Status: Done
6 participants