Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Commit

Permalink
refactor(types): prefer EmailAddress type to raw strings
Browse files Browse the repository at this point in the history
The EmailAddress type was only being used in some places, which meant
some places that weren't using it had to call `validate::email_address`
manually. This change spreads it throughout most of the project, so
that almost everywhere gets to benefit from strong typing.

The only place I decided to leave it alone was the provider layer, where
some providers have their own `EmailAddress` struct. I could have
aliased it at that layer too, but by that point addresses are just dumb
strings anyway so it didn't seem worth it.
  • Loading branch information
philbooth committed Oct 4, 2018
1 parent 12c2e32 commit e49fce8
Show file tree
Hide file tree
Showing 17 changed files with 186 additions and 172 deletions.
12 changes: 5 additions & 7 deletions src/app_errors/mod.rs
Expand Up @@ -17,6 +17,7 @@ use rocket_contrib::{Json, JsonValue};
use serde_json::{map::Map, ser::to_string, Value};

use auth_db::BounceRecord;
use email_address::EmailAddress;
use logging::MozlogLogger;

#[cfg(test)]
Expand Down Expand Up @@ -133,19 +134,19 @@ pub enum AppErrorKind {
/// An error for when a bounce violation happens.
#[fail(display = "Email account sent complaint")]
BounceComplaintError {
address: String,
address: EmailAddress,
bounced_at: u64,
bounce: BounceRecord,
},
#[fail(display = "Email account soft bounced")]
BounceSoftError {
address: String,
address: EmailAddress,
bounced_at: u64,
bounce: BounceRecord,
},
#[fail(display = "Email account hard bounced")]
BounceHardError {
address: String,
address: EmailAddress,
bounced_at: u64,
bounce: BounceRecord,
},
Expand Down Expand Up @@ -282,10 +283,7 @@ impl AppErrorKind {
ref bounced_at,
ref bounce,
} => {
fields.insert(
String::from("address"),
Value::String(format!("{}", address)),
);
fields.insert(String::from("address"), Value::String(address.to_string()));
fields.insert(
String::from("bouncedAt"),
Value::Number(bounced_at.clone().into()),
Expand Down
17 changes: 9 additions & 8 deletions src/auth_db/mod.rs
Expand Up @@ -26,6 +26,7 @@ use serde::{
};

use app_errors::{AppError, AppErrorKind, AppResult};
use email_address::EmailAddress;
use settings::Settings;

#[cfg(test)]
Expand Down Expand Up @@ -156,7 +157,7 @@ impl Serialize for BounceSubtype {
#[derive(Clone, Debug, Deserialize, Eq, Serialize, PartialEq)]
pub struct BounceRecord {
#[serde(rename = "email")]
pub address: String,
pub address: EmailAddress,
#[serde(rename = "bounceType")]
pub bounce_type: BounceType,
#[serde(rename = "bounceSubType")]
Expand Down Expand Up @@ -192,8 +193,8 @@ impl DbUrls {
}
}

pub fn get_bounces(&self, address: &str) -> Result<Url, UrlError> {
self.get_bounces.join(&hex::encode(address))
pub fn get_bounces(&self, address: &EmailAddress) -> Result<Url, UrlError> {
self.get_bounces.join(&hex::encode(address.as_ref()))
}

pub fn create_bounce(&self) -> Url {
Expand All @@ -202,11 +203,11 @@ impl DbUrls {
}

pub trait Db: Debug + Sync {
fn get_bounces(&self, address: &str) -> AppResult<Vec<BounceRecord>>;
fn get_bounces(&self, address: &EmailAddress) -> AppResult<Vec<BounceRecord>>;

fn create_bounce(
&self,
_address: &str,
_address: &EmailAddress,
_bounce_type: BounceType,
_bounce_subtype: BounceSubtype,
) -> AppResult<()> {
Expand All @@ -230,7 +231,7 @@ impl DbClient {
}

impl Db for DbClient {
fn get_bounces(&self, address: &str) -> AppResult<Vec<BounceRecord>> {
fn get_bounces(&self, address: &EmailAddress) -> AppResult<Vec<BounceRecord>> {
let mut response = self
.request_client
.get(self.urls.get_bounces(address)?)
Expand All @@ -243,15 +244,15 @@ impl Db for DbClient {

fn create_bounce(
&self,
address: &str,
address: &EmailAddress,
bounce_type: BounceType,
bounce_subtype: BounceSubtype,
) -> AppResult<()> {
let response = self
.request_client
.post(self.urls.create_bounce())
.json(&BounceRecord {
address: address.to_string(),
address: address.clone(),
bounce_type,
bounce_subtype,
created_at: 0,
Expand Down
17 changes: 4 additions & 13 deletions src/auth_db/test.rs
Expand Up @@ -138,21 +138,11 @@ fn serialize_bounce_subtype() {
fn get_bounces() {
let settings = Settings::new().expect("config error");
let db = DbClient::new(&settings);
if let Err(error) = db.get_bounces("foo@example.com") {
if let Err(error) = db.get_bounces(&"foo@example.com".parse().unwrap()) {
assert!(false, format!("{}", error));
}
}

#[test]
fn get_bounces_invalid_address() {
let settings = Settings::new().expect("config error");
let db = DbClient::new(&settings);
match db.get_bounces("") {
Ok(_) => assert!(false, "DbClient::get_bounces should have failed"),
Err(error) => assert_eq!(format!("{}", error), "400 Bad Request"),
}
}

#[test]
fn create_bounce() {
let settings = Settings::new().expect("config error");
Expand Down Expand Up @@ -200,12 +190,13 @@ fn create_bounce() {
assert!(second_bounce.created_at > bounce.created_at);
}

fn generate_email_address(variant: &str) -> String {
fn generate_email_address(variant: &str) -> EmailAddress {
format!(
"fxa-email-service.test.auth-db.{}.{}@example.com",
variant,
now_as_milliseconds()
)
).parse()
.unwrap()
}

fn now_as_milliseconds() -> u64 {
Expand Down
13 changes: 7 additions & 6 deletions src/bounces/mod.rs
Expand Up @@ -8,6 +8,7 @@ use std::{collections::HashMap, time::SystemTime};

use app_errors::{AppErrorKind, AppResult};
use auth_db::{BounceSubtype as DbBounceSubtype, BounceType as DbBounceType, Db};
use email_address::EmailAddress;
use queues::notification::{BounceSubtype, BounceType, ComplaintFeedbackType};
use settings::{BounceLimit, BounceLimits, Settings};

Expand Down Expand Up @@ -49,7 +50,7 @@ where
/// defined in the [`BounceLimits` setting][limits].
///
/// [limits]: ../settings/struct.BounceLimits.html
pub fn check(&self, address: &str) -> AppResult<()> {
pub fn check(&self, address: &EmailAddress) -> AppResult<()> {
let bounces = self.db.get_bounces(address)?;
let now = SystemTime::now()
.duration_since(SystemTime::UNIX_EPOCH)
Expand All @@ -69,17 +70,17 @@ where
if is_bounce_violation(*count, bounce.created_at, now, limits) {
return match bounce.bounce_type {
DbBounceType::Hard => Err(AppErrorKind::BounceHardError {
address: address.to_string(),
address: address.clone(),
bounced_at: bounce.created_at,
bounce: bounce.clone(),
}.into()),
DbBounceType::Soft => Err(AppErrorKind::BounceSoftError {
address: address.to_string(),
address: address.clone(),
bounced_at: bounce.created_at,
bounce: bounce.clone(),
}.into()),
DbBounceType::Complaint => Err(AppErrorKind::BounceComplaintError {
address: address.to_string(),
address: address.clone(),
bounced_at: bounce.created_at,
bounce: bounce.clone(),
}.into()),
Expand All @@ -95,7 +96,7 @@ where
/// against an email address.
pub fn record_bounce(
&self,
address: &str,
address: &EmailAddress,
bounce_type: BounceType,
bounce_subtype: BounceSubtype,
) -> AppResult<()> {
Expand All @@ -108,7 +109,7 @@ where
/// against an email address.
pub fn record_complaint(
&self,
address: &str,
address: &EmailAddress,
complaint_type: Option<ComplaintFeedbackType>,
) -> AppResult<()> {
let bounce_subtype = complaint_type.map_or(DbBounceSubtype::Unmapped, |ct| From::from(ct));
Expand Down

0 comments on commit e49fce8

Please sign in to comment.