Replace coreunix.Add with shell/fsnode's AddFromReader

[wking]

coreunix.Add is basically a wrapper around BuildDagFromReader that
returns a string-ified key instead of a DAG node. Since we'll need
the DAG node to add to the directory, converting to a key and then
looking that key back up in dirbuilder.AddChild is just extra work.

In #1127, @whyrusleeping mentioned that the file- and
directory-addition API needed some polishing, and this WIP PR is my
attempt to grind things down to a more compact API. I'll probably
just work up to removing coreunix.Add completely, and then start a new
PR for whatever function we want to cull next.

[jbenet]

all the text above sgtm, except:

I'll probably just work up to removing coreunix.Add completely

why remove coreunix.Add?

i want every function in the "API" to be in all, {cli, lib, http api}. coreunix is part of the (currently very unpolished) lib api, but it should remain to mirror things.

the reason for mirroring the api exactly is ease of use. having a consistent api across all {cli, http api, and lib} will make using IPFS to build things very easy. we can certainly have more in the lib, and guide users to use other functions, but every part of the api should exist.

I'll try to synthesize a generalized API spec next week

[whyrusleeping]

@jbenet I gotta be straight with you, i'm not a huge fan of the whole coreX.Function stuff... its a lot of thin wrappers over things that require running the daemon in your app to even make use of it. I think that the shell API is the way to go for making a consistent, easy to use API for users in Go

[jbenet]

I just want the API to be implemented in the interface, shell or core doesn't really matter.

Also, everything in the shell will have to be in the core one way or another. The shell is a wrapper that can RPC over to the daemon. It has to be implemented somewhere. 

And one shell implementation may just be direct calls to the core, if you DO happen to be running an embedded node in your app. That is very much a use case. Think sqllite, leveldb, apps that build use IPFS as a database and don't want to require installing a node in the OS.

[wking]

On Fri, Apr 24, 2015 at 09:24:40PM -0700, Juan Batiz-Benet wrote:

I'll probably just work up to removing coreunix.Add completely

why remove coreunix.Add?

i want every function in the "API" to be in all, {cli, lib, http
api}. coreunix is part of the (currently very unpolished) lib api,
but it should remain to mirror things.

the reason for mirroring the api exactly is ease of use. having a
consistent api across all {cli, http api, and lib} will make using
IPFS to build things very easy. we can certainly have more in the
lib, and guide users to use other functions, but every part of the
api should exist.

And I'm fine with that. It just seems to me that importer is
currently where reader → file-node lives. I don't want to mirror that
or write a big stack of wrappers on top of it. We currently use
BuildDagFromReader in coreunix.Add(), coreunix.add(), and
commands.add(). I want:

lib location → use in command-line or HTTP-endpoint implementation

not the current:

lib location → coreunix wrapper → command wrapper → use in
command-line or HTTP-endpoint implementation

If you want the lib location to be in coreunix, or lib/ipfs/unix or
wherever and just want the chunking algorithms in ‘importer’, that's
fine with me. But I think BuildDagFromReader is closer to the …err…
building-a-DAG-node-from-a-reader signature than the current
collection of add wrappers ;).

I'll try to synthesize a generalized API spec next week

Sounds good. In the interim, I think removing any extra layers of
thin wrappers makes sense, and it will make it easier to move things
over to the generalized API once we've decided what that will look
like ;).

[whyrusleeping]

I think removing any extra layers of thin wrappers makes sense
👍

[jbenet]

The creation of this function is a good addition, let's keep this.

[jbenet]

I very much appreciate the effort to remove unnecessary code, and unnecessary symbols. In general, I'm all for that. And thanks for taking the time to look through this case.

But in this case, I'm strongly against this change. It increases (not reduces) the complexity for us, and for end users. When i want to add an io.Reader to my node, i really don't want to have to know what a dagservice, pinning, and chunking all are. (of course, i may need to, if i want to do something fancy, like use my own chunker, but sane defaults should be trivial to use). This removal also increases the potential for errors / bug surface area.

---

I'm ok with the testutil.AddCat abstraction, as that removes boilerplate test code and reduces the surface area of what could go wrong.

[jbenet]

I know this is not documented anywhere, please bear with me as I make all this clearer through specs.

I don't really care whether it's MyFunc(nd, ...) or ndinterface.MyFunc(...). What i care about is there being a standard interface for everything a node does, as simple as possible for the end user. this is a UX thing. these thin wrappers here are really beneficial to end users who do not know much about how the system works, and do not want to sink knee-deep into it to be able to use it. ipfs add file is enough in the terminal. so should something like ipfs.Add(reader) in Go.

For now (and until the shell interface appears), please look at the core* functions as a (messy) interface that defines the entire functionality of an IPFS node, in Go. An end user who embeds IPFS into her application should be able to use all the core* functions which will be familiar from the cli + api.

---

@whyrusleeping

its a lot of thin wrappers over things that require running the daemon in your app to even make use of it

Yeah, that's fine. the ability to embed an IPFS node and use it directly from within an application (as you would an HTTP client or server) is very much a target use case. Not all IPFS nodes will be bulky, os-level services. Many will be ephemeral nodes created inside apps. (and yeah, the shell interface -- when it arrives -- will allow embedding only a proxy object and RPCing out to use it. ideally we will use the shell interface for the directly embedded case too, so the interface is the exact same, but until then, core* is what we have)

[wking]

On Mon, Apr 27, 2015 at 02:29:16AM -0700, Juan Batiz-Benet wrote:

What i care about is there being a standard interface for everything
a node does, as simple as possible for the end user. this is a UX
thing.

I agree that having a simple UX is really important. I just think
having a single interface to perform a task (e.g. creating a file node
from a reader) is going to be simpler than having a range of similar
functions for performing that task with varying numbers of knobs.
Reading a brief docs saying “if you don't have a specific chunker in
mind, use chunk.DefaultSplitter()” and similarly for the other options
seems easier from a UX perspective than “select the reader-based-adder
from this list that best matches your desired signature”.

these thin wrappers here are really beneficial to end users who do
not know much about how the system works, and do not want to sink
knee-deep into it to be able to use it. ipfs add file is enough in
the terminal. so should something like ipfs.Add(reader) in Go.

Folks using a terminal or an HTTP API will have a different interface
because there's a limit to what you can fit down a socket (e.g. data
is easy, methods are not). I agree that the Go API should be
familiar to folks who already know the command line or HTTP
interface, but I think forcing an exact match (e.g. returning the key
string from Add instead of the file node from which the key string can
be extracted, but which also supports other actions) just makes the
library interface more complicated. Because some folks will need
access to the file node, and that means either a parallel function
with a slightly different syntax, or they'll need a workaround like
dirbuilder.AddChild to recover the node from the key (see d77b90c).

Also, adding a file from a reader is going to get more complicated as
the amount of per-file metadata we track increases (access modes,
owner and group IDs, modification times?, …). I think you can make a
much stronger case for the current coreunix.AddR than for the current
coreunix.Add. Adding files from a reader (vs. from a path) seems
peripheral/advanced enough that it doesn't need an entry-level
wrapper, even if you feel like entry-level wrappers are a useful tool.

Anyhow, I think we have the same end goal in mind (providing an
accessible API to library consumers), we just have different views on
what would be most accessible. If we have the dev time and clear
separation, I'm ok with having parallel low- and high-level APIs, but
until we have a clear and stable low-level API I don't think it's
worth maintaining sugar-coated wrappers on top, when a few lines of
documentation seem to address your “how do I use this” concerns ;).
But if you're not buying what I'm selling, I'm happy to rebase that
change to the end of my branch :p.

[jbenet]

having a single interface to perform a task (e.g. creating a file node
from a reader) is going to be simpler than having a range of similar
functions for performing that task with varying numbers of knobs.

agreed, which is why i prefer a simple:

Add(io.Reader)

which will cover most of users' first steps into using this.

I think forcing an exact match (e.g. returning the key
string from Add instead of the file node from which the key string can
be extracted, but which also supports other actions) just makes the
library interface more complicated.

changing the signature from:

// from
func Add(...) Key

// to
func Add(...) Node

is fine with me. what isn't fine is adding lots of knowledge to the user (dagservice, pinning, chunker).

We already ask users to understand a lot of stuff in the model. complicating it even further with more implemenetation-specific knowledge is not good.

Also, adding a file from a reader is going to get more complicated as
the amount of per-file metadata we track increases (access modes,
owner and group IDs, modification times?, …).

That's fine, all of those can be more advanced function calls that do ask the user to understand them. What i'm rejecting here is removing the simplest possible use case, which most users will fall into at the very beginning.

Adding files from a reader (vs. from a path) seems
peripheral/advanced enough that it doesn't need an entry-level
wrapper, even if you feel like entry-level wrappers are a useful tool.

Not really, this is what I use, and what other people use when they first try to add data with Go. It's very natural -- both from an idiomatic Go perspective (manipulating io.Readers) and from what we established with the cli (ipfs add <file>)

If we have the dev time and clear separation, I'm ok with having parallel low- and high-level APIs, but until we have a clear and stable low-level API I don't think it's worth maintaining sugar-coated wrappers on top

It takes waaay more time explaining to users on irc what the more complicated things mean, and how to use them. and those are just the users brave enough to ask. most give up on the spot, and i've seen it first hand. At this point i've run multiple "classes" with 30+ people hacking with ipfs. I've observed people trying to grasp the large number of concepts when their goal is a simple "i want to add this data into ipfs".

I'm interested in dropping the barriers of entry as much as possible, and this requires having extremely simple interfaces that do the simplest possible thing -- the average use case -- by default. Advanced use should be optional, not forced.

when a few lines of documentation seem to address your “how do I use this” concerns

The problem is not 5 sentences of doc. the problem is 5 sentences here, 5 sentences there, and very soon we're at 1000s of sentences of doc. we already ask waaaay too much from users. ipfs must be extremely simple to use, and leveraging the analogues we establish in the cli is critical to this.

take a look at Go's net and http packages. they hide away a ton of complexity for the common case. the more advanced use cases can do things by dropping down into lower levels. crystal clear, unencumbered interfaces are really valuable to users new to IPFS, new to Go, new to content addressing in general.... Compare this to asking users to enter:

ipfs add <file>
ipfs add <dagservice> <pinner> <splitter> <file>

[wking]

On Mon, Apr 27, 2015 at 01:53:49PM -0700, Juan Batiz-Benet wrote:

Adding files from a reader (vs. from a path) seems
peripheral/advanced enough that it doesn't need an entry-level
wrapper, even if you feel like entry-level wrappers are a useful
tool.

Not really, this is what I use, and what other people use when they
first try to add data with Go. It's very natural -- both from an
idiomatic Go perspective (manipulating io.Readers) and from what we
established with the cli (ipfs add <file>)

No, that's adding a file from a path (where coreunix.AddR is what you
want). I like that API a lot more, because paths also give you access
to the access mode, owner ID, …. What coreunix.Add is like is adding
from stdin:

$ ipfs add </path/to/file

and we don't have a command-line interface to do that.

I'm interested in dropping the barriers of entry as much as
possible, and this requires having extremely simple interfaces
that do the simplest possible thing -- the average use case -- by
default. Advanced use should be optional, not forced.

Ok, and on IRC you suggested to levels of API:

• core = kernel. it defines that an IPFS node is.
• shell = a wrapper around the core that makes the api very simple +
  clean for consumers, and also makes it possible to use across rpc.

So having high-level wrappers around adding and accessing files in
shell/fsnode.go works for me. And maybe moving the lower-level
BuildDagFromReader to AddFromReader core/fsnode would fill the
low-level API slot.

take a look at Go's net and http packages. they hide away a ton of
complexity for the common case. the more advanced use cases can do
things by dropping down into lower levels. crystal clear,
unencumbered interfaces are really valuable to users new to IPFS,
new to Go, new to content addressing in general.... Compare this to
asking users to enter:

ipfs add <file>
ipfs add <dagservice> <pinner> <splitter> <file>

The command line is a lot easier here, because you can have optional
arguments with sane defaults (not possible without hoop-jumping in a
single Go function). I'm +1 on:

$ ipfs add [options]

with options like:

--dagservice address of the IPFS node to use. Defaults to
manipulating your local ~/.ipfs directory directly.
--pinner pinner to use. Select from …. Defaults to
recursively pinning each added object.
--splitter algorithm for chunking files into content-addressable
blocks. Select from 'rabin' or 'size'. Defaults to
'size'. For details on the options, use:

               $ godoc github.com/ipfs/go-ipfs/importer/chunk

