-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix resolving links in sharded directories on gateway #5271
Conversation
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
plugin/loader/load_linux.go
Outdated
@@ -1,3 +1,5 @@ | |||
// +build !noplugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a git add .
.
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
should be ready |
path/resolver/resolver.go
Outdated
continue | ||
} | ||
|
||
val, rest, err := nd.Resolve(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 'rest' not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems right (although rest should probably be _
). This last resolve step is just to make sure that the rest of the path can be resolved. However, the function is only supposed to resolve up to the last node (it returns that last node and the rest of the path through this object).
Although, unless I'm mistaken, rest should be empty. It might make sense to check that.
path/resolver/resolver.go
Outdated
return nil, nil, err | ||
} | ||
lnk, rest, err := r.ResolveOnce(ctx, r.DAG, nd, p) | ||
if lnk != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could clean this loop up a little bit by pulling everything below this if statement out of the for loop, then reversing the logic to 'break' if lnk == nil
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
path/resolver/resolver.go
Outdated
@@ -69,25 +69,42 @@ func (r *Resolver) ResolveToLastNode(ctx context.Context, fpath path.Path) (ipld | |||
} | |||
|
|||
for len(p) > 0 { | |||
val, rest, err := nd.Resolve(p) | |||
lnk, rest, err := r.ResolveOnce(ctx, r.DAG, nd, p) | |||
if lnk == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll drop the error, as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually have to drop the error here as ResolveOnce
will try to either resolve to a full node or to a link (so for echo '{"foo":123}' | ipfs dag put
, trying to resolve /foo
it would return an error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, interesting. Can we make sure to document that explicitly? At a glance, it looks wrong, so let's make sure nobody comes along and 'fixes' it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably remove the error check below in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can technically still be set at that point, but that would be a rather weird edge case
License: MIT Signed-off-by: Łukasz Magiera <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the fix @magik6k :)
Fixes #5270