Fix the PUT-existing bug in writable gateway API

[jes]

See on https://discuss.ipfs.io/t/writeable-http-gateways/210

Previously, a PUT to a filename that already exists would return the hash
of the newly-inserted file instead of the hash of the new tree. This is not
useful, and it makes much more sense for the API to be consistent.

Furthermore, anybody who knows about the bug is already DELETE-ing the filename
before PUT-ing the new content, so the fix doesn't trample over that behaviour
either.

With this fix, every conceivable usage of the API is either unaffected or
improved.

It could do with a unit test.

---

All I did was remove the special-case for the "file already exists" case, and it works correctly. It's easier to see this if you view git diff -w.

[jes]

continuous-integration/jenkins/pr-merge — This commit cannot be built

Not sure if I'm expected to fix this as part of this pull request, but from what sense I can make of the output it looks like the "cannot be built" refers to unit tests failing, which are nothing to do with this commit.

Am I reading it wrong?

[whyrusleeping]

@jes the tests are broken because you made the gateway default to be writeable

[whyrusleeping]

I'm fine with the fixes, but making writeable the default needs to be considered separately.

[jes]

The tests were failing before I added the second commit. I've removed it now, but I expect the tests will still fail. I looked at the Jenkins output and it doesn't seem to be anything I've done, they're annotated "known breakage".

[Kubuxu]

Test that failed:

[go test] PUT http://localhost:42613/ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn/
[go test] putHandler: InsertNodeAtPath failed: cannot create link with no name!
[go test] * Hostname was NOT found in DNS cache
[go test] *   Trying ::1...
[go test] * connect to ::1 port 42613 failed: Connection refused
[go test] *   Trying 127.0.0.1...
[go test] * Connected to localhost (127.0.0.1) port 42613 (#0)
[go test] > PUT /ipfs/QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn/ HTTP/1.1
[go test] > User-Agent: curl/7.38.0
[go test] > Host: localhost:42613
[go test] > Accept: */*
[go test] > 
[go test] < HTTP/1.1 500 Internal Server Error
[go test] < Date: Sun, 21 May 2017 09:51:39 GMT
[go test] < Content-Length: 70
[go test] < Content-Type: text/plain; charset=utf-8
[go test] < 
[go test] { [data not shown]
[go test] * Connection #0 to host localhost left intact
[go test] not ok 29 - HTTP PUT empty directory
[go test] #	
[go test] #	  URL="http://localhost:$port/ipfs/$HASH_EMPTY_DIR/" &&
[go test] #	  echo "PUT $URL" &&
[go test] #	  curl -svX PUT "$URL" 2>curl_putEmpty.out &&
[go test] #	  cat curl_putEmpty.out &&
[go test] #	  grep "Ipfs-Hash: $HASH_EMPTY_DIR" curl_putEmpty.out &&
[go test] #	  grep "Location: /ipfs/$HASH_EMPTY_DIR" curl_putEmpty.out &&
[go test] #	  grep "HTTP/1.1 201 Created" curl_putEmpty.out
[go test] #	

[jes]

Thanks @Kubuxu.

Things we should clarify:

What should PUT /ipfs/Qm.../directory-name/ do in the event that the directory already exists? What about when it does not already exist?

What should PUT /ipfs/Qm.../ do? The unit test seems to check that it returns the same hash, but it's not obvious to me why that's even a valid request to make.

---

I think PUT to a name ending in a / should leave the directory alone if it already exists, and create an empty directory if not. But then what would the request body be good for? Would it just be ignored? The code in these commits makes it create a file called "directory-name", which doesn't seem right.

[Tangent128]

Is it practical to define PUT-ing to a path ending in a / as an error for now, actually? My rationale:

• it shouldn't create a file, since that feels "surprising" (the server either makes up a name or sticks the file where you claimed a directory should be)
• it shouldn't create a directory (since the uploaded content can't mean anything)
• current usecases for the gateway (HTML applications, basically) don't have access to machine-readable directory listings, so the ability to create empty directories is not especially useful yet.
• it's easier to error now and define semantics later rather than define poor semantics now and have to support them forever. (but if anything in service already depends on the current behavior, that's a problem with this approach)

Longer-term evolution would then look like:

• an interface to PUT a link to an existing hash is desirable; at that point you could create empty directories by PUT-ting a link to the empty directory. This probably still shouldn't use the /, actually.

[jes]

Another point:

what should happen if /ipfs/Qm.../directory-name/ already exists and you PUT /ipfs/Qm.../directory-name, without the trailing slash?

Error? Remove the directory and replace it with a file?

When I get time I'll document the existing behaviour in all of these cases and write up what I think they should do. I think @Tangent128 is probably right, PUT'ing a directory should be an error.

[Tangent128]

Although Unix semantics would say that replacing a directory with a file should be an error, I don't see a harm in the PUT file overwriting the directory link in this case.

Philosophically, because the Unix error is kinda arbitrary and itself surprising.

Practically, because AFAIK implementing that check would require fetching the node in question to see if it's a file or directory, demanding an otherwise unnecessary search for the block.

[whyrusleeping]

@jes @Tangent128 What are the open questions here? I'm not super familiar with REST semantics and implications/best practices, so i don't have too many opinions on this PR. That said, i'd like to get this fixed so the cool apps that are being written to use this can get better and better :)

[jes]

I think @Tangent128 has answered all the policy questions. Just a case of documenting what the policy is, writing unit tests, and implementing the policy. I'll try and do it tomorrow.

[jes]

The policy can be defined simply as:

• A path ending in a / is an error, always (will need to remove the unit test that PUTs to the empty hash); this behaviour can easily be changed in the future as nobody will be using it
• A filename that does not already exist is created and populated with the request body
• A filename that already exists (even if it is currently a directory) is replaced with a file populated with the request body

[jes]

Seems to be broken in Jenkins because the build machine is out of disk space?

[whyrusleeping]

@jes hrm... thats no good.

@victorbjelkholm how fix?

[whyrusleeping]

@jes it looks like some of the sharness tests are actually failing

[whyrusleeping]

Youve got some hardcoded logfile paths in the sharness tests

[jes]

Failing test:

not ok 9 - daemon no longer running
#	
#		test_expect_code  1 kill -0 $IPFS_PID
#	

Not quite sure how my changes have caused this. I'll be able to have a look when I'm back in ~8 days.

[jes]

That test is still failing after I merged in the latest.

It passes on my laptop so not sure what is wrong.

[whyrusleeping]

@jes could you rebase this on master and force push it? I think its a residual bug from when you branched off.

[whyrusleeping]

you set IPFS_HASH two different places here

[whyrusleeping]

code looks good to me, and CI is much happier after the rebase. I would prefer having the sharness tests broken up into a few more smaller checks, that makes it easier to actually see whats going wrong when things break.

Also pointed out youre reusing a variable in one test without checking it.

[Kubuxu]

code SGTM, feeling the same as @whyrusleeping about the tests

[Stebalien]

Dammit! I pushed the wrong rebase.

[Stebalien]

Apologies. Moved to: #5942.