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

Rework body handling to use providers #23

Closed
wants to merge 1 commit into from

Conversation

jawher
Copy link
Contributor

@jawher jawher commented Aug 22, 2016

Hi !

Currently, sling only handles JSON, form and raw reader bodies.
These 3 types are hard coded in the Sling struct, by having one field for every type.

While it was still possible to send other types of bodies (xml, multipart, ...), their usage wasn't as fluent as the other 3 types.

This PR proposes the following:

  • Introduce the BodyProvider interface
  • Rework the existing supported types to use the interface above
  • Add support for multipart and file upload using the same abstraction
  • Keep the existing shortcut body methods to not break clients

Adding support for a new body types would not require touching the Sling struct. Simply a new implementation of the interface and maybe a factory function would do.

Users wanting to handle other types can simply implement the BodyProvider and use the API as seamlessly as the baked-in types:

sling.New().
        Post("http://localhost:4000/customer").
        BodyProvider(YamlBody(customer))
        ...

The code in this PR is not finished (it could use some more polish and tests).
I just wanted to get your feedback on this proposal before investing more time and effort on it.

@jawher jawher changed the title Reword body handling to use providers Rework body handling to use providers Aug 23, 2016
var body io.Reader = nil

if s.body != nil {
body, err = s.body.Body()
Copy link
Owner

Choose a reason for hiding this comment

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

You can call it s.bodyProvider to avoid the double body awkwardness.

body, err := s.getRequestBody()
if err != nil {
return nil, err
var body io.Reader = nil
Copy link
Owner

Choose a reason for hiding this comment

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

nil is the default

@dghubble
Copy link
Owner

dghubble commented Oct 3, 2016

Thanks. I like the refactor in general, though I'd prefer the multipart code be split into a separate review. I'll set aside time to consider it in depth wrt. other APIs, whereas the refactor by itself to use BodyProvider is more straight forward to merge. Then in the short term you could use your customer Provider too.

How about sticking with io.Reader for now, I'm not sure the ReadCloser is needed for all bodies and it seems to be introduced for multipart.

"io"
"io/ioutil"
"strings"

Copy link
Owner

Choose a reason for hiding this comment

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

stdlib packages can all be clumped together

@jawher
Copy link
Contributor Author

jawher commented Oct 4, 2016

Thanks. I like the refactor in general, though I'd prefer the multipart code be split into a separate review. I'll set aside time to consider it in depth wrt. other APIs, whereas the refactor by itself to use BodyProvider is more straight forward to merge. Then in the short term you could use your customer Provider too.

Great ! I'll get working on it.

How about sticking with io.Reader for now, I'm not sure the ReadCloser is needed for all bodies and it seems to be introduced for multipart.

Actually I used io.ReaderColer because that was the type of the existing raw body field. But yeah, I can change to a simple io.Reader 👍

@dghubble
Copy link
Owner

dghubble commented Oct 4, 2016

Re-reading my original comment for Body, it looks like the reason for the io.ReadCloser was so

If the provided body is also an io.Closer, the request Body will be closed by http.Client methods.

rc, ok := body.(io.ReadCloser)
if !ok && body != nil {
    rc = ioutil.NopCloser(body)
}

I don't think a type assertion attempt and NopCloser wrap at the time of setting the body are required though. It looks like http.NewRequest does this already (perhaps it always did) https://golang.org/src/net/http/request.go?s=21674:21746#L680. So, I don't think io.ReadCloser would need to make its way into the BodyProvider and we're justified using io.Reader and it is simpler as you said.

Incidentally, also add support for multipart and file uploading.
@jawher
Copy link
Contributor Author

jawher commented Oct 4, 2016

@dghubble right !

I updated the PR to handle your feedback.

@dghubble
Copy link
Owner

dghubble commented Oct 10, 2016

Awesome. I've added a tweak in #25 to keep the BodyProvider implementations as an internal detail and a few minor things. Unless there are objections, I'll merge that tomorrow.

Thanks!

@dghubble
Copy link
Owner

Merged in #25

@dghubble dghubble closed this Oct 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants