gateway: use core api for serving GET/HEAD/POST #3244

ghost · 2016-09-20T02:59:12Z

Basing this PR on the feat/coreapi branch just so the diff is more obvious.

whyrusleeping

One comment, then LGTM

whyrusleeping · 2016-09-23T05:14:09Z

core/corehttp/gateway_handler.go

 	defer func() {
 		if r := recover(); r != nil {
 			log.Error("A panic occurred in the gateway handler!")
 			log.Error(r)
 			debug.PrintStack()
+			cancel()


no need for the cancel here. Its deferred as well and will be run on a panic

Updated (note: this PR depends on #3207, and conflicts with #2971)

whyrusleeping · 2016-09-23T05:14:18Z

core/corehttp/gateway_handler.go

 		}
 	}()

 	if i.config.Writable {
 		switch r.Method {
 		case "POST":
-			i.postHandler(w, r)
+			i.postHandler(ctx, w, r)


whyrusleeping · 2016-09-30T01:11:56Z

@lgierth progress here? tests seem unhappy, i'd rebase them on master

ghost · 2016-10-28T16:23:12Z

I've rebased this PR back on master and closed the other one. Waiting for tests.

whyrusleeping · 2016-10-28T21:55:24Z

core/coreapi/interface/interface.go

+// 	Version() CoreVersion
+// }
+
+type Link struct {


could this just rebind ipldnode.Link? (fine if not for now, but maybe later?)

could this just rebind ipldnode.Link? (fine if not for now, but maybe later?)

updated

whyrusleeping

One small nitpick, then LGTM

whyrusleeping · 2016-10-28T21:56:46Z

core/corehttp/gateway_handler.go

-	if !ok {
-		webError(w, "Cannot read non protobuf nodes through gateway", dag.ErrNotProtobuf, http.StatusBadRequest)
-		return
+	} else {


drop the else

One of the conditionals above doesn't return (else if err == coreiface.ErrIsDir), so we'd be calling nil.Close(). This piece first tries a reader, then lists links if it's a directory.

Oooooooh, okay. Thats mildly confusing then... But i guess it works.

whyrusleeping · 2016-10-30T18:53:10Z

core/coreapi/unixfs.go

+		return nil, err
+	}
+
+	_, ok := dagnode.(*mdag.ProtoNode)


no longer need this cast. NewDagReader takes the ipldnode.Node interface

whyrusleeping · 2016-10-30T18:53:35Z

core/coreapi/unixfs.go

+}
+
+func (api *UnixfsAPI) Add(ctx context.Context, r io.Reader) (*cid.Cid, error) {
+	k, err := coreunix.AddWithContext(ctx, api.node, r)


maybe AddWithContent should return a cid? not sure

Agreed, but not in this PR. Happy to convert the unixfs package to using CIDs in another PR.

Agreed, but not in this PR. Happy to convert the unixfs package to using CIDs in another PR.

Eh I meant coreunix. Maybe we can just rip coreunix out.

whyrusleeping · 2016-10-30T19:01:05Z

core/corehttp/gateway_handler.go

+			select {
+			case <-clientGone:
+			case <-ctx.Done():
+			}


The cancel got dropped from here. Thats important

wooops. thanks so much for spotting this.

ghost · 2016-10-31T13:09:11Z

addressed:

missing cancel() call
confusing if-conditional ordering

Filed an issue for the unrelated teamcity failure -- #3341

whyrusleeping · 2016-11-02T19:59:30Z

@lgierth whats needed here? more review?

ghost · 2016-11-02T22:03:42Z

I think it's good to go, I addressed the remaining feedback

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

ghost · 2016-11-14T18:48:40Z

Ping -- I think this is good to go

whyrusleeping · 2016-11-14T22:23:46Z

Thanks @lgierth !

gateway: use core api for serving GET/HEAD/POST This commit was moved from ipfs/kubo@aae9eb7

ghost added topic/gateway Topic gateway need/review Needs a review topic/core-api Topic core-api labels Sep 20, 2016

ghost added this to the IPFS Core API milestone Sep 20, 2016

ghost added the status/in-progress In progress label Sep 20, 2016

whyrusleeping requested changes Sep 23, 2016

View reviewed changes

ghost force-pushed the feat/coreapi branch from b7a1b66 to 3a3a305 Compare October 5, 2016 01:20

ghost force-pushed the feat/gateway-post-coreapi branch from d18ed3c to 4841153 Compare October 5, 2016 01:26

ghost force-pushed the feat/coreapi branch from 3a3a305 to 9917eb3 Compare October 28, 2016 16:01

ghost force-pushed the feat/gateway-post-coreapi branch from 4841153 to 7017c0a Compare October 28, 2016 16:21

ghost mentioned this pull request Oct 28, 2016

Initial Core API, and gateway integration #3207

Closed

ghost changed the base branch from feat/coreapi to master October 28, 2016 16:22

ghost changed the title ~~gateway: use core api for serving POST requests~~ gateway: use core api for serving GET/HEAD/POST Oct 28, 2016

whyrusleeping reviewed Oct 28, 2016

View reviewed changes

whyrusleeping reviewed Oct 30, 2016

View reviewed changes

ghost force-pushed the feat/gateway-post-coreapi branch from e2c6410 to d968711 Compare October 31, 2016 12:50

ghost mentioned this pull request Oct 31, 2016

t0400 add-and-cat failure #3341

Closed

ghost self-assigned this Nov 2, 2016

ghost added the RFM label Nov 4, 2016

Lars Gierth added 3 commits November 7, 2016 18:25

coreapi: get going, add Cat() and Ls()

46239e8

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

gateway: use core api for serving GET/HEAD requests

029f971

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

coreapi: add Add()

036ca3a

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

Lars Gierth added 3 commits November 7, 2016 18:25

gateway: move context/close-notify wiring

c31e4f7

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

gateway: use core api for serving POST requests

0097b42

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

coreapi: reuse go-ipld-node.Link

f610e19

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

ghost force-pushed the feat/gateway-post-coreapi branch from d968711 to f610e19 Compare November 7, 2016 17:26

ghost removed the need/review Needs a review label Nov 9, 2016

whyrusleeping merged commit aae9eb7 into master Nov 14, 2016

whyrusleeping deleted the feat/gateway-post-coreapi branch November 14, 2016 22:15

whyrusleeping removed the status/in-progress In progress label Nov 14, 2016

ghost mentioned this pull request Dec 23, 2016

Code Review v0.4.4...v0.4.5 #3534

Closed

aschmahmann mentioned this pull request Aug 23, 2021

Release v0.10 #8176

Closed

62 tasks

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

Merge pull request ipfs/kubo#3244 from ipfs/feat/gateway-post-coreapi

722c84f

gateway: use core api for serving GET/HEAD/POST This commit was moved from ipfs/kubo@aae9eb7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gateway: use core api for serving GET/HEAD/POST #3244

gateway: use core api for serving GET/HEAD/POST #3244

ghost commented Sep 20, 2016

whyrusleeping left a comment

whyrusleeping Sep 23, 2016

ghost Oct 5, 2016

whyrusleeping Sep 23, 2016

whyrusleeping commented Sep 30, 2016

ghost commented Oct 28, 2016

whyrusleeping Oct 28, 2016

ghost Oct 30, 2016 •

edited by ghost

Loading

whyrusleeping left a comment

whyrusleeping Oct 28, 2016

ghost Oct 30, 2016

whyrusleeping Oct 30, 2016

whyrusleeping Oct 30, 2016

whyrusleeping Oct 30, 2016

ghost Oct 31, 2016

ghost Oct 31, 2016

whyrusleeping Oct 30, 2016

ghost Oct 30, 2016

ghost commented Oct 31, 2016

whyrusleeping commented Nov 2, 2016

ghost commented Nov 2, 2016

ghost commented Nov 14, 2016

whyrusleeping commented Nov 14, 2016

gateway: use core api for serving GET/HEAD/POST #3244

gateway: use core api for serving GET/HEAD/POST #3244

Conversation

ghost commented Sep 20, 2016

whyrusleeping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Sep 30, 2016

ghost commented Oct 28, 2016

Choose a reason for hiding this comment

ghost Oct 30, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 31, 2016

whyrusleeping commented Nov 2, 2016

ghost commented Nov 2, 2016

ghost commented Nov 14, 2016

whyrusleeping commented Nov 14, 2016

ghost Oct 30, 2016 •

edited by ghost

Loading