fixes #32 add validation method to endpoint trait and implement bulk write payload size validation #42
Conversation
geota
force-pushed
the
add-endpoint-validation
branch
2 times, most recently
from
0f67c59
to
3db0293
Compare
add validation method to endpoint trait. implement payload size validation for bulk write endpoint.
geota
force-pushed
the
add-endpoint-validation
branch
from
3db0293
to
2928a94
Compare
|
Sorry for the commit noise. My commits were being linked to my old employer github user. |
|
Adrian: thank you so much for this PR! I really like your design for general-purpose validation. My current thought is: I think flexibility of validation is more important than serializing only once. We're already incurring the overhead of sending a network request - surely the serialization overhead is small compared to that. And it's only this one endpoint that has to incur the extra overhead. I like this approach. |
| fn validate(&self) -> Result<(), ApiFailure> { | ||
| if let Some(body) = self.body() { | ||
| // this matches the serialization in HttpApiClient | ||
| let len = serde_json::to_string(&body).unwrap().len(); |
There was a problem hiding this comment.
We try to avoid using unwrap() in production code, because it will panic if the Option is None. Could we use an if let or match or something similar here?
PS if you haven't seen this already, this Which combinator should I use page is great for understanding some nice option/result processing.
There was a problem hiding this comment.
makes sense, its worth calling that I originally took this line from the ApiClient:
https://github.com/cloudflare/cloudflare-rs/blob/master/src/framework/mod.rs#L93
I am happy to fix it there as well. For now I have just corrected my usage of unwrap.
This actually highlights another issue with this approach... the serialization in validate must stay in sync with the impl in the ApiClient or we risk not calculating the length incorrectly. This could be addressed by encapsulating the serialization into a method on the endpoint trait and calling the same method in both places. Happy to take a stab at that... that also may be needed to support #26
This is the first rust I have ever written so please take me to the woodshed.
fn validate(&self) -> Result<(), ApiFailure>method to Endpoint trait and default to Ok.Some design decisions/questions:
Originally, I wanted to expose a validator method on the Endpoint trait that returned a ValidatorType that implemented a Validate trait that operated on reqwest::Request. Wow thats a mouth full...
My hope was that I wouldn't need to serialize the body twice and could defer to Request to check Content-Length and/or just call it's internal len() method.
However, I could not do this because everything in reqwest:Request is crate private/internally scoped and Content-Length is not set by reqwest until send/execute is called.
I could pass in the body directly to the validate function and avoid serializing twice, but then the validation would be more tightly coupled to "body" type validation - i.e. what if an endpoint wants to validate query params?
Im open to suggestions on how we can avoid serializing the body twice, yet still support other forms of validations endpoints may want to perform.