The provider type was modelled as a newtype struct around a `String` but
there's a finite number of values, so really it should be an enum. This
wasn't too pressing until now but will shortly come in useful for the
configuration stuff, which needs a provider type too.
The error-handling in this repo grew pretty organically without us
giving much thought to an over-arching strategy. The end result of that
was a proliferation of different error types that weren't really adding
much value.
The API here is simple and the 4xx responses are really limited to bad
requests and bounce/complaint limit violations. Everything else is an
internal server error. As such, the `error` module could be simplified
by employing a blanket `Internal` error type to cover the multitude of
niche errors that should never occur during normal conditions. The
detail for those errors is still available in the `message` property and
Sentry will still log the backtrace of course.
I also noticed that the Rocket catcher stuff wasn't really being used,
so pulled that out too. We have a standard JSON error format like the
rest of the FxA ecosystem, and we always want the response payload to be
the serialisation of that structure.
One side-effect of all these changes is that the errno value has changed
in most cases. However, I took care to preserve the errno for the bounce
and complaint violations, because it's hard-coded in the auth server. It
wouldn't have been that onerous to open a PR for commensurate changes
over there, but the number of errors left here worked out in such a way
that made sense not to bother.
Fixes#201.
Email addresses that we receive from bounce, complaint and delivery
notifications may not be identical matches to what we set on messages.
We already did some normalization by lower-casing the address when
parsing. This change extends that normalization process by trimming
whitespace and unwrapping the address part if it has been wrapped in
angle bracket delimiters to separate it from the display name. This
matches the behaviour of the auth server.
We were sending errors to Sentry at the point where they are logged,
but there are a number of paths where an error is raised without being
logged. Instead, this change captures them at the point of creation,
which should mean we now capture every error.
Fixes#205.
SES provides a UTC-based timestamp for bounce and complaint events but
the auth server opted to ignore it. I'm not sure that was the optimal
decision because messages might linger on a queue (either ours or the
provider's) for a length of time before we process it. Given that we
already trust SES et al to deliver sane notifications, it doesn't seem
too great a stretch to also trust the timestamp on those notifications.
There was a minor violation of the law of Demeter in our `AppError`
struct, where it was directly exposing its inner `AppErrorKind` to
callers. Not a huge deal, but it meant that any changes made to the
inner structure would leak out and require corresponding changes to the
consuming code (and I am planning some of those inner changes as part of
issue #210). Better to expose just the parts that are necessary via its
own API.
At the same time, I opted to replace the home-baked `AppError::json()`
method with a more conventional `impl Serialize`, because it's less
astonishing and just as easy to use with `serde_json::to_string`.
We've grown a lot of modules in this repo and it's hard to see the wood
for the trees when you look in the `src` directory. This commit attempts
to neaten everything up a bit by scoping some of them under collective
parent modules:
* `auth_db`, `delivery_problems` and `message_data` have moved to `db`,
and the old `db` has moved to `db/core`.
* `healthcheck` and `send` have moved to `api`.
* `app_errors`, `duration`, `email_address` and `validate` have moved to
`types`, and `app_errors` is now just `error`.
* `serialize` has moved to `settings` as it was only referenced there.
Hopefully the end result makes it easier to zero in on modules of
interest when working in the repo.
Fixes#211.
For a bunch of types in `src/settings.rs` and for `NotificationType` in
`src/queues/sqs/notification/mod.rs`, we were allocating `String`
instances when it wasn't always necessary. Where an existing slice can
be used, it's preferable.
At the same time, I removed a load of `.0` inner type access for the
newtype structs in `Settings`, because that notation is leaky (if we
want to refactor the struct to something else, all the usage must change
too).
The `bounced_recipients` property of the outgoing notification structure
was missing a serde `rename` attribute. That problem went undetected
because we had no test coverage for the outgoing structure, which was a
pretty large hole in our coverage. This change fixes the issue and adds
some serialization assertions.
This is the first step towards migrating away from the FxA auth db.
We'll start by dual-writing to both databases. Then, after we've got
sufficient history to fully evaluate limit violations, we can switch
over to read from our own datastore and ditch the auth db.
This change does two things:
1. The old `BounceRecord`, `BounceType` and `BounceSubtype` types are
removed from the `auth_db` module into `bounces`, so that they can
more easily be re-used when writing bounce/complaint data to Redis.
2. Those types are renamed to `DeliveryProblem`, `ProblemType` and
`ProblemSubtype` respectively, and the `bounces` module is renamed to
`delivery_problems`. That may seem weird at first, but I have good
reason for it and am open to alternative names if anyone can suggest
something better.
Historically we've always treated complaints as a type of bounce
event, but that's inaccurate because bounces are never preceded by a
delivery event whereas complaints are always preceded by one. This
has caught me out in the past when analysing metrics and expecting a
blanket sum of `deliveries + bounces = sends` to be true. It isn't.
It's only true if you filter complaints from bounces before
calculating the sum.
Because of that, I wanted some different nomenclature that didn't use
"bounces" as an umbrella term for "bounces and complaints". It won't
affect the names we use in the metrics but I think it's important for
language we use in the codebase to be as precise as possible. Things
like "delivery error" or "delivery failure" also seemed inaccurate
for the same reason; a complaint implies that the actual delivery
succeeded. Hence "delivery problems", which seems generic enough to
legitimately include both event types.
Phil Booth
Vlad Filippov