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

Provide path for getting sizes on directory iteration #60

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

willscott
Copy link
Collaborator

When making a map iterator over a directory, calls to .Next() currently return a value of the FieldHash() of the dagpb link.

This change has them instead return a IterLink.

In both cases, the "exposed" interface of the returned type, AsLink() exposes the underlying hash cidLink to the entry.

By using a wrapping IterLink type, we provide an opportunity to type-cast the response, and instead of treating the response under it's returned ipld.Node interface, the client has the ability to do something of the form:

k, next, err := iterator.Next()
nextSize := next.(*iter.IterLink).Substrate.FieldSize()

…ntly return a value of the `FieldHash()` of the dagpb link.

This change has them instead return a `IterLink`.

In both cases, the "exposed" interface of the returned type, `AsLink()` exposes the underlying hash cidLink to the entry.

By using a wrapping `IterLink` type, we provide an opportunity to type-cast the response, and instead of treating the response under it's returned `ipld.Node` interface,
the client has the ability to do something of the form:
```
k, next, err := iterator.Next()
nextSize := next.(*iter.IterLink).Substrate.FieldSize()
```
@willscott
Copy link
Collaborator Author

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM -- much better than my take

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me and IMO less confusing than #51.

An area I'm fuzzy about is what the expectations are for ipld.Node around getting out data of multiple forms. For example, should this have been able to return a PBLink and PBLink be able to easily return a cidlink.Link and the rest of its fields? If we need a separate wrapper struct (e.g. IterLink) would this be better off as another type of PBLink in go-codec-dagpb?

Answering these isn't a blocker from me, but something I'm curious about from the people who work more closely with go-ipld-prime (e.g. @rvagg). If we need to break this in the future it's not the end of the world or anything, but if we can figure this out sooner it'll save some pain later.

@willscott
Copy link
Collaborator Author

I went with this so that the .AsLink() directly provides .FieldHash().AsLink() of the PBLink as a way of maintaining compatibility with current semantics.

This is (if i remember correctly) to provide the higher level semantics of directory traversal that are used for pathing through unixfs.

@aschmahmann
Copy link
Contributor

@willscott I agree, and probably changing PBLink to allow AsLink to return a cidlink.Link would be conceptually problematic (IIUC the rough go-ipld-prime node contract is that an object is exactly one of the data model types not multiple). I wouldn't be surprised if other uses of PBLink would run into similar issues and want a standard way to access both representations of the data (all the fields, and just the CID). Again, not a blocker for me though.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

seems inoffensive to me

@rvagg
Copy link
Member

rvagg commented Aug 10, 2023

Good question @aschmahmann, I'm not sure there's a clean answer (or that I'm the best person to provide one!); except to say that I think there should be a reasonable expectation of consistent behaviour of an ipld.Node based on inspecting its Kind(). A PBLink is a Kind_Map and making it act like a link might lead to some funky (unexpected) behaviour in strange parts of a system using it.

You could look at #51 with that lens and see how the approach here is a bit cleaner. Not that #51 has obvious breakage potential, but this approach has the standard mapping of Kind:Behaviour.

@willscott willscott merged commit 38cb88e into main Aug 10, 2023
18 checks passed
@willscott willscott deleted the feat/linkSubstrate branch August 10, 2023 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants