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

Core API refinements and efficiency improvements #3493

Merged
merged 2 commits into from
Mar 22, 2017

Conversation

ghost
Copy link

@ghost ghost commented Dec 9, 2016

cc @hsanjuan @keks @kevina @Kubuxu

Closes #3490

commit e1c190a
Author: Lars Gierth larsg@systemli.org
Date: Wed Nov 16 06:21:15 2016 +0100

coreapi: smarter way of dealing with the different APIs

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>

commit 2e15ba9
Author: Lars Gierth larsg@systemli.org
Date: Fri Dec 9 16:35:28 2016 +0100

coreapi: be more flexible about inputs

While coreapi functions previously only accepted path strings,
they now accept anything from Path structs and strings,
Multihash structs and strings, and most notably: Cid structs,
strings and byte arrays. See go-cid.Parse() for more information.

If the argument is a string containing at least one forward-slash,
it's treated as a path and resolved accordingly, i.e. including
IPNS resolution and link traversal.

Anything else will be parsed as a Cid and fetched directly using DAG.Get().

This change makes various use cases of the Core API more efficient,
since there's no more need for expensive string/Path/Cid conversions.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>

commit 7241c02
Author: Lars Gierth larsg@systemli.org
Date: Fri Dec 9 16:49:24 2016 +0100

gateway: make serving index.html a little more efficient

Prevents unneccessary conversions and lookups :):)

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>

@ghost ghost added topic/core-api Topic core-api topic/gateway Topic gateway labels Dec 9, 2016
@ghost ghost added this to the ipfs 0.4.5 milestone Dec 9, 2016
@ghost ghost added status/in-progress In progress need/review Needs a review labels Dec 9, 2016
// ID() CoreID
// Version() CoreVersion
// }
type Ref interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I object to such type for API methods parameters. See my comment in #3490 .

@ghost ghost mentioned this pull request Dec 10, 2016
@whyrusleeping
Copy link
Member

blocked on #3490

@whyrusleeping whyrusleeping added the status/blocked Unable to be worked further until needs are met label Dec 14, 2016
@ghost ghost force-pushed the feat/coreapi-refinements branch from 7241c02 to c6dc74b Compare December 16, 2016 21:36
@Kubuxu Kubuxu modified the milestones: ipfs 0.4.6, ipfs 0.4.5 Jan 10, 2017
@whyrusleeping
Copy link
Member

@lgierth progress here? I think its probably safe to move this to the 0.4.7 milestone for now?

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.7, ipfs 0.4.6 Feb 14, 2017
@ghost
Copy link
Author

ghost commented Feb 17, 2017

progress here? I think its probably safe to move this to the 0.4.7 milestone for now?

Yes good to move, it needs the two changes we discussed but which I haven't posted here yet (will dig them up).

@whyrusleeping
Copy link
Member

@lgierth any progress here since last update?

@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.8, Ipfs 0.4.7 Mar 11, 2017
@ghost
Copy link
Author

ghost commented Mar 11, 2017

will take care of this first thing in the docs sprint, so we can get it into 0.4.8

Lars Gierth added 2 commits March 17, 2017 02:35
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
The new coreiface.Path maps a path to the cid.Cid
resulting from a full path resolution.

The path is internally represented as a go-ipfs/path.Path,
but that doesn't matter to the outside.

Apart from the path-to-CID mapping, it also aims to hold all
resolved segment CIDs of the path. Right now it only exposes
Root(), and only for flat paths a la /ipfs/Qmfoo. In other cases,
the root is nil.

In the future, resolution will internally use
go-ipfs/path.Resolver.ResolvePathComponents and thus always return
the proper resolved segments, via Root(), or a future Segments() func.

- Add coreiface.Path with Cid() and Root().
- Add CoreAPI.ResolvePath() for getting a coreiface.Path.
- All functions now expect and return coreiface.Path.
- Add ParsePath() and ParseCid() for constructing a coreiface.Path.
- Add coreiface.Node and Link which are simply go-ipld-node.Node and Link.
- Add CoreAPI.ResolveNode() for getting a Node from a Path.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost force-pushed the feat/coreapi-refinements branch from 963a812 to ee45b8d Compare March 17, 2017 02:58
@ghost
Copy link
Author

ghost commented Mar 17, 2017

coreapi: make the interfaces path centric

The new coreiface.Path maps a path to the cid.Cid
resulting from a full path resolution.

The path is internally represented as a go-ipfs/path.Path,
but that doesn't matter to the outside.

Apart from the path-to-CID mapping, it also aims to hold all
resolved segment CIDs of the path. Right now it only exposes
Root(), and only for flat paths a la /ipfs/Qmfoo. In other cases,
the root is nil.

In the future, resolution will internally use
go-ipfs/path.Resolver.ResolvePathComponents and thus always return
the proper resolved segments, via Root(), or a future Segments() func.

  • Add coreiface.Path with Cid() and Root().
  • Add CoreAPI.ResolvePath() for getting a coreiface.Path.
  • All functions now expect and return coreiface.Path.
  • Add ParsePath() and ParseCid() for constructing a coreiface.Path.
  • Add coreiface.Node and Link which are simply go-ipld-node.Node and Link.
  • Add CoreAPI.ResolveNode() for getting a Node from a Path.

@ghost ghost requested review from whyrusleeping and Kubuxu March 20, 2017 18:16
Add(context.Context, io.Reader) (*cid.Cid, error)
Cat(context.Context, string) (Reader, error)
Ls(context.Context, string) ([]*Link, error)
Add(context.Context, io.Reader) (Path, error)
Copy link
Member

Choose a reason for hiding this comment

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

Hrm... this one feels weird. I like that Cat takes a path and returns a reader, and that Add is the inverse operation, but it just feels weird that Add returns a path...

I think i'll get over it. The Path in question can be thought of as /ipfs/Cid

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks @lgierth :)

We're this much closer to having the gateway extracted!

@whyrusleeping whyrusleeping merged commit 7dee31b into master Mar 22, 2017
@whyrusleeping whyrusleeping deleted the feat/coreapi-refinements branch March 22, 2017 01:57
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Mar 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review status/blocked Unable to be worked further until needs are met topic/core-api Topic core-api topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core API needs to take flexible inputs
3 participants