-
-
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
Refactoring towards gateway extraction #4418
Conversation
License: MIT Signed-off-by: Lars Gierth <[email protected]>
License: MIT Signed-off-by: Lars Gierth <[email protected]>
I'll fix the codeclimate errors |
@lgierth I think it would be really helpful to the whole coreapi effort to have some examples that show how its used in a couple different usecases. Using the go examples thing also allows to add tests and documentation at the same time :) |
if rsegs[0] == ipnsPathPrefix { | ||
webError(w, "putHandler: updating named entries not supported", errors.New("WritableGateway: ipns put not supported"), http.StatusBadRequest) | ||
return | ||
} | ||
|
||
var newnode node.Node | ||
if rsegs[len(rsegs)-1] == "QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn" { | ||
// TODO(lgierth): this seems to mean that PUT /ipfs/QmUNLL/foo/bar | ||
// results in bar being an empty directory, | ||
// no matter what | ||
newnode = ft.EmptyDirNode() |
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 seems that QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn
is the hash for an empty directory.
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.
Looked at that code and it seems ok. Change few direct calls to use golang interfaces instead, always a good idea when we want to do some refactoring.
This is my first review on go-ipfs, but I thought I could help by reading some code too...
switch ev := err.(type) { | ||
case path.ErrNoLink: | ||
_, rnode, err := i.api.ResolveNode(ctx, rootPath) | ||
if err == coreiface.ErrNotFound { |
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.
When changing \core.Resolveto
api.ResolveNode, there is an extra
api.node.DAG.Get` call hiding behind. Its result is discarded. Could it be possible to reuse it for later?
|
||
tctx, cancel := context.WithTimeout(ctx, time.Minute) | ||
defer cancel() | ||
rootnd, err := i.node.Resolver.DAG.Get(tctx, c) | ||
rootnd, err := i.node.Resolver.DAG.Get(tctx, p.RootCid()) |
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.
Could it be possible to use the result of the previous i.api.ResolveNode(ctx, rootPath)
changed up here which result get discarded?
Thanks @mildred, good points! I'll work on this a bit more. |
Cat(context.Context, Path) (Reader, error) | ||
Ls(context.Context, Path) ([]*Link, error) | ||
Cat(context.Context, Path) (Path, Reader, error) | ||
Ls(context.Context, Path) (Path, []*Link, 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.
As much as I'm not a fan of mutable state, I'd say that we may want to not return resolved paths and just modify the ones passed to the methods - while it makes sense when we implement the API on top of go-ipfs node, doing this in http-api (or in any other wrapper that might be created) imo doesn't, and it would likely require changing the http api to return those resolved paths.
This replaces the gateway's usage of the
path
package with the Core API. The usages ofimporter
,merkledag
, andunixfs
packages will land in another PR in the next couple of days.Note that gateway_handler.go is a huuuuge mess. For now the priority is to extract it into its own package (ipfs/go-ipfs-gateway) and then refactor and cleanup there.