Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer of object.patch.set-data in http API #2070

Closed
ianopolous opened this issue Dec 14, 2015 · 23 comments
Closed

Transfer of object.patch.set-data in http API #2070

ianopolous opened this issue Dec 14, 2015 · 23 comments
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@ianopolous
Copy link
Member

The data in object.patch.set-data is stored in the URL. This is quite limiting if I want to put arbitrary binary data there (esp if I want ~512kb). Would putting the data in the request body or a multi-part make sense?

@whyrusleeping
Copy link
Member

the input for set data is passed via stdin (aka, the request body)

@ianopolous
Copy link
Member Author

In 0.3.11 it only works as an extra argument in the URL.

@whyrusleeping
Copy link
Member

mmmm, no. You definitely can pass file data:

$ ipfs object patch `ipfs object new` set-data < somefile

@whyrusleeping
Copy link
Member

unless something really weird is happening and turning that into a url parameter... have you tried it from the cli and checked what request it makes?

@whyrusleeping
Copy link
Member

oh weird:

cmds.StringArg("args", true, true, "extra arguments").EnableStdin(),

So it can take file input, but apparently the file gets read in and turned into a normal 'string' argument...
thats kindof annoying

@ianopolous
Copy link
Member Author

Yep, at the moment this is the only thing stopping us fully using IPFS in Peergos.

@jbenet
Copy link
Member

jbenet commented Dec 15, 2015

@whyrusleeping can we make this a cmds.FileArg ?

@whyrusleeping
Copy link
Member

@jbenet its not quite that simple unfortunately... we need to find a way to support arbitrary position arguments between subcommands: CMD SUB <arg> SUB <arg>

@whyrusleeping
Copy link
Member

dammit. we've been able to do this the whole time.

@whyrusleeping
Copy link
Member

@ianopolous should be fixed now on dev0.4.0, confirm?

@ianopolous
Copy link
Member Author

Hmm, I get a 400 on dev0.4.0 for add-link:
Server returned HTTP response code: 400 for URL: http://127.0.0.1:5001/api/v0/object/patch?arg=QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n&arg=add-link&arg=somelinkname&arg=QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n

I get a similar 400 for the set-data post as well.

@whyrusleeping
Copy link
Member

hrm... let me try a few things...

@whyrusleeping
Copy link
Member

@ianopolous are you using the js-ipfs-api?

@ianopolous
Copy link
Member Author

Nope, the Java one, but I haven't pushed the changes to make the set-data use the http body..

@whyrusleeping
Copy link
Member

@ianopolous alright, you'll have to make a change to the way your url is constructed. A valid one from the CLI looks like:

/api/v0/object/patch/add-link?api=%2Fip4%2F127.0.0.1%2Ftcp%2F5005&arg=QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn&arg=test&arg=QmQMx3hDUQqgCRi9kXBKLYDcJj3Q213o5jm91ZiKdcDLNj&encoding=json&stream-channels=true

Note that 'add-link' is no longer an argument, but one of the commands.

@ianopolous
Copy link
Member Author

Okay, that's fixed add-link to work again, but set-data still 400s: e.g.
http://127.0.0.1:5001/api/v0/object/patch/set-data?arg=QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n
with a non empty body of some random bytes.

@whyrusleeping
Copy link
Member

@ianopolous check the http trailers sent on the request, they should give you error messages (if you don't get anything useful from the body). The data is expected to be multipart to mesh easily with everything else that we use multipart for.

On a related note, are there any tests for the java api bindings that we could run in CI? It would be nice if we could see when changes we make here break code that depends on us.

@ianopolous
Copy link
Member Author

Ah I didn't realise it had to be multipart. That'll do it.

There are indeed tests, run using "make tests" in the root, obviously requiring the jdk to be installed

@whyrusleeping
Copy link
Member

@ianopolous cool. We will discuss automated testing of libraries that are dependant on our code during the infrastructure sprint this week. cc @lgierth @RichardLitt

@ianopolous
Copy link
Member Author

I've switched it to multipart, but now it gets a 500, and the Trailer response header says: X-Stream-Error
The response body is empty. The Multipart works fine for other commands like block.put

@ianopolous
Copy link
Member Author

@em-ly em-ly added the kind/enhancement A net-new feature or improvement to an existing feature label Aug 25, 2016
@whyrusleeping
Copy link
Member

@ianopolous is this still an issue?

@ianopolous
Copy link
Member Author

@whyrusleeping Not sure why this is still open. You fixed it during that chat we had ages ago. All is good now. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants