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

[daviddias]

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]

orbitdb-archive/orbit#40 is blocked due to this bug

[dignifiedquire]

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

[dignifiedquire]

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]

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

[dignifiedquire]

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]

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]

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:

• https://github.com/ipfs/go-ipfs/blob/master/misc/release-checklist.md#ipfs-release-checklist
• https://github.com/ipfs/go-ipfs/blob/master/docs/releases.md

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]

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]

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.