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.

Sign up for GitHub

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

relaxed reification #27

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

relaxed reification #27

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
953bdf1
relaxed reification
willscott Mar 6, 2022
e943499
add ReifyDagPB, add tests, update docs
rvagg Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion reification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ import (
func Reify(lnkCtx ipld.LinkContext, maybePBNodeRoot ipld.Node, lsys *ipld.LinkSystem) (ipld.Node, error) {
pbNode, ok := maybePBNodeRoot.(dagpb.PBNode)
if !ok {
return maybePBNodeRoot, nil
// see if the node has the right structure anyway
pbb := dagpb.Type.PBNode.NewBuilder()
if err := pbb.AssignNode(maybePBNodeRoot); err != nil {
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

@willscott this will this change the behavior of Reify such that it's no longer spec compliant? i.e. UnixFSv1 is only defined over DagPB nodes, will this allow me to create DagCBOR objects that will render as long as they are convertable into DagPB (i.e. they contain the fields in https://ipld.io/specs/codecs/dag-pb/spec/#serial-format)?

If so, you may want to define some LooseReify or something that can be used for testing and some conversion work but is called out as non spec-compliant.

I may have misinterpreted how this works though and if so please correct me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is used in a selector where the requester explicitly signals interpretAs: unixfs. If the data at the point they signal fits the data structure of unixfs (e.g. a data field in a struct that can itself be unmarshaled with dagpb) that seems like a pretty reasonable thing to do, rather than block on the concrete implementation.

Note that this isn't DagPB the serialization, but rather DagPB the concrete schema types. I could have valid dagpb data, but if it's been re-built into a basicnode wrapper, e.g. due to a WalkTransforming or other ipld internal process, you would also have this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so this effectively is a UnixFS spec change. It means that any applications that rely on UnixFS behavior, and this package, (e.g. ipfs cat, ipfs.io/ipfs/bafyfoobar, ...) will have their behavior changed to be more relaxed.

I don't think modifying a spec implicitly for the purposes of testing is a particularly good idea. IIUC we ran into this problem last year where originally /ipfs/bafydagcbor/myunixfsfield was not supposed to resolve but did due to lax implementation which as a result of users building structures like this has effectively forced us to be stuck with this and cause problems for our UnixFS upgrade path.

Would it be problematic to just define a new ADL "UnixFSLoose" and we can use that without requiring a UnixFS spec change (which you could propose, but might take some time to accumulate and integrate feedback).

It seems like maintaining the UnixFS spec within the go-ipld-prime context and ADLs might start to get more complicated since UnixFS is only defined over DagPB.

Should we be considering a UnixFS spec change that redefines them in terms of either ADL "interfaces" or data model structures going forward here? Mostly I'd like us to not make spec changes in code without ecosystem involvement of the various people that depend on the behavior of UnixFS (i.e. a huge portion of the IPFS ecosystem).

I was thinking this might be a bit complicated and so a new ADL would allow us to side-step this.

cc @mvdan @rvagg @ribasushi in case you have any thoughts or advice here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the spec you link does not specify that the outer structure that the unixfs protobuf is encoded within (e.g a struct with a 'data' and 'links' field) needs to be serialized using the dag pb codec. If i took that same data and happened to transfer it over the wire in a cbor codec, would it stop being unixfs data?

Copy link

Choose a reason for hiding this comment

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

@willscott it's a little obscure but the spec does mandate pb-in-pb:
https://github.com/ipfs/specs/blame/master/UNIXFS.md#L81

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even that can be interpreted with some flexibility: it specifies that the outer container of the data follows the merkeldag protocol buffer structure of 'Data' and 'Links', but does not express that it must be encoded using the 'dagpb' codec vs other codecs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This data is serialized and placed inside the 'Data' field of the outer merkledag protobuf, which also contains the actual links to the child nodes of this object.

"Placed inside of the outer merkledag protobuf" seems pretty clear that it's a protobuf, not to mention all of the existing tooling that insists it's a protobuf.

Basically, I keep seeing perceived ambiguity in our specs cause implementation issues (dag-cbor in /ipfs, Bitswap protobuf ambiguity, /api/v0 on gateways, etc.). I am strongly opposed to changing specs without changing the specs, especially when a written spec already exists.

If we want to change the specs then let's open a spec PR and figure it out (changing specs is fine, but doing so in some Go repo is IMO not), otherwise I've been told that multiple ADLs are supposed to be allowed to exist so why not just make a new one?

Copy link

Choose a reason for hiding this comment

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

➕ on above - no spec changes without spec changes please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can continue in ipfs/specs#271

return maybePBNodeRoot, nil
}
pbNode = pbb.Build().(dagpb.PBNode)
}
if !pbNode.FieldData().Exists() {
// no data field, therefore, not UnixFS
Expand Down