Coder Social home page Coder Social logo

Comments (5)

Ten0 avatar Ten0 commented on June 22, 2024 2

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.)

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.

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.

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.

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.

again:

providing typing for an API is 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.


That won't be validated until the request actually runs in production

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.

from sendgrid-rs.

levkk avatar levkk commented on June 22, 2024 1

As far as omitting required fields, we could leverage the Option type and check that there aren't any None values before sending a request.

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!

from sendgrid-rs.

gsquire avatar gsquire commented on June 22, 2024

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 Content type would handle this either. Could you expand on that?

from sendgrid-rs.

Ten0 avatar Ten0 commented on June 22, 2024

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.

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...

I don't see how your example Content type would handle this either. Could you expand on that?

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.
The representation with the enum would prevent that typo. It could still implement serde::Serialize by serializing into a valid content-type string:

#[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 Body avoids most mistakes one could make when creating this object, while serializing in the exact same way as before, while also being faster (not reallocating Strings all the time), and (correct me if I'm wrong) does not prevent any previously possible valid usage of Sendgrid (the doc seems to specify only text/html and text/plain are valid values for this field. If that wasn't the case, nothing prevents us from adding a Custom variant one would use for specific edge-cases).

from sendgrid-rs.

gsquire avatar gsquire commented on June 22, 2024

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 Option type and check that there aren't any None values before sending a request.

Perhaps we can start small and incrementally add guard rails for things like this?

from sendgrid-rs.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.