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

Fixes #26. Allow endpoints to specify a serialization function. #105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ispivey
Copy link
Contributor

@ispivey ispivey commented Feb 9, 2020

This change allows endpoints to specify their own body serialization function, while preserving the current default behavior with a default function in the Endpoint trait, addressing #26 .

A subsequent PR will demonstrate how this can be used to implement the "Create or update Worker" API endpoint.

More detail in my comment here: #26 (comment)

@ispivey
Copy link
Contributor Author

ispivey commented Feb 9, 2020

Hrm, thinking about it a little more - I'm not sure if this takes us in the right direction, if the next step is being able to specify a multi-part form body.

To get that, the Endpoint needs to be able to tell framework::ApiClient that it should construct a reqwest::multipart::Form and set the request body with request.multipart(form) rather than request.body(...). Additionally, it needs to specify the parts of the form in excruciating detail (as @ashleymichal pointed out in #26 , seemingly years ago when I was a naive child and had not wrestled with this for a while).

A couple ideas:

  1. Make Endpoints capable of specifying exactly how the body is constructed. They could have a function called by ApiClient::request that takes a RequestBuilder, adds the body in the desired manner, and returns a RequestBuilder that ApiClient::request can further modify and send. This has the downside of coupling Endpoint with reqwest, and I assume they're purposefully decoupled today. However, it would allow an endpoint like CreateWorker to make a multi-part form body explicitly using reqwest.
  2. Add another enum to specify body type (json, string, form, multi-part form) that can be specified as part of an Endpoint implementation. This is analogous to the endpoint::Method enum, and thus seems more in line with the library's current approach. However, it would require creating another layer of indirection for the Endpoint to specify how its form should be built, and for ApiClient::request to read and act on that specification.

Gonnna go take a break from this and talk about it on Monday 😬

@sssilver
Copy link
Contributor

Thanks for the PR! This subject has been coming up a few times, and at this point I think it'd be good for us to have a technical discussion where we all converge on a single solution.

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