Of course, the command line UI is going to be less flexible about
output types (you can't just return a dag.Node), but it is nice to
have the flexibility of options that newcomers can ignore until they
really *need to use Rabin chunking (or whatever). There's no need
for separate high-level and low-level interfaces, but you may want to
separate your options into high-level and low-level options.

[jbenet]

ipfs add </path/to/file

sure

and we don't have a command-line interface to do that.

what do you mean? we do. ipfs add </path/to/file works just fine. try it:

> echo "hello wking" | ipfs add
added QmciRKQJNeejQ8qT8mNgCuDXGmY7VfMfREHWDFyu6M8aK9

not possible without hoop-jumping in a single Go function)

it's ok to have multiple functions. i prefer this. but the simplest, most straightforward case that users will want to try first should be trivially easy to spot and use. hence Add(io.Reader). (I think AddReader(io.Reader) would be fine as well, but not straying further than that)

[wking]

On Mon, Apr 27, 2015 at 03:58:27PM -0700, Juan Batiz-Benet wrote:

and we don't have a command-line interface to do that.

what do you mean? we do. ipfs add </path/to/file works just
fine. try it:

> echo "hello wking" | ipfs add
added QmciRKQJNeejQ8qT8mNgCuDXGmY7VfMfREHWDFyu6M8aK9

Ahh, good stuff. Docs updated in #1151. I guess we can pull the
intended user/group from the process's effective user/group and umask,
so that's not too bad (but does mean the local client has to do more
legwork to setup a call to an IPFS daemon).

[jbenet]

we can pull the intended user/group from the process's effective user/group and umask, so that's not too bad (but does mean the local client has to do more legwork to setup a call to an IPFS daemon).

yeah, sounds good.

i wonder if this should be an option. for example, most cases when publishing to the web, i do not want ipfs to grab my user or permissions, only the content. something like --keep-permissions or something.

[jbenet]

(though it doesnt matter much either way)

[wking]

On Mon, Apr 27, 2015 at 04:33:23PM -0700, Juan Batiz-Benet wrote:

i wonder if this should be an option. for example, most cases when
publishing to the web, i do not want ipfs to grab my user or
permissions, only the content. something like --keep-permissions
or something.

So “please don't wrap this file with a metadata node”, which makes it
easy to create the file, and could be interpreted on unpacking as “use
the current user/group, consult the current umask for the access mode,
use the current time for atime/mtime, …).

Do you want that boolean flag to be part of the high-level
add-from-reader signature?

func Add(node _core.IpfsNode, reader io.Reader, metadata bool) (_dag.Node, err)

where ‘metadata == true’ means “wrap the top-level file node with
mtetadata based on the current environment” and ‘metadata == false’
means “just return the top-level file node”.

Or should the high-level signature just assume we don't want to guess
at metadata and use unwrapped files?

[jbenet]

Or should the high-level signature just assume we don't want to guess at metadata and use unwrapped files?

i think this is right. we could have a set of functions that use metadata. it's an additional concept people need to know and want. its possible it'll be common enough to warrant being the default, but im not convinced of that yet.

[wking]

I'm still running through the local tests, but I think this is ready
to land. I've shifted coreunix.Add to shell/fsnode's AddFromReader
and return a dag.Node there instead of a stringified key. I've also
added a deprecation warning to coreunix.Add following #1159.

core/corehttp/gateway_test.go would be a bit simpler if it could use
fsnode.AddFromReader instead of importer.BuildDagFromReader 1, but
we can't do that without either externalizing the test from the
corehttp package or breaking our anti-cyclic-import hierarchy (#1158).
Personally, using importer.BuildDagFromReader directly from this test
seems like the simplest choice, but I'm happy to externalize the test
if that's the only thing holding up this PR.

[jbenet]

we're not currently doing descriptions like this. seems very nice and useful for consumers.

the binary's changelog is not the best place. this changelog is just a tiny summary. maybe we can have a different file for things like this.

we should define clearly what interfaces we commit to as stable. in particular:

• core
• shell

come to mind as the main ones. changes in these are potentially impactful.

[wking]

On Wed, Apr 29, 2015 at 01:26:36PM -0700, Juan Batiz-Benet wrote:

the binary's changelog is not the best place. this changelog is just
a tiny summary. maybe we can have a different file for things like
this.

Git uses a directory of per-release notes 1. I'm happy to put this
in docs/release-notes/unreleased.md (to be moved to
docs/release-notes/0.3.4.md when you cut that release) if that sounds
reasonable to you.

[jbenet]

sure, sounds good.

[whyrusleeping]

@wking whats the status here?

[whyrusleeping]

closing, old PR cleanup time.