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

Add Group add #17

Merged
merged 1 commit into from
Jan 21, 2016
Merged

Add Group add #17

merged 1 commit into from
Jan 21, 2016

Conversation

RichardLitt
Copy link
Contributor

This is the chunk for ipfs add. Note that there are a few outstanding issues before this can be added to the API:

  • Parameters have not been adequately tested with the API, and I suspect none of them work.
  • add seems to me to be a POST request, but I'm not sure how to verify this, as curl -F "file=test" http://localhost:5001/api/v0/add seems to work just fine without needing -x POST.
  • I don't know if I should include the HTTP 100 status code as a response; I think not, as it is is just a continuation code. Can I have a second, on this?
  • The header Transfer-encoding is relayed twice on a successful response. This is invalid.
  • I am unable to add a directory recursively. Is this functionality possible?

I have included a curl request with each request. This will be the format for PRs from now on, so everyone please take a look and let me know what you think. I think that PRs like this - adding bit by bit to the API, as we go along - is the right way to do things. At any point, we can merge this and add it, as accurately reflects the API. However, I think we should try and address any issues first if we can.

cc @whyrusleeping @diasdavid @dignifiedquire @jbenet

@jbenet
Copy link
Contributor

jbenet commented Jan 8, 2016

this is looking good-- @dignifiedquire @diasdavid will be best people to comment on this for now. i dont have much bandwidth to take a deep look atm

@RichardLitt
Copy link
Contributor Author

Sounds good. I'll ping them as this progresses.

@dignifiedquire
Copy link
Collaborator

I will try to take some time for CR tomorrow and give some feedback

@RichardLitt
Copy link
Contributor Author

Thanks @dignifiedquire

@daviddias
Copy link
Contributor

Parameters have not been adequately tested with the API, and I suspect none of them work.

At least -r and -w work well because we use it all the time. We should add tests for all of the params on js-ipfs-api and on the go-ipfs tests

add seems to me to be a POST request, but I'm not sure how to verify this, as curl -F "file=test" http://localhost:5001/api/v0/add seems to work just fine without needing -x POST.

This is because currently the http API doesn't care which method it receives the call, it will accept any, which is not the right behaviour, @whyrusleeping agrees . Referenced here:

I don't know if I should include the HTTP 100 status code as a response; I think not, as it is is just a continuation code. Can I have a second, on this?

I would not expect to see it on the spec as well, we can add if someone asks :)

The header Transfer-encoding is relayed twice on a successful response. This is invalid.

Relayed? Could you show an example response

I am unable to add a directory recursively. Is this functionality possible?

How are you doing it? Curl is not going to create a HTTP Multipart message for you (that I know of or at least without a special option passed)

@@ -4,7 +4,107 @@ FORMAT: 1A

The API for interacting with IPFS nodes.

# Group add
# Group add [POST /add{?arg}{&recursive,quiet,silent,progress,trickle,only%2Dhash,wrap%2Dwith%2Ddirectory,hidden,chunker,pin}]
Copy link
Contributor

Choose a reason for hiding this comment

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

agree that add should be locked to be only a POST method

I believe the API is only expecting the aliases for the flags though (recursive -> r, quiet -> q, etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be much in favor of dropping support for the shorthands like r and q as they are just very cryptic and q has a different meaning in most http based APIs as query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree with this. Flags are not the best way to send arguments to an HTTP API.

@dignifiedquire
Copy link
Collaborator

don't know if I should include the HTTP 100 status code as a response; I think not, as it is is just a continuation code. Can I have a second, on this?

I wonder why they actually happen, seems unnecessary to me. But yes they should not be part of the spec.

@daviddias
Copy link
Contributor

@RichardLitt can we break the apiary API docs description using the same categories found on https://github.com/ipfs/specs/blob/ipfs/api/api/core/README.md ?

@RichardLitt
Copy link
Contributor Author

Lots to cover here. I'm breaking it down into sections.

100 Status Code

I removed the 100 Status code from the Headers for the 200 Response, where it should not be anyway, but where I included it for this PR. Instead of having a 100 Response section, I have added a note to the 200 Response section pointing out that 100 will be passed.

POST/GET Methods

I have added a note at the very top level that all are currently functioning, and added a link to the go-ipfs discussion issue (ipfs/kubo#2165) for now. This note will need to be removed when we fix this.

Flags

I opened ipfs/kubo#2189:

According to feedback given in #17, the API uses flags such as r and w instead of recursive and wrap-with-directory. I do not understand the logic behind this. I suggest that we move the API to either accept aliases, or to use the full names. It is awkward, documenting and using the API the way it currently exists.

Tests

@diasdavid:

We should add tests for all of the params on js-ipfs-api and on the go-ipfs tests

Is this documented, anywhere? What would be the best workflow for adding these or documenting their existence?

Multiple Headers

I have included responses which show the multiple headers. I have logged an issue here: ipfs/kubo#2190.

Recursive add

@diasdavid:

How are you doing it? Curl is not going to create a HTTP Multipart message for you (that I know of or at least without a special option passed)

I was trying using Postman, and curl. Unable to get either working. That may have been because I didn't know I needed to pass -r option as r and not recursive. How would you send this?

Mime type of request

I opened #21 to try and figure this out.

ipfs add moving to ipfs files add

We'll change this when it happens.

Multipart response

@diasdavid says that it won't work with curl. What do we want to log here, then?

Categorization

@diasdavid:

@RichardLitt can we break the apiary API docs description using the same categories found on https://github.com/ipfs/specs/blob/ipfs/api/api/core/README.md ?

Unrelated to ipfs add. Please see: #22

@RichardLitt
Copy link
Contributor Author

At what point should we merge this, and open new PRs to point out issues in the current API that need to be fixed? This PR is holding up the state of the current API by trying to conform to an ideal that doesn't exist.

@dignifiedquire
Copy link
Collaborator

Re tests: We should setup dredd for CI in go-ipfs and drakov for CI in js-ipfs-api I would say

@dignifiedquire
Copy link
Collaborator

Re Multipart, these docs are not restricted to curl in any form, you can see a multipart example for apiblueprint here

@daviddias
Copy link
Contributor

@RichardLitt answering below:

Tests
@diasdavid:
We should add tests for all of the params on js-ipfs-api and on the go-ipfs tests
Is this documented, anywhere? What would be the best workflow for adding these or documenting their existence?

Every option param for any CLI (which maps the API) has a help menu with the available arguments, example:

» ipfs add --help

    ipfs add <path>... - Add an object to ipfs.

ARGUMENTS:

    <path>... - The path to a file to be added to IPFS

OPTIONS:

    -r, --recursive           bool   - Add directory paths recursively
    -q, --quiet               bool   - Write minimal output
    -p, --progress            bool   - Stream progress data
    -t, --trickle             bool   - Use trickle-dag format for dag generation
    -n, --only-hash           bool   - Only chunk and hash - do not write to disk
    -w, --wrap-with-directory bool   - Wrap files with a directory object
    -H, --hidden              bool   - Include files that are hidden
    -s, --chunker             string - chunking algorithm to use

But as you can see, we don't cover them all in our tests at js-ipfs-api https://github.com/ipfs/js-ipfs-api/blob/master/test/api/add.spec.js, it would be great if we had. It would help to have a issue on js-ipfs-api listing all the params that still need to be tested, so that other members on the IPFS community can find items where they can contribute that are fairly easy to get done and that can create a great impact on how well IPFS is tested :) Would you be able to document/write that issue?

Recursive add
@diasdavid:
How are you doing it? Curl is not going to create a HTTP Multipart message for you (that I know of or at least without a special option passed)
I was trying using Postman, and curl. Unable to get either working. That may have been because I didn't know I needed to pass -r option as r and not recursive. How would you send this?

I tried to find a way to do it with curl, but since our multipart messages are custom (meaning that there is no nesting), what I would recommend is to capture the messages sent and received from the API when this js-ipfs-api test is run: https://github.com/ipfs/js-ipfs-api/blob/master/test/api/add.spec.js#L83-L96

Multipart response
@diasdavid says that it won't work with curl. What do we want to log here, then.

See above

@RichardLitt
Copy link
Contributor Author

  • Parameters have not been adequately tested with the API, and I suspect none of them work. I know how this works now. See spec.
  • ~~add seems to me to be a POST request, but I'm not sure how to verify this, as curl -F "file=test" http://localhost:5001/api/v0/add seems to work just fine without needing -x POST. ~~ Getting this using waveshark.
  • ~~I don't know if I should include the HTTP 100 status code as a response; I think not, as it is is just a continuation code. Can I have a second, on this? ~~ Not including it.
  • The header Transfer-encoding is relayed twice on a successful response. This is invalid. issue is open.
  • ~~I am unable to add a directory recursively. Is this functionality possible? ~~ Not possible in curl, which was my problem. Added a generic one using waveshark.

@RichardLitt RichardLitt reopened this Jan 21, 2016
RichardLitt added a commit that referenced this pull request Jan 21, 2016
@RichardLitt RichardLitt merged commit 849c065 into master Jan 21, 2016
@daviddias daviddias deleted the feature/add-add branch January 21, 2016 14:21
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