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

Use ResolveUnixfsOnce as default resolver. #4444

Closed
wants to merge 1 commit into from
Closed

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Nov 30, 2017

Might fix #4431

License: MIT
Signed-off-by: Kevin Atkinson <k@kevina.org>
@Stebalien
Copy link
Member

This seems like a good opportunity to fix the IPLD/IPFS namespace problem. That is, we should be using /ipld/QmId for IPLD addresses and /ipfs/QmId for IPFS addresses (and choosing the resolver based on that namespace).

@kevina
Copy link
Contributor Author

kevina commented Nov 30, 2017

@Stebalien actually it is core.Resolve job to handle the prefix. That function takes a protocol specific resolver as a parameter. What this is doing is changing the protocol specific resolver normally used from ResolveSingle to ResolveUnixfsOnce.

@Stebalien
Copy link
Member

This shouldn't have a default resolver. It should chose the resolver based on the protocol.

However, looking into this, it looks like all the resolver code is a mess so this is probably the best fix for now.

@Stebalien
Copy link
Member

However, I am worried that this may break things.

@kevina
Copy link
Contributor Author

kevina commented Dec 2, 2017

We agree that the resolver code is a mess. 😃

@whyrusleeping
Copy link
Member

My worry here is that this will break things like that actually do really want the raw ipld resolver (like ipfs dag). Lets add some tests to make sure that you can use ipfs dag get to view the raw structure of a sharded directory.

@kevina
Copy link
Contributor Author

kevina commented Dec 6, 2017

@whyrusleeping Unless I am missing something this will change the resolver not the output of ipfs dag get or ipfs refs, both commands should still show the raw dag. What do you expect might break?

@whyrusleeping
Copy link
Member

@kevina

ipfs dag get QmFooBar/shardedDirectory/AF/B2/C7filename

Thats how i expect paths to be addressed with ipfs dag, since its a command that operates on the base ipld layer (and contains no unixfs context)

@kevina
Copy link
Contributor Author

kevina commented Dec 6, 2017

@whyrusleeping I didn't know that was possible, but will add some tests cases for it

@kevina
Copy link
Contributor Author

kevina commented Dec 6, 2017

@whyrusleeping so are you saying you want command like ipfs dag to return different results then commands like ipfs get ipfs cat ipfs resolve, etc.

I am not sure I agree, because then we need to go through each command one by one and sort out which resolver to use for each. We might also want a --raw-ipld for commands where wither resolver type could be used. This in my option is just asking for pain.

I think we should have different syntax for resolving a raw dag vs a higher level path. Maybe something like. QmFooBar/shardedDirectory/@AF/@B2/.

@Stebalien, and others, thought.

As it stands right now ipfs cat and ipfs ls use the higher level resolver and most other commands use the lower level resolver.

@Stebalien
Copy link
Member

I think we should have different syntax for resolving a raw dag vs a higher level path

The correct fix, as far as I know, is to differentiate between /ipfs/... and /ipld/.... There is no good fix without doing that.

However, for now, we should probably have get, cat, files * and maybe object * work with ipfs paths and dag * block * work with ipld paths. In the future, we'd expand this to be "unless otherwise specified (with a path prefix of /ipld or /ipfs).

Resolve, unfortunately, is tricky. In this case, we really do need to look at the path prefix. If we have an IPNS link that points to /ipfs/QmId, we need to use the IPFS resolver.

@kevina
Copy link
Contributor Author

kevina commented Dec 9, 2017

@Stebalien I think the solution then is to make the default resolver also handle ipld prefixes. I can take a stab at it but will likely need some guidance. I would say the default resolve should default to /ipfs when a prefix is not given with an option to change the default to /ipld for low level commands such as dag block etc.

I will also see what I can do about cleaning up the code in general.

@Stebalien
Copy link
Member

I think the solution then is to make the default resolver also handle ipld prefixes. I can take a stab at it but will likely need some guidance. I would say the default resolve should default to /ipfs when a prefix is not given with an option to change the default to /ipld for low level commands such as dag block etc.

I'd actually take a different approach.

  1. The global resolver (not default) would take paths in the form of /ns/rest. The /ns/ would be mandatory. It would then delegate resolution to sub-resolvers (you'd be able to register resolvers for different namespaces).
  2. Individual API commands would prepend /ns/ as necessary depending on the command.
  3. There are no "default" resolvers. The global resolver picks the correct resolver based on /ns/.

That way, we handle all of these ambiguities at the edges.

@kevina
Copy link
Contributor Author

kevina commented Dec 16, 2017

@Stebalien I think that is a good plan. My only concern is the performance cost of the extra allocation when prefixing plain CIDs with "/ipfs" only to have it immediately removed, but if it is not a frequent operation I doubt it will mater.

I will see what I can make work, I am thinking that the resolver should take in a map that maps prefixed to protocol specific resolvers.

@Stebalien
Copy link
Member

My only concern is the performance cost of the extra allocation when prefixing plain CIDs with "/ipfs" only to have it immediately removed, but if it is not a frequent operation I doubt it will mater.

Doing this at the edge should help. Also, allocations... go-ipfs... *snicker*.

I will see what I can make work, I am thinking that the resolver should take in a map that maps prefixed to protocol specific resolvers.

Something like that. Personally, I was thinking of something more like a registry. There's a single Resolver attached to the IpfsNode where one can call Register(namespace, resolver) to register a resolver for that namespace. That keeps it modular and allows, e.g., plugins to register new namespaces.

@kevina
Copy link
Contributor Author

kevina commented Dec 16, 2017

@Stebalien

Doing this at the edge should help.

I'm not sure I am following, what exactly do you mean by "at the edge".

@Stebalien
Copy link
Member

Superseded by the CoreAPI refactor.

@Stebalien Stebalien closed this Jan 24, 2019
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@Stebalien Stebalien deleted the kevina/test branch February 28, 2019 22:26
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.

Long hierarchical IPFS paths fail to resolve (HAMT, object links)
3 participants