Files API (add, cat, get) tests break go-ipfs 0.4.3 #323
Comments
haadcode
changed the title
|
orbitdb-archive/orbit#40 is blocked due to this bug |
|
Actually only |
|
Okay found the issue why the Running // 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' } ] |
|
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) |
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. |
|
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).
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.
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.
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.
This is what the https://github.com/ipfs/http-api-spec/ effort is for. Help land it. The goals are:
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.
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.
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. |
|
This issue was caused by an ambiguous default value for the 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. |
|
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. |
|
This is exactly the reason why our tests started to fail when being supplied with go-ipfs 0.4.3-rc.1 |
There was a change to the default behaviour of the progress flag, so now we are setting it always to false to ensure it works as intended. Closes #323
|
A fix based on @whyrusleeping description was added in f51f8fb and the whole test suite passes now locally for me with go-ipfs 0.4.3 |
|
@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
|
|
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? |
|
js-ipfs-api should, with #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. |
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. |
make sense, but i'd love having to force everyone in the space to upgrade every client library right now over this :0
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. |
|
It can be also solved in other way: Output of 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. |
|
@whyrusleeping and @diasdavid are telling me that it wasn't NDJSON for long time so my response above isn't really valid. |
|
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. |
|
Did people got my post on: #323 (comment)? Any feedback? |
|
Can this issue be closed? |
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:13The text was updated successfully, but these errors were encountered: