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.

Sign up for GitHub

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

Jump to bottom

coreapi: Basic object API implementation #4492

Merged
merged 5 commits into from
Jan 30, 2018
Merged

coreapi: Basic object API implementation #4492

merged 5 commits into from
Jan 30, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Dec 14, 2017 •
edited
Loading

TODO:

  • Review
  • Docs
  • Tests

@magik6k magik6k added the topic/core-api Topic core-api label Dec 14, 2017
@magik6k magik6k requested a review from a user December 14, 2017 22:57
@ghost ghost assigned magik6k Dec 14, 2017
@ghost ghost added the status/in-progress In progress label Dec 14, 2017
magik6k
magik6k commented Dec 14, 2017
View reviewed changes
core/coreapi/interface/interface.go Outdated
// DataSize int
// CumulativeSize int
// }
AddLink(ctx context.Context, base Path, name string, child Path, create bool) (Node, error) //TODO: make create optional
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make this optional after the approach to optional params in #4477 is reviewed/accepted

Copy link

Choose a reason for hiding this comment

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

No need for arg names in an interface definition

magik6k
magik6k commented Dec 14, 2017
View reviewed changes
core/coreapi/object.go Outdated
}

func (api *ObjectAPI) Put(context.Context, coreiface.Node) error {
return errors.New("todo") // TODO: what should this method take? Should we just redir to dag-put?f
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not even sure if we want to keep this method, it seems to me that all of it's use cases are covered by Dag.Put

Copy link

Choose a reason for hiding this comment

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

We should still implement it -- it can call DagAPI.Put() underneath

@magik6k magik6k mentioned this pull request Dec 15, 2017
Open
51 tasks
ghost
ghost reviewed Dec 17, 2017
View reviewed changes
core/coreapi/interface/interface.go Outdated
Get(context.Context, Path) (Node, error)
Data(context.Context, Path) (io.Reader, error)
Links(context.Context, Path) ([]*Link, error)
Stat(context.Context, Path) (*ObjectStat, error)
Copy link

@ghost ghost Dec 17, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I've been pondering in #4418 whether we should return a resolved Path in almost any place where we already have it resolved internally anyway. In some cases it saves a ton of additional resolve roundtrips, imagine we use the returned Path to pass it directly into another Core API function.

In addition, this is also useful in the case of functions that modify existing objects, e.g. SetData or AddLink. If I call it with /ipfs/QmFoo/a/b/c, I'd like it to return /ipfs/QmNewHash/a/b/c.

(Regardless of that, Put() should definitely return a Path.)

ghost
ghost reviewed Dec 17, 2017
View reviewed changes
core/coreapi/object.go Outdated
type ObjectAPI CoreAPI

func (api *ObjectAPI) New(ctx context.Context) (coreiface.Node, error) {
node := new(dag.ProtoNode)
Copy link

Choose a reason for hiding this comment

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

On the CLI/API, object new takes a template argument which can be unixfs-dir for a directory. We can do this here similarly to the KeyAPI.WithType() option in #4477.

ghost
ghost reviewed Dec 17, 2017
View reviewed changes
core/coreapi/object.go Outdated
}

func (api *ObjectAPI) AddLink(ctx context.Context, base coreiface.Path, name string, child coreiface.Path, create bool) (coreiface.Node, error) {
rootNd, err := api.core().ResolveNode(ctx, base)
Copy link

Choose a reason for hiding this comment

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

Let's call these variables base instead of root -- the root of /ipfs/QmFoo/a/b would be QmFoo, but the base of this operation is b

ghost
ghost suggested changes Dec 17, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This looks super good :) Left a comment or two. I think that's all the object stuff needed for extracting the gateway, thanks!

@magik6k magik6k force-pushed the feat/coreapi/object branch 2 times, most recently from 29e178a to 2ac9768 Compare January 1, 2018 16:47
magik6k
magik6k commented Jan 5, 2018
View reviewed changes
core/coreapi/interface/interface.go
}

// ObjectStat provides information about dag nodes
type ObjectStat struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I wrap this into an interface for consistency?

Copy link

Choose a reason for hiding this comment

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

I think it's fine - given that the object API is quasi-frozen (it's the old merkledag/dag-pb structure), it's very unlikely this struct will ever have any functions attached to it.

ghost
ghost reviewed Jan 12, 2018
View reviewed changes
core/coreapi/interface/options/object.go

func ObjectNewOptions(opts ...ObjectNewOption) (*ObjectNewSettings, error) {
options := &ObjectNewSettings{
Type: "empty",
Copy link

Choose a reason for hiding this comment

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

The default for this should be "unixfs-dir" as in ipfs object new --help

Copy link

Choose a reason for hiding this comment

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

The default for this should be "unixfs-dir" as in ipfs object new --help

Ah, nevermind, I understand :)

@ghost
Copy link

ghost commented Jan 12, 2018

This LGTM 👍 - pretty cool!

Next let's extract coreiface to its own package, so we can get go-ipfs-api going.

@magik6k
coreapi: Basic object API implementation
529f847 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
coreapi: Object api review
ae949e3 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
coreapi: object docs
64e2327 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
coreapi: implement object.Put
f6663bd 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k
coreapi: object API tests
0377e49 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@whyrusleeping whyrusleeping merged commit 4d8b3c9 into master Jan 30, 2018
@ghost ghost removed the status/in-progress In progress label Jan 30, 2018
@whyrusleeping whyrusleeping deleted the feat/coreapi/object branch January 30, 2018 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

Assignees

@magik6k magik6k

Labels
topic/core-api Topic core-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@magik6k @whyrusleeping