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.

Sign up for GitHub

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

Jump to bottom

Fix/compliant writable gw #1917

Closed
wants to merge 1 commit into from
Closed

Fix/compliant writable gw #1917

wants to merge 1 commit into from

Conversation

cryptix
Copy link
Contributor

@cryptix cryptix commented Oct 31, 2015

This is follow up to #1886 (and might have some commits from it until it is merged) rebased.

@ion1 made a fair point here about POST being a better choice for the ipfs object patch functionality of the writable gateway. I whipped up some tests for these cases.

My main open question: How do we want to specify the name to overwrite in the POST /ipfs/QmFoo/... case? I don't like to involve the Content-Disposition: …; filename="…" header. Can't we just use the last part of the request path as that? Also feels like a pain to get right for users.

@jbenet jbenet added the status/in-progress In progress label Oct 31, 2015
@ion1
Copy link

ion1 commented Oct 31, 2015

My main open question: How do we want to specify the name to overwrite in the POST /ipfs/QmFoo/... case? I don't like to involve the Content-Disposition: …; filename="…" header. Can't we just use the last part of the request path as that? Also feels like a pain to get right for users.

There is a bit of a mismatch between the assumptions of the HTTP methods and our assumption of content-addressed and immutable paths. However, the RFC text on POST seems much more flexible about the exact semantics than the text on PUT (which says “A successful PUT of a given representation would suggest that a subsequent GET on that same target resource will result in an equivalent representation being sent in a 200 (OK) response”).

The usual assumption about POST /resource is that the /resource already exists and the POST will create a subresource under it. If /ipfs/QmBar/baz/file does not exist, it would be surprising if there was a resource at that path that responds to POST. Normally one would POST to the existing resource /ipfs/QmBar/baz and specify the filename of the subresource to be created using a means that is not the path.

On the other hand, perhaps we could read the RFC liberally and interpret “The POST method requests that the target resource process the representation enclosed in the request according to the resource's own specific semantics” with the assumption that given an existing /ipfs/QmBar, as far as POST is concerned, there is a resource at all possible paths under it such that POST /ipfs/QmBar/baz/file patches QmBar and responds with 201 Created, Location: /ipfs/QmQuux/baz/file.

This would certainly be much nicer for everyone than having to handle a special Content-Disposition header in the payload.

@cryptix cryptix force-pushed the fix/compliantWritableGW branch from 70b14ec to 3c3683a Compare October 31, 2015 12:23
@cryptix cryptix mentioned this pull request Nov 2, 2015
Closed
47 tasks
@cryptix cryptix force-pushed the fix/compliantWritableGW branch from 3c3683a to 385749e Compare November 3, 2015 09:57
@cryptix
Copy link
Contributor Author

cryptix commented Nov 3, 2015

Note to self: keep an eye on ipfs/infra#105 for relevant parts.

@cryptix cryptix mentioned this pull request Nov 3, 2015
Closed
@jbenet
Copy link
Member

jbenet commented Nov 16, 2015

Was this waiting on input from me?

@cryptix
Copy link
Contributor Author

cryptix commented Nov 16, 2015

@jbenet no, sorry. I'll try to get this done this week.

@cryptix cryptix force-pushed the fix/compliantWritableGW branch from aa7b637 to 120f500 Compare December 2, 2015 16:45
@cryptix
Copy link
Contributor Author

cryptix commented Dec 2, 2015

rebased, tests still broken.

@cryptix cryptix force-pushed the fix/compliantWritableGW branch from eb204f6 to b342035 Compare December 2, 2015 16:53
@cryptix
Copy link
Contributor Author

cryptix commented Dec 2, 2015

Tests should be fixed now. RFCR.

@cryptix cryptix added the RFCR label Dec 2, 2015
@cryptix
Copy link
Contributor Author

cryptix commented Dec 2, 2015

Forgot about the sharness tests - can't run them on my bsd office box right now, fill fix them later at home

