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

Refactoring towards gateway extraction #4418

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Nov 22, 2017

This replaces the gateway's usage of the path package with the Core API. The usages of importer, merkledag, and unixfs 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.

Lars Gierth added 2 commits November 22, 2017 04:21
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost added topic/cleanup Topic cleanup topic/core-api Topic core-api labels Nov 22, 2017
@ghost ghost self-assigned this Nov 22, 2017
@ghost ghost added the status/in-progress In progress label Nov 22, 2017
@ghost
Copy link
Author

ghost commented Nov 22, 2017

I'll fix the codeclimate errors

@whyrusleeping
Copy link
Member

@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 :)

@ghost ghost mentioned this pull request Dec 17, 2017
3 tasks
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()
Copy link
Contributor

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.

Copy link
Contributor

@mildred mildred left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

When changing \core.Resolvetoapi.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())
Copy link
Contributor

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?

@ghost
Copy link
Author

ghost commented Jan 12, 2018

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)
Copy link
Member

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.

@ghost ghost closed this Oct 5, 2018
@ghost ghost removed the status/in-progress In progress label Oct 5, 2018
@ghost ghost deleted the feat/coreapi-gateway branch January 20, 2019 19:00
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/cleanup Topic cleanup topic/core-api Topic core-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants