fixing putHandler #1886

cryptix · 2015-10-22T23:36:46Z

trying to finally get a working putHandler for the writable http gateway.

License: MIT
Signed-off-by: Henry [email protected]

cryptix · 2015-10-23T00:34:53Z

Since #1845 is based on 0.4, I tried my best to get the putHandler back to work.

I fixed overwriting an existing path and 'object add-link' but I'm not sure how in line this are with the old sharness tests and or if we still wont their behavior.

cryptix · 2015-10-23T00:35:05Z

cc @jbenet @whyrusleeping

rht · 2015-10-23T03:44:48Z

core/corehttp/gateway_handler.go

-		webError(w, "Could not resolve root object", err, http.StatusBadRequest)
-		return
-	}
+		createfunc := func() *dag.Node {


This is uio.NewEmptyDirectory

createfunc := uio.NewEmptyDirectory

rht · 2015-10-23T03:45:50Z

Since netstat is not available in the test env, the first test should be commented out.

whyrusleeping · 2015-10-23T12:17:04Z

core/corehttp/gateway_handler.go

-	if err != nil {
-		webError(w, "Could not recursively add new node", err, http.StatusInternalServerError)
+func (i *gatewayHandler) putHandler(w http.ResponseWriter, r *http.Request) {
+	rootPath := path.Path(r.URL.Path)


could probably use path.Parse here

whyrusleeping · 2015-10-23T18:08:02Z

core/corehttp/gateway_handler.go

-		return
+	var newPath string
+	if len(rsegs) > 1 {
+		newPath = filepath.Join(rsegs[2:]...)


we dont want to use filepath here, on windows systems this would put forward slashes in the ipfspaths, which i dont think works like we expect it to.

ugh, true.. I always manage to ignore that.

whyrusleeping · 2015-10-23T18:39:16Z

just a few comments, then squash some commits so none of them break. (i think some of the earlier ones break) then LGTM

cryptix · 2015-10-23T18:51:45Z

CR adressed and squashed.

ion1 · 2015-10-23T19:00:38Z

Please correct me if I have misunderstood the RFC text, but it seems to me like the methods should have the following semantics in this context:

POST /ipfs creates a new resource under /ipfs whose name (in this case: hash) is determined by the gateway and receives the response:

201 Created
Location: /ipfs/QmFoo

POST /ipfs/QmBar or POST /ipfs/QmBar/baz request a resource to be added under QmBar and QmBar/baz respectively. The server has to choose the name of the resource; we should dictate that the request must have a Content-Disposition: …; filename="…" header and use that for the name.

Technically the resulting resource will be added under a new /ipfs/QmQuux which is the result of patching QmBar but this might not be too great a sin. The response to POST /ipfs/QmBar/baz with filename="my-file":

201 Created
Location: /ipfs/QmQuux/baz/my-file

We will have to decide what to do if the request would overwrite an existing file in QmBar. The choices:

Just overwrite it and give a 201 Created response as above. Any qualms about POST apparently deleting content may be soothed by realizing that the original QmBar is still there with the original file. This would be my choice.
Give a 409 Conflict response and require the client to DELETE the original file first. This increases complexity, requires two round trips and results in a useless intermediate object being created.

PUT /ipfs is not meaningful (“I expect a future GET /ipfs to return this content”). 405 Method Not Allowed.

PUT /ipfs/QmFoo can only succeed if QmFoo is in fact the hash of the object being uploaded (requiring the client to compute the hash in advance). Either don’t support this and respond with 405 Method Not Allowed or check whether the hash matches and respond with 201 Created or 409 Conflict.

PUT /ipfs/QmBar/baz is not meaningful: the gateway might not know anything about QmBar under which baz is requested to be created, and if it does know, QmBar already exists and is immutable. 405 Method Not Allowed.

This can not be used to patch QmBar: a successful PUT to a resource means that a subsequent GET to the same resource will return the uploaded content, actual mutation of the resource needs to occur.

DELETE /ipfs is not meaningful. 405 Method Not Allowed.

DELETE /ipfs/QmFoo is not meaningful. 405 Method Not Allowed.

DELETE /ipfs/QmQuux/baz/my-file can be used to patch QmQuux to remove my-file. The RFC only says a successful DELETE “SHOULD” give a 202 Accepted/204 No Content/200 OK response. We should not be in violation if we create a patched object and respond with:

201 Created
Location: /ipfs/QmBar/baz

rht · 2015-10-23T21:29:26Z

You're suggesting PUT should be used only for mutable (ipns) case?

ion1 · 2015-10-23T21:38:51Z

PUT could be used with IPNS, yes. The rule seems to be:

After a successful PUT <location>, a subsequent GET <location> will return what you put.

This implies mutation.

mildred · 2015-10-27T07:38:17Z

UpdateNodeLink() in merkledag/node.go can probably be removed as part of this PR. gateway_handler was the only one using it.

Also, I'm generally liking PUT much more than POST when it can be used, but I agree that in the case of ipfs, resources are immutable and PUT doesn't makes sense except at the root when the client already knows the hash of the node it uploads. POST with Content-Disposition should probably be implemented instead.

jbenet · 2015-10-27T10:08:09Z

core/corehttp/gateway_handler.go

-	w.Header().Set("IPFS-Hash", key.String())
-	http.Redirect(w, r, ipfsPathPrefix+key.String()+"/"+strings.Join(components, "/"), http.StatusCreated)
+	w.Header().Set("IPFS-Hash", newkey.String())
+	http.Redirect(w, r, filepath.Join(ipfsPathPrefix, newkey.String(), newPath), http.StatusCreated)


jbenet · 2015-10-27T10:11:13Z

The PR is hard to follow because changes change code quite a bit. i haven't read it all. but if LGTWhyrusleeping, and the tests pass, i'd be ok to merge it.

(in the end i want to split out all the gateway stuff into own repo)

i think ion covered it very well, all that SGTM.

cryptix · 2015-10-27T10:56:53Z

@jbenet it's easier to follow if you just look at the new code.

Do we want to merge it now and do the change to POST in a follow up PR? The changes will only get bigger because the tests need to change as well. OTOH we would introduce features again that aren't there to stay..

I'd also like to introduce pure go tests for the things @ion1 brought up. I think it's easier to outline and test all those in a table based manner.

whyrusleeping · 2015-10-27T16:20:07Z

I would be comfortable merging this as its something that is disabled by default.

I disabled this a long time ago and never refactored it. About time. License: MIT Signed-off-by: Henry <[email protected]>

cryptix · 2015-10-29T11:41:56Z

rebased.

I'll start a new branch based on this one for the POST conversion, if we want to do it in this PR I can still push it here.

rht · 2015-10-29T14:15:15Z

core/corehttp/gateway_handler.go

-	}
+	var newkey key.Key
+	rnode, err := core.Resolve(i.node.Context(), i.node, rootPath)
+	switch ev := err.(type) {


This is a switch from err casting.
This is cleaner, and should be used across the code.

In current master, where cast is used:

$ grep -rE 'err\.\(' . | grep -v Godeps ./routing/dht/ext_test.go: if merr, ok := err.(u.MultiErr); ok && len(merr) > 0 { ./routing/dht/ext_test.go: if merr, ok := err.(u.MultiErr); ok && len(merr) > 0 { ./routing/dht/ext_test.go: if merr, ok := err.(u.MultiErr); ok && len(merr) > 0 { ./core/corehttp/gateway_handler.go: if _, ok := err.(path.ErrNoLink); ok { ./core/corehttp/gateway_handler.go: if _, ok := err.(path.ErrNoLink); ok { ./core/commands/add.go: if _, ok := err.(*hiddenFileError); ok { ./p2p/net/conn/dial_test.go: if ne, ok := err.(net.Error); ok && ne.Temporary() { ./p2p/net/conn/dial.go: if nerr, ok := err.(net.Error); ok && nerr.Timeout() { ./p2p/net/conn/dial.go: errno, ok := err.(syscall.Errno) ./cmd/ipfs/main.go: switch e := err.(type) {

rht · 2015-10-29T14:29:06Z

Unless I'm missing something, to implement POST for composite files on 0.3.x, doesn't it mean the code for ipfs add is duplicated almost verbatim?

rht · 2015-10-29T14:35:38Z

How is @ion1's PUT /ipfs/QmBar/baz case addressed in test/sharness/t0111-gateway-writeable.sh#L60 ? The new hash isn't precomputed in the test.

jbenet · 2015-10-31T00:24:20Z

core/corehttp/gateway_handler.go

@@ -273,116 +275,82 @@ func (i *gatewayHandler) postHandler(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, ipfsPathPrefix+k.String(), http.StatusCreated)
 }

-func (i *gatewayHandler) putEmptyDirHandler(w http.ResponseWriter, r *http.Request) {


what happened to this part? do empty dirs still work?

yup - works by virtue of core.Resolve and friends iirc. There is also a sharness test that checks against the hash.

jbenet · 2015-10-31T00:25:56Z

@cryptix a lot of people want this. eta on addressing CR?

License: MIT Signed-off-by: Henry <[email protected]>

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

cryptix · 2015-10-31T02:31:55Z

@jbenet done :)

jbenet · 2015-10-31T05:03:31Z

Thanks @cryptix ! LGTM. I'll merge this for now -- so @victorbjelkholm can use in IPFSBin and @cloutier and @didiercf can use in ipfspics. we can move from PUT to POST in the future.

fixing putHandler

rht · 2015-10-31T07:00:04Z

And ipfs-wiki, which uses python -m SimpleHTTPServer.

jbenet · 2015-10-31T07:10:34Z

cc @jamescarlyle o/

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

fixing putHandler This commit was moved from ipfs/kubo@f703b2c

jbenet added the status/in-progress In progress label Oct 22, 2015

rht reviewed Oct 23, 2015
View reviewed changes

cryptix force-pushed the fix/ThatPut branch from 6ad9e9b to abea118 Compare October 23, 2015 08:51

whyrusleeping reviewed Oct 23, 2015
View reviewed changes

cryptix force-pushed the fix/ThatPut branch from eb3d2aa to 5b47bb4 Compare October 23, 2015 15:45

whyrusleeping reviewed Oct 23, 2015
View reviewed changes

cryptix force-pushed the fix/ThatPut branch 2 times, most recently from 51cbc49 to cfc0824 Compare October 23, 2015 18:51

mildred mentioned this pull request Oct 27, 2015

Remove usage of merkledag.Link.Node pointer outside of merkledag #1902

Closed

jbenet reviewed Oct 27, 2015
View reviewed changes

fixing putHandler for --writable http gateway

b8b4e45

I disabled this a long time ago and never refactored it. About time. License: MIT Signed-off-by: Henry <[email protected]>

cryptix force-pushed the fix/ThatPut branch from cfc0824 to 0b1572a Compare October 29, 2015 11:39

rht reviewed Oct 29, 2015
View reviewed changes

jbenet reviewed Oct 31, 2015
View reviewed changes

jbenet mentioned this pull request Oct 31, 2015

Webapp work on top of IPFS api directly victorb/ipfsbin#42

Open

putHandler: addressed CR from @jbenet

c1847e9

License: MIT Signed-off-by: Henry <[email protected]>

cryptix added a commit that referenced this pull request Oct 31, 2015

writable gateway: added tests from @ion1's RFC summary

70b14ec

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

cryptix mentioned this pull request Oct 31, 2015

Fix/compliant writable gw #1917

Closed

cryptix force-pushed the fix/ThatPut branch from 0b1572a to 273670a Compare October 31, 2015 02:31

cryptix force-pushed the fix/ThatPut branch from 273670a to c1847e9 Compare October 31, 2015 02:36

cryptix changed the title ~~WIP: fixing putHandler~~ fixing putHandler Oct 31, 2015

jbenet added a commit that referenced this pull request Oct 31, 2015

Merge pull request #1886 from ipfs/fix/ThatPut

f703b2c

fixing putHandler

jbenet merged commit f703b2c into master Oct 31, 2015

jbenet removed the status/in-progress In progress label Oct 31, 2015

jbenet deleted the fix/ThatPut branch October 31, 2015 05:03

cryptix added a commit that referenced this pull request Oct 31, 2015

writable gateway: added tests from @ion1's RFC summary

3c3683a

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

cryptix added a commit that referenced this pull request Nov 3, 2015

writable gateway: added tests from @ion1's RFC summary

385749e

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

cryptix added a commit that referenced this pull request Dec 2, 2015

writable gateway: added tests from @ion1's RFC summary

c2fce54

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

cryptix added a commit that referenced this pull request Dec 15, 2015

writable gateway: added tests from @ion1's RFC summary

0b202cd

#1886 (comment) License: MIT Signed-off-by: Henry <[email protected]>

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#1886 from ipfs/fix/ThatPut

69e0d64

fixing putHandler This commit was moved from ipfs/kubo@f703b2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing putHandler #1886

fixing putHandler #1886

cryptix commented Oct 22, 2015

cryptix commented Oct 23, 2015

cryptix commented Oct 23, 2015

rht Oct 23, 2015

whyrusleeping Oct 23, 2015

rht commented Oct 23, 2015

whyrusleeping Oct 23, 2015

whyrusleeping Oct 23, 2015

cryptix Oct 23, 2015

whyrusleeping commented Oct 23, 2015

cryptix commented Oct 23, 2015

ion1 commented Oct 23, 2015

rht commented Oct 23, 2015

ion1 commented Oct 23, 2015

mildred commented Oct 27, 2015

jbenet Oct 27, 2015

jbenet commented Oct 27, 2015

cryptix commented Oct 27, 2015

whyrusleeping commented Oct 27, 2015

cryptix commented Oct 29, 2015

rht Oct 29, 2015

rht commented Oct 29, 2015

rht commented Oct 29, 2015

jbenet Oct 31, 2015

cryptix Oct 31, 2015

jbenet commented Oct 31, 2015

cryptix commented Oct 31, 2015

jbenet commented Oct 31, 2015

rht commented Oct 31, 2015

jbenet commented Oct 31, 2015

fixing putHandler #1886

fixing putHandler #1886

Conversation

cryptix commented Oct 22, 2015

cryptix commented Oct 23, 2015

cryptix commented Oct 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rht commented Oct 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Oct 23, 2015

cryptix commented Oct 23, 2015

ion1 commented Oct 23, 2015

rht commented Oct 23, 2015

ion1 commented Oct 23, 2015

mildred commented Oct 27, 2015

Choose a reason for hiding this comment

jbenet commented Oct 27, 2015

cryptix commented Oct 27, 2015

whyrusleeping commented Oct 27, 2015

cryptix commented Oct 29, 2015

Choose a reason for hiding this comment

rht commented Oct 29, 2015

rht commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Oct 31, 2015

cryptix commented Oct 31, 2015

jbenet commented Oct 31, 2015

rht commented Oct 31, 2015

jbenet commented Oct 31, 2015