@cryptix cryptix removed the RFCR label Dec 2, 2015
ghost
ghost reviewed Dec 3, 2015
View reviewed changes
core/corehttp/gateway_handler.go
return
case "PUT":
i.putHandler(w, r)
// TODO(cryptix): where are the docs?
http.Error(w, "writableGateway: PUT method not meaningful on IPFS - use POST and see the docs", http.StatusMethodNotAllowed)
Copy link

Choose a reason for hiding this comment

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

Can just remove the PUT-case and let it fall through to the method error in lines 81 ff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m) thanks! :)

@ghost
Copy link

ghost commented Dec 3, 2015

From ipfs/infra#105

@jbenet: need a way to force --pin=false for all ops on the gateway.

How would you like to set this? Config option oder flag on 'ipfs daemon' ?

Just unconditionally have the gateway not pin -- we can add an option for implicit pinning later (I don't think we need it).

@cryptix cryptix force-pushed the fix/compliantWritableGW branch from 105aeb7 to 7d45f21 Compare December 3, 2015 00:51
@cryptix
Copy link
Contributor Author

cryptix commented Dec 3, 2015

Just unconditionally have the gateway not pin.

Thanks for that insight and the review!

cryptix added a commit that referenced this pull request Dec 3, 2015
@cryptix
tests: fixing sporadic failure in t0060-daemon.sh
f7fb258 
daemon failed to startup really often for me, increased pollEndpoint
timeout. (updates #1917)

also fixes a small printf bug in pollEndpoint.

License: MIT
Signed-off-by: Henry <cryptix@riseup.net>
@jbenet
Copy link
Member

jbenet commented Dec 6, 2015

Just unconditionally have the gateway not pin -- we can add an option for implicit pinning later (I don't think we need it).

ok sgtm

@jbenet
Copy link
Member

jbenet commented Dec 6, 2015

could other people CR this too?

@whyrusleeping whyrusleeping self-assigned this Dec 6, 2015
@ghost ghost added the topic/gateway Topic gateway label Dec 7, 2015
@ghost ghost mentioned this pull request Dec 7, 2015
Closed
@cryptix
writable gateway: added tests from @ion1's RFC summary
0b202cd 
#1886 (comment)

License: MIT
Signed-off-by: Henry <cryptix@riseup.net>
@cryptix cryptix force-pushed the fix/compliantWritableGW branch from ede0559 to 0b202cd Compare December 15, 2015 10:49
@cryptix
Copy link
Contributor Author

cryptix commented Dec 15, 2015

rebased and now apparently broken.. not sure what changed under this.

@jbenet
Copy link
Member

jbenet commented Dec 16, 2015

:( --

@whyrusleeping
Copy link
Member

just looks like your code is returning 405's for a bunch of things...

@cryptix
Copy link
Contributor Author

cryptix commented Dec 16, 2015

I know - it passed the last time I worked on it before the rebase. Need to take a log of the source subtree and see what was introduce.

Sent from my iPhone

On 16.12.2015, at 10:18, Jeromy Johnson notifications@github.com wrote:

just looks like your code is returning 405's for a bunch of things...

—
Reply to this email directly or view it on GitHub.

@whyrusleeping
Copy link
Member

@cryptix do you have time to take another look at this?

@cryptix
Copy link
Contributor Author

cryptix commented Jan 29, 2016

Actually, Yup. I fear the rebase, though. I might need to nag somebody but will try.

On 29.01.2016, at 18:40, Jeromy Johnson notifications@github.com wrote:

@cryptix do you have time to take another look at this?

—
Reply to this email directly or view it on GitHub.

@cryptix
Copy link
Contributor Author

cryptix commented Feb 11, 2016

@whyrusleeping I tried but I have no idea what is going on with the code changes. Hope somebody else can pick this up.

@cryptix cryptix closed this Feb 11, 2016
@Kubuxu Kubuxu deleted the fix/compliantWritableGW branch February 27, 2017 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@whyrusleeping whyrusleeping

Labels
status/in-progress In progress topic/gateway Topic gateway
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cryptix @ion1 @jbenet @whyrusleeping