Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

add support for ipfs files stat --with-local #695

Merged
merged 2 commits into from
Mar 14, 2018

Conversation

MichaelMure
Copy link
Contributor

So, I upgraded my js-ipfs-api and found out that the support for ipfs files stat --with-local was broken after 98000af (the response object is not relayed entirely anymore, so the new values get dropped).

Another problem I had and that is not solved yet by this PR is that due to the migration in go-ipfs to the new go-ipfs-cmds framework (ipfs/kubo#4638), the command ipfs files stat now reply with a stream which is not converted properly in values by js-ipfs-api. There is two way to solve that problem:

  • convert the stream in a single value in js-ipfs-api
  • use cmds.EmitOnce in go-ipfs to hint the ReponseEmitter and reply without streaming

Do you have a preference ?

CC @whyrusleeping, @Stebalien

@MichaelMure
Copy link
Contributor Author

If the best choice is to adjust go-ipfs, it would be nice to choose before the upcoming 0.4.14.

@MichaelMure
Copy link
Contributor Author

Someone to answer my question ?

@daviddias
Copy link
Contributor

after 98000af (the response object is not relayed entirely anymore, so the new values get dropped).

What are the values missing? Mind updating that with the docs + tests on https://github.com/ipfs/interface-ipfs-core ?

the command ipfs files stat now reply with a stream which is not converted properly in values by js-ipfs-api.

The endpoint should be setting the headers so that js-ipfs-api figures that on the fly. Is that not the case?

@MichaelMure
Copy link
Contributor Author

What are the values missing? Mind updating that with the docs + tests on https://github.com/ipfs/interface-ipfs-core ?

New values comes from ipfs/kubo#4638. I will update interface-ipfs-core as well.

The endpoint should be setting the headers so that js-ipfs-api figures that on the fly. Is that not the case?

Actually, I'm not really sure what's going on. This is what I get when I do a request with the last go-ipfs:

image

Here, res is a DestroyableTransform which is part of the through2 npm package. This doesn't happen if cmds.EmitOnce is used in go-ipfs.
go-ipfs current master
js-ipfs-api v17.5.0

@vmx
Copy link
Contributor

vmx commented Feb 26, 2018

@MichaelMure I had a problem which was similar, though after a quick look at the stats source I think I was triggering a different code path. Nonetheless it might be worth to check. Could you set a breakpoint at https://github.com/ipfs/js-ipfs-api/blob/f81dce5bf54df3a3578e4786bfb741d761cd1297/src/utils/send-files-stream.js#L135 and see if you come across it? If yes, then it might be related to #696.

Another hack that worked in my case when I didn't get any value back from a stream was hard-coding the chunkedObjects to false at https://github.com/ipfs/js-ipfs-api/blob/f81dce5bf54df3a3578e4786bfb741d761cd1297/src/utils/send-request.js#L34.

@MichaelMure
Copy link
Contributor Author

The breakpoint at https://github.com/ipfs/js-ipfs-api/blob/f81dce5bf54df3a3578e4786bfb741d761cd1297/src/utils/send-files-stream.js#L135 is not triggered.

Hardcoding chunkedObjects to false indeed solve the problem. It's the same effect as using cmds.EmitOnce in go-ipfs.

The root problem might linked to 210dabb that introduce changes on how data is decoded when chunkedObjects is true.

@vmx
Copy link
Contributor

vmx commented Feb 26, 2018

@pgte There seem to be more issues with streamed responses.

@MichaelMure
Copy link
Contributor Author

@diasdavid does that make sense for you to set the optional values of the http request to default value as I did in this PR ? Or should the keys be missing entirely ?

For reference, the go struct:

type statOutput struct {
	Hash           string
	Size           uint64
	CumulativeSize uint64
	Blocks         int
	Type           string
	WithLocality   bool   `json:",omitempty"`
	Local          bool   `json:",omitempty"`
	SizeLocal      uint64 `json:",omitempty"`
}

@MichaelMure
Copy link
Contributor Author

@diasdavid spec completed here: ipfs-inactive/interface-js-ipfs-core#227
I'll add some test when I'm sure what the interface should look like.

@MichaelMure
Copy link
Contributor Author

PR updated with better default values and a fixed test

@hacdias
Copy link
Contributor

hacdias commented Mar 6, 2018

@MichaelMure the code looks code. Could you fix the tests please?

I see you did it already. I'll check your PR on interface-ipfs-core and see if everything is passing and then I'll leave my review here 😄

@MichaelMure
Copy link
Contributor Author

@hacdias please note that you need go-ipfs master and #696 fixed (I used the cmds.EmitOnce workaround in go-ipfs to have the test pass).

@hacdias
Copy link
Contributor

hacdias commented Mar 6, 2018

@MichaelMure curiously enough, the tests passed here. Anyway, I'm approving this changes because they seem to be correct. We can wait until those issues are also fixed to merge this one.

@MichaelMure
Copy link
Contributor Author

So, huuuu, I actually had trouble when the http reply was a stream because I ... didn't unwrap the stream with streamToValue. So #696 might have been a problem but my code was incorrect as well. Well ...

I still have trouble running the tests properly (lots of unrelated tests are failing), so I'm not sure everything is correct. That said I'm currently using a fork of js-ipfs-api in my project (https://github.com/MichaelMure/js-ipfs-api/tree/fork) and it works properly now.

@daviddias
Copy link
Contributor

daviddias commented Mar 12, 2018

@MichaelMure released interface-ipfs-core with your changes, please rebase master onto this branch and update the interface-ipfs-core dep to 0.56.0.

Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Merging for #713

@daviddias daviddias merged commit b08f21a into ipfs-inactive:master Mar 14, 2018
@ghost ghost removed the ready label Mar 14, 2018
vmx added a commit that referenced this pull request Mar 14, 2018
vmx added a commit that referenced this pull request Mar 14, 2018
vmx added a commit that referenced this pull request Mar 15, 2018
vmx added a commit that referenced this pull request Mar 15, 2018
vmx added a commit that referenced this pull request Mar 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants