-
Notifications
You must be signed in to change notification settings - Fork 48
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
Type system is not leveraged to avoid mistakes when building sendgrid requests #67
Comments
While it would be nice to ensure that a type has all the necessary fields, I'm not sure there is a feasible way to do this for all the various combinations. There are potentially lots of valid messages that would be hard to check against. I don't see how your example |
If I understand correctly, you mean "It may not be possible to enforce everything by typing, while we should let every possible use of Sendgrid accessible". I agree, and there are a lot of examples of this impossibility to type every constraint. (Template id may be invalid, or corresponding template may not specify a body while body is an Option in the model I suggested...). I even agree that there may be a point where the added complexity in the interface may not be worth the risk of putting incoherent values in the fields. However I don't see how that could be a reason for not preventing mistakes we can easily prevent, e.g. no body, no target email addresses, invalid body content type...
When specifying a content type with the current interface, it's easy to make mistakes: Content::new().set_content_type("text/htlm").set_value(body)) You can forget to set one of the fields, or you can typo the content type. #[derive(Clone, Serialize)]
pub struct Body<'a> {
#[serde(rename = "type")]
pub content_type: ContentType,
pub body: &'a str,
}
#[derive(Clone, Copy)]
pub enum ContentType {
Html,
PlainText,
}
impl Serialize for ContentType {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let content_type_str: &str = match self {
Self::Html => "text/html",
Self::PlainText => "text/plain",
};
content_type_str.serialize(serializer)
}
} As presented, this new representation of |
I am open to adding in typed APIs around things like content type. It would be very easy to just interpret those values at construction time and set a string accordingly. I would welcome changes like this. But I do agree with you that I don't want to have too many types to remember when adding fields, etc. As far as omitting required fields, we could leverage the Perhaps we can start small and incrementally add guard rails for things like this? |
That won't be validated until the request actually runs in production. I think you are right overall; API call validation is the job of the server, not the client. The server can change its mind any day and make a field optional/required, or change the format of that field. It's trivial to make many mistakes when dealing with an API, e.g. typo in the email which Sendgrid will reject correctly but the client can't validate easily. I think the current approach is the best one. Enums for content type are definitely doable, and there is precedent for that in crates like reqwest, although they always have "break glass" ability to set any arbitrary header the user needs and that's important to keep imo. P.S. Really great library, thank you for writing it! |
The following is severely sarcastic and I apologize in advance, but I couldn't find any reasonable way to express my thoughts. Please try to see past the form and understand the idea behind. (Unless by "current approach" you mean the approach I suggested in my previous comment? In which case the following is probably invalid - pretend I didn't write it.)
Yeah let's also just drop all SDKs and just let everyone write their requests raw, because after all the servers could also add new endpoints or remove them.
Yeah the client if they try really hard can make at least one mistake which is difficult to prevent so let's just try to enable as many alternate mistakes as possible so that it's more challenge to write code that works. If you don't agree with the sarcastic stuff above (which would be very legitimate), ask yourself : where do you set the limit, and why? I think there basically shouldn't be one.
Indeed, this would dodge a request in case of bug, but not avoid it, so that doesn't help. The correct thing to do if the field is not optional is... simply to not use an option. |
The library provides lots of way to build Sendgrid queries that have zero chance to succeed:
Examples:
It's already been several times we've gotten trapped by this, resulting in broken deployed software.
Given the amount of fields that are mandatory/have restricted values, there should only be, as much as possible, constructors that respect the Sendgrid constraints.
e.g.:
where
Content
could be:etc.
I feel like providing typing for an API is be the whole point of this kind of library. Otherwise if I'm going to have to read the Sendgrid spec anyway to figure which fields are mandatory and where I should put them, I almost might as well write my own serde SDK.
The text was updated successfully, but these errors were encountered: