cloudflare-rs icon indicating copy to clipboard operation
cloudflare-rs copied to clipboard

fixes #32 add validation method to endpoint trait and implement bulk write payload size validation

Open geota opened this issue 5 years ago • 2 comments

This is the first rust I have ever written so please take me to the woodshed.

  • Add a fn validate(&self) -> Result<(), ApiFailure> method to Endpoint trait and default to Ok.
  • Implement payload size validation for write_bulk endpoint.
  • Tests to confirm behavior.

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.

geota avatar Sep 12 '19 05:09 geota

Sorry for the commit noise. My commits were being linked to my old employer github user.

geota avatar Sep 12 '19 05:09 geota

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.

adamchalmers avatar Sep 12 '19 19:09 adamchalmers