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

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

Merged
merged 6 commits into from
Nov 14, 2016

Conversation

ghost
Copy link

@ghost ghost commented Sep 20, 2016

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

@ghost ghost added topic/gateway Topic gateway need/review Needs a review topic/core-api Topic core-api labels Sep 20, 2016
@ghost ghost added this to the IPFS Core API milestone Sep 20, 2016
@ghost ghost added the status/in-progress In progress label Sep 20, 2016
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

One comment, then LGTM

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

}
}()

if i.config.Writable {
switch r.Method {
case "POST":
i.postHandler(w, r)
i.postHandler(ctx, w, r)
Copy link
Member

Choose a reason for hiding this comment

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

<3

@whyrusleeping
Copy link
Member

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

@ghost ghost force-pushed the feat/gateway-post-coreapi branch from d18ed3c to 4841153 Compare October 5, 2016 01:26
@ghost ghost force-pushed the feat/gateway-post-coreapi branch from 4841153 to 7017c0a Compare October 28, 2016 16:21
@ghost ghost changed the base branch from feat/coreapi to master October 28, 2016 16:22
@ghost
Copy link
Author

ghost commented Oct 28, 2016

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

@ghost ghost changed the title gateway: use core api for serving POST requests gateway: use core api for serving GET/HEAD/POST Oct 28, 2016
// Version() CoreVersion
// }

type Link struct {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@ghost ghost Oct 30, 2016

Choose a reason for hiding this comment

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

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

updated

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

One small nitpick, then LGTM

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

Choose a reason for hiding this comment

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

drop the else

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

return nil, err
}

_, ok := dagnode.(*mdag.ProtoNode)
Copy link
Member

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

maybe AddWithContent should return a cid? not sure

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

select {
case <-clientGone:
case <-ctx.Done():
}
Copy link
Member

Choose a reason for hiding this comment

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

The cancel got dropped from here. Thats important

Copy link
Author

Choose a reason for hiding this comment

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

wooops. thanks so much for spotting this.

@ghost ghost force-pushed the feat/gateway-post-coreapi branch from e2c6410 to d968711 Compare October 31, 2016 12:50
@ghost ghost mentioned this pull request Oct 31, 2016
@ghost
Copy link
Author

ghost commented Oct 31, 2016

addressed:

  • missing cancel() call
  • confusing if-conditional ordering

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

@ghost ghost self-assigned this Nov 2, 2016
@whyrusleeping
Copy link
Member

@lgierth whats needed here? more review?

@ghost
Copy link
Author

ghost commented Nov 2, 2016

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

@ghost ghost added the RFM label Nov 4, 2016
Lars Gierth added 3 commits November 7, 2016 18:25
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
Lars Gierth added 3 commits November 7, 2016 18:25
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost force-pushed the feat/gateway-post-coreapi branch from d968711 to f610e19 Compare November 7, 2016 17:26
@ghost ghost removed the need/review Needs a review label Nov 9, 2016
@ghost
Copy link
Author

ghost commented Nov 14, 2016

Ping -- I think this is good to go

@whyrusleeping whyrusleeping merged commit aae9eb7 into master Nov 14, 2016
@whyrusleeping whyrusleeping deleted the feat/gateway-post-coreapi branch November 14, 2016 22:15
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Nov 14, 2016
@whyrusleeping
Copy link
Member

Thanks @lgierth !

@ghost ghost mentioned this pull request Dec 23, 2016
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
gateway: use core api for serving GET/HEAD/POST

This commit was moved from ipfs/kubo@aae9eb7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/core-api Topic core-api topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant