Files API (add, cat, get) tests break go-ipfs 0.4.3

daviddias commented 8 years ago

js-ipfs-api files API (add, cat, get) tests break with the new go-ipfs 0.4.3 release. The error is always the same.

1) .files .add stream:
  Uncaught TypeError: stream.pipe is not a function
    at done (src/get-dagnode.js:30:14)
    at node_modules/async/dist/async.js:3679:13

haadcode commented 8 years ago

https://github.com/haadcode/orbit/issues/40 is blocked due to this bug

dignifiedquire commented 8 years ago

Actually only .add breaks for me, and that is due to object/data breaking.

dignifiedquire commented 8 years ago

Okay found the issue why the object requests are breaking.

Running ipfs add data.txt returns

// go-ipfs 0.4.1
[ { Name: 'data.txt',
    Hash: 'QmVv4Wz46JaZJeH5PMV4LGbRiiMKEmszPYY3g6fjGnVXBS' } ]

// go-ipfs 0.4.3-rc1
[ { Name: 'data.txt', Bytes: 9 },
  { Name: 'data.txt',
    Hash: 'QmVv4Wz46JaZJeH5PMV4LGbRiiMKEmszPYY3g6fjGnVXBS' } ]

jbenet commented 8 years ago

how is this not caught in a go-ipfs test!? o/

dignifiedquire commented 8 years ago

how is this not caught in a go-ipfs test!? o/

Not sure, are there tests for all commands through the http api? (this only happens on the http api not on the cli as far as I can tell)

Kubuxu commented 8 years ago

how is this not caught in a go-ipfs test!? o/

It is very simple, the way that current ipfs-http-api was developed was for internal daemon - CLI communication. It is directly bound to CLI commands so every change there, might introduce changes in ipfs-http-api.

In go-ipfs there are tests for CLI, and few tests for edge cases of the HTTP API but coverage of HTTP API is minimal. In my opinion go-ipfs HTTP daemon - CLI connector should have never be named the ipfs-http-api.

The way it was designed was directly for our CLI implementation, not as HTTP API, resulting in very hard to use API. It is directly bound to go-ipfs CLI which means that we have to be really careful even when fixing smallest bugs in CLI, not only from CLI userspace perspective but also HTTP perspective.

The http-api-spec tries to describe what we call ipfs-http-api but from what I've learned this isn't how API specs should work. If we want to have nice to use and easy to implement bindings for API, it should be in reverse. Spec first, implement after it, repeat for improvements.

This means that there are no hard tests of the API, it also means that we don't really know what is breaking change in this API as set of possible assumptions in all existing right now bindings is uncountable.

I know that API design and implementation is costly (from dev-hours point of view) but I think it is something we should really put pressure on following IPLD deployment.

Sorry if that response is a bit rough, but it is something that I wanted to let go for quite a time.

jbenet commented 8 years ago

Forgive any terseness below, i'm short on time. There's a lot of confusion going on here, and things keep breaking for users. I find this completely unacceptable. Doubly so given that we DO have tests to address this, that either aren't being run on every PR (js-ipfs-api tests need to be run on go-ipfs) or aren't good enough (the http api tests should've caught this too).

It is very simple, the way that current ipfs-http-api was developed was for internal daemon - CLI communication.

Incorrect. it was developed as an HTTP API precisely so that other things, like js-ipfs-api, could use it, and so that the command line client in go could target other daemons.

Either inform yourself better OR qualify your claims. As a go-ipfs developer, you speak with authority. If you're wrong, you're going to confuse the hell out of other people. You are not always right, and particularly in cases like this -- where you make claims about what something complicated is for, it is very, very important to qualify: "i think the ipfs-http-api was developed" reduces the authority claim.

It is directly bound to CLI commands so every change there, might introduce changes in ipfs-http-api.

In go-ipfs there are tests for CLI, and few tests for edge cases of the HTTP API but coverage of HTTP API is minimal. In my opinion go-ipfs HTTP daemon - CLI connector should have never be named the ipfs-http-api.

That is what it is named... it's not "ipfs-http-api", but it is referred to as "the go-ipfs http api" all over the place.

The way it was designed was directly for our CLI implementation, not as HTTP API, resulting in very hard to use API. It is directly bound to go-ipfs CLI which means that we have to be really careful even when fixing smallest bugs in CLI, not only from CLI userspace perspective but also HTTP perspective.

Do not make claims about the way it was designed if you're not 100% sure. "The way it was designed was directly for our CLI implementation, not as HTTP API" is simply not true. It WAS designed to be a standalone API that other things could target. Again, speaking with authority when you're incorrect will confuse the hell out of people, including others in the core team.

It's annoying that I have to step in here to correct this, because others not in to go-ipfs team would just take your word for it blindly. Please be careful with what you say.

The historical reason as to why it's directly coupled to the commands is because we wanted to write one library that could allow us to write commands that would work both in CLI, in HTTP API, and unix domain socket RPC -- making it so we only have to change one thing to mount the thing over another transport.

I think what you want to get at is: "the go-ipfs commands lib is way too complicated and making command changes without breaking the API is difficult. we should improve this, by improving the commands lib and the go-ipfs commands package". This is a very different claim, and one I agree with very much.

The http-api-spec tries to describe what we call ipfs-http-api but from what I've learned this isn't how API specs should work. If we want to have nice to use and easy to implement bindings for API, it should be in reverse. Spec first, implement after it, repeat for improvements.

This is what the https://github.com/ipfs/http-api-spec/ effort is for. Help land it. The goals are:

spec it out independently
write an independent test suite for it
- maybe can start with the sharness HTTP tests in go-ipfs
all implementations (clients and servers) must pass these tests.

The effort was supposed to be done in Q2. It failed to be completed. I'm not sure it will be completed this quarter unless people like you prioritize working on it.

This means that there are no hard tests of the API,

Incorrect, there are a few tests. Certainly not enough, but it's not ok to claim "there are no hard tests". A better claim is "the existing tests are too few and do not test enough cases, hence this breakage happened. we need much stronger tests", which i wholeheartedly agree with. Please be more precise.

Anyway, write more hard tests. It's been on our TODOs for a long time. Prioritize it.

it also means that we don't really know what is breaking change in this API as set of possible assumptions in all existing right now bindings is uncountable.

No, incorrect. the bindings exist, and has been stated MANY times across github that we need to run all bindings (and at the very least js-ipfs-api) before making a release. I have discussed this with others both in github and in person many times. It's even on a release checklist:

https://github.com/ipfs/go-ipfs/blob/master/misc/release-checklist.md#ipfs-release-checklist as "js-ipfs-api tests".

But hey look, you probably didn't see it because there's two go-ipfs release checklists:

Although the recent go-ipfs releases missed updating npm-go-ipfs (on the other checklist), so i'm not even sure you're using either 100%...

They need to be combined into one. Evaluate every step and if you don't think it's necessary, ask me first. Once done, you need to follow every step to completion. This is a hard protocol required for correctness.

Anyway, the ipfs-http-api spec is being developed with the express purpose of knowing what's correct, and "knowing what's a breaking change". It was supposed to be done last quarter. It's not done yet-- so if you want it sooner then put work into it. It's not OK to claim this effort doesn't exist, that there are NO tests when there are some, or this all hasn't been thought out. It has been. And process problems in your release schedule are allowing breakages through. You cannot blame users and force them to upgrade for this.

whyrusleeping commented 8 years ago

This issue was caused by an ambiguous default value for the --progress option on ipfs add. in commit 6f796dc2 it was set to default to true, meaning that we will return progress information for add calls by default, where previously the progress information was default to false unless certain flaky conditions were met (stdin 'file' detection, and the CLI lib making special cased modifications to the outgoing requests, and also some size detection on the input file).

The likely reason this problem was never triggered in the past was because we previously set a 'minimum' file size for which to enable the progress bar. My guess is that js-ipfs-api was never tested with a file size above that limit, and now that we've removed that limit, we're seeing the progress data pop up here.

dignifiedquire commented 8 years ago

Thank you very much @whyrusleeping for clarifying. I think it's important to get this added to the changelog as it is not clear at all from the resulting behaviour what changed.

jbenet commented 8 years ago

@whyrusleeping

great, thanks for looking into it and the precise explanation :heart:
[ ] please add any necessary test cases to go-ipfs to protect against this breakage
[ ] please add js-ipfs-api tests to the go-ipfs test suite. either for every PR or just for releases. Releasing something that accidentally breaks known client libraries -- particularly those we write ourselves -- is not fun. I'm not sure if the test suite right now catches this-- if not, OOOOOF! but let's make sure we run all the tests we can before a release.

@diasdavid @dignifiedquire

[x] can you verify whether the js-ipfs-api tests catch this?
[x] if not, can you add a test to make sure that it does?

dignifiedquire commented 8 years ago

can you verify whether the js-ipfs-api tests catch this?

This is exactly the reason why our tests started to fail when being supplied with go-ipfs 0.4.3-rc.1

dignifiedquire commented 8 years ago

A fix based on @whyrusleeping description was added in https://github.com/ipfs/js-ipfs-api/pull/326/commits/f51f8fb02927e400613579f7a77d9b831a186540 and the whole test suite passes now locally for me with go-ipfs 0.4.3 master

jbenet commented 8 years ago

@dignifiedquire wait, should this be fixed in js-ipfs-api? or should this be fixed in go-ipfs?

i'm not sure having to send {progress: false} makes sense. it seems to me that:

go-ipfs deamon api should default to {progress: false} so that api clients dont all have to say {progress: false} on everything.
the go-ipfs cli client should default ipfs add to {progress: true} when not given user input: ipfs add <file> should show progress by default. this is a bit annoying (having a different default for UX than for API) and im not sure it's a good idea yet. it's what makes for a smoother UX in each case, but not sure it's a smoother UX in totality.

jbenet commented 8 years ago

It may be that the API should have its own set of dafaults, and the CLI should have its own (potentially different) defaults tuned for CLI UX. thoughts?

daviddias commented 8 years ago

js-ipfs-api should, with https://github.com/ipfs/js-ipfs-api/pull/305, follow https://github.com/ipfs/interface-ipfs-core, so anything DX related should be part of the spec + with tests, so that js-ipfs implements them too (and therefore, the http-api in js-ipfs).

The most sane way to achieve this is to have a across impl core-api spec that both js and go agree (with regards to features/ux, each language will have its own way, of course), however requires time for designing, reviewing and migrating.

The other way, which is what we are on track now (less sane, but makes us keep moving forward), is keep pushing a spec definition for each API (https://github.com/ipfs/interface-ipfs-core/pulls), get js-ipfs and js-ipfs-api to behave in the same way and (then), bring features like progress bar or output encoding that go-ipfs has to this land.

dignifiedquire commented 8 years ago

First js-ipfs-api relies on no progress information being present in the way it handles the response currently, so it should be explicit about this (as my patch now is).
We are already breaking user space, so let us break it for the better as @jbenet suggested
- go-ipfs http-api: progress: false as the default
- go-ipfs cli-api: progress: true as the default

While I really like 2 I'm not sure how good it is to have different defaults depending on which api type you use as it could be potentially very confusing. So far we tried to be as consistent as possible here.

jbenet commented 8 years ago

@dignifiedquire

1 ...

make sense, but i'd love having to force everyone in the space to upgrade every client library right now over this :0

2 ...

yeah makes sense.

in the long run,i think it will be ok to have two default sets, we just have to be super clear about that in docs. the more intuitive UX will win.

but if it's too much work now, we can delay that.

Kubuxu commented 8 years ago

It can be also solved in other way:

Output of add endpoint is chunked, meaning it is streaming. This mean that reader should expect NDJSON. This is the case with how add endpoint sends the data:

{ Name: 'data.txt', Bytes: 9 }
{ Name: 'data.txt', Hash: 'QmVv4Wz46JaZJeH5PMV4LGbRiiMKEmszPYY3g6fjGnVXBS' }

js-ipfs-api should respect that and read from the endpoint until it finds required information.

This is how I was taught to use NDJSON.

Kubuxu commented 8 years ago

@whyrusleeping and @diasdavid are telling me that it wasn't NDJSON for long time so my response above isn't really valid.

whyrusleeping commented 8 years ago

No, we need to maintain the pre-existing API expectations. One of the two solutions @dignifiedquire suggested should be our course of action moving forward.

daviddias commented 8 years ago

Did people got my post on: https://github.com/ipfs/js-ipfs-api/issues/323#issuecomment-236579451? Any feedback?

daviddias commented 7 years ago

Can this issue be closed?

ipfs-inactive / js-ipfs-http-client

Files API (add, cat, get) tests break go-ipfs 0.4.3 #323