Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

.add/files.add http-api endpoint #323

Closed
wants to merge 10 commits into from
Closed

.add/files.add http-api endpoint #323

wants to merge 10 commits into from

Conversation

nginnever
Copy link
Member

@nginnever nginnever commented Jun 15, 2016

This continues PR

Currently the files add http resource is failing when the js-ipfs daemon is running do to this line in js-ipfs-api.

https://github.com/ipfs/js-ipfs-api/blob/master/src/get-dagnode.js#L30

@noffle might have some ideas on this. It seems that the js-ipfs-api object/data responses don't match when coming from js-ipfs and go-ipfs daemons.

@hackergrrl
Copy link
Contributor

Do you have some failing tests? (Travis only seems to be complaining about standard) How can I repro?

@nginnever
Copy link
Member Author

nginnever commented Jun 15, 2016

@noffle thanks for taking a look!

I've added the http tests with daemon on that should throw an error. I have just been manually trying to add a file with the go/js-ipfs daemons running.

@hackergrrl
Copy link
Contributor

hackergrrl commented Jun 15, 2016

Hey @nginnever, I noticed a couple of things:

  1. It looks like js-ipfs is returning a Buffer for HTTP object/data and go-ipfs returns a stream. We really ought to update js-ipfs and interface-ipfs-core to use streams consistently.
  2. For your HTTP tests: ipfs.files.add returns { path: '...', node: DAGNode }, not { Name: '...', Hash: '...' }

@nginnever
Copy link
Member Author

  1. It looks like js-ipfs is returning a Buffer for HTTP object/data and go-ipfs returns a stream. We really ought to update js-ipfs and interface-ipfs-core to use streams consistently.

My thoughts exactly on the inconsistency in object. I will take a look at those commands and see about returning streams instead of the buffer (which i noticed that buffer isn't even the same data buffer that gets returned from the dag service so not sure what's going on there.)

2

Ahh good catch, still some old tests slipping in before the changes to return type.

@nginnever
Copy link
Member Author

nginnever commented Jun 17, 2016

@noffle I've created a branch on js-ipfs-api that updates get-dagNode.js to check the response from the daemons for buffer or stream.

Though the issue is still as mentioned above that sometimes the response from object/data is outputting a slightly different buffer causing the dag node to multihash to the incorrect value.

If you run the tests now you will see this issue throw an error.

Seems like the core gets the proper data from the dagservice here.

but not when returned here

After some investigating I found that this line in the object resource is the culprit. Removing the toString seems to fix.

This PR is now waiting on

@daviddias
Copy link
Member

@nginnever how is PR coming along?

@daviddias
Copy link
Member

Once #420 is merged, could you rebase this PR @nginnever ?

@daviddias daviddias changed the title WIP add http resource .add/files.add http-api endpoint Aug 20, 2016
@daviddias
Copy link
Member

beep boop, #420 is merged :)

@daviddias
Copy link
Member

beep boop, needing this one for #432 :)

@victorb
Copy link
Member

victorb commented Sep 9, 2016

@daviddias
Copy link
Member

@victorbjelkholm got #323 (comment) released

@dignifiedquire
Copy link
Member

Upgrade and merge conflict resolution happening here: #469

@daviddias
Copy link
Member

Ok, so @victorbjelkholm finished up the integration and got all tests passing for get, cat and add 👏👏👏👏. Thank you @nginnever and @noffle for having kicked off all of this work 🌟 👌🏽

Now it is all about the big rebase (because js-ipfs codebase changed with the migration to pull-streams). This is happening here: #469 by @dignifiedquire

Closing this PR, let's track the rest on the rebase PR (which is already pointing to master) #469

@daviddias daviddias closed this Sep 9, 2016
daviddias added a commit that referenced this pull request Sep 12, 2016
Upgrade the files branch (#323) to work with pull-streams
@daviddias daviddias deleted the add branch December 8, 2016 20:43
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.

6 participants