Summary:
Previously we stored message bodies uncompressed inside two different
databases. This was bad for a few reasons:
* Duplicate data in multiple places is an obvious waste of disk space
* Uncompressed data also made the disk footprint bigger than it might
otherwise be
* Storing these large message bodies in the database made the db file
larger than it otherwise could have been, increasing the size of
tables and slowing down queries.
This diff adds support for storing message bodies outside of the
database in compressed flat files. It changes the use of the body column
in the K2 database and the MessageBody table in the edgehill database to
contain a blob of JSON that contains a path to the file on disk. We
use the new format in an incremental fashion without having to perform an
actual database migration by first checking if the body matches our expected
JSON format and treating it appropriately if it doesn't. Both databases refer
to the same file on disk, thus deduplicating the messages bodies. We also
transparently support gzipping the message bodies stored on disk when we
read from/write to the files. The real world space savings depends on the
compressibility of the messages, but we've seen up to ~60% improvement in disk
space usage for certain inboxes. Typical savings are closer to 20%. Also, by
storing messages as separate files on disk we can potentially integrate with
Spotlight search at some point in the future.
Test Plan:
Run locally, make sure that upgrade to this doesn't hose things,
look at size of DB, read emails
Reviewers: evan, spang, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4422
Summary:
After https://github.com/nylas/nylas-mail-all/commit/008cb4c, the shape of
account.connectionSettings changed, which means that ids for accounts
will also change, given that they are based on the connection settings (see
Account.hash()).
This is fine in most cases, except for accounts that were running on a
version of Nylas Mail before 008cb4c, then upgraded to a version
after 008cb4c, and then re-authed their account.
In this scenario, re-authing the account will create a second account with
a different `id` but with the same email address, along with an extra
sequelize database, and will start a second SyncWorker for the same
account. App-side, the `AccountStore` will correctly overwrite the first account,
so users would correctly see just 1 account in the sidebar. However, given
that 2 SyncWorkers would be running for the same account,
message ids would collide in edgehill.db and this would cause
threads to disappear from the ui as if they were being deleted.
To fix this, we need to do 2 things:
- Upon app start, we remove any duplicate accounts that might have been created due to this bug before starting any sync workers
- If we detect that we are going to create a duplicate account upon auth success, we delete the old account first. This will effectively cause sync to restart for the account.
Test Plan:
Verified problem: Checked out a commit before 008cb4c, authed an account, checked out master, re-authed account, verified that duplicate accounts are created.
Then test 2 scenarios:
- With duplicate accounts present, checked out this commit, verified that duplicate account would be removed and sync would function normally.
- With an account authed on a build before 008cb4c, checked out this commit, re-authed the account, and verified that duplicate account wouldn't be created
Reviewers: mark, halla, khamidou, spang
Reviewed By: spang
Subscribers: tomasz
Differential Revision: https://phab.nylas.com/D4425
Summary:
Since buildMime isn't always used explictly for the send operation (we
mostly use it for stuffing the sent folder, which is a separate
operation), move it to MessageUtils. Additionally, add an includeBcc
option. We don't want the BCC header to be visible on outbound messages,
but it needs to be present on the version that is saved to the sent
folder.
When we weren't including the BCC header in the sent folder version, we
were getting an id mismatch between our optimistic message and the
message that gets processed by the sync loop (because we include the bcc
participants when computing the id hash). When the sent message was a
reply, the optimistic message included the threadId, and this had the
side effect of leaving both versions of the sent message in the thread.
Since the optimistic message never receieved it's imap UID, any action
performed on the thread would fail with the missing UID error as seen in
T7941. Adding this BCC header will allow us to compute the proper id
hash and reconcile the synced message with the optimistic message.
Test Plan: manual
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4365
Summary:
Something weird happens with the mime building when the participant name
has an @ symbol in it (e.g. a name and email of hello@gmail.com turns
into 'hello@ <gmail.com hello@gmail.com>'), so replace it with
whitespace.
Test Plan: manual
Reviewers: evan, mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4363
Summary:
Fixes T8076. So, the Gmail OAuth API returns us an expiration time in milliseconds, but we convert it to seconds everywhere, except in the cloud-api. This caused us to never refresh tokens because we'd be comparing seconds to milliseconds.
Fix this by converting the `expiry_date` from our credentials to seconds.
Test Plan: Tested manually. Will do more testing tomorrow, after my credentials expire.
Reviewers: evan, juan
Reviewed By: evan, juan
Maniphest Tasks: T8076
Differential Revision: https://phab.nylas.com/D4390
Summary:
MySQL would insert bad JSON due to emoji errors. This would cause JSON
parse errors and prevent us from ever reading objects from the DB. We now
catch and return null instead with an error which will at least let the
query complete to let us isolate and deal with the malformed obeject (like
destroy it)
Test Plan: manual
Reviewers: spang, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4389
Summary:
We need this in order to be able to store 4-byte emoji and other
extended unicode characters in the database.
Test Plan: deploy to staging - need the change that sets the connection charset on staging :(
Reviewers: khamidou, juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D4387
Summary:
We were not generating the XOAuth2 token on the first go when connecting
accounts. This would cause the first send later to fail. Since this
happens in ISO core, we needed to move the function out of cloud-core and
import from other places that use it
Test Plan: Manual
Reviewers: khamidou, juan, halla, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4375
Summary:
Previously, we would initialize an `AccountPool` with an account
instance and reference the account as an instance variable. Then,
every time we created a connection, we used the account we had a
reference to obtain the credentials.
However, if the credentials on the account changed, e.g. when we refresh
the access token for gmail accounts, the connection pool would never
realize that the account had new credentials and always try to create
the connection with the old credentials, causing the sync worker to
enter an error loop of `Invalid credentials` errors
To fix this, we could reload the account every time we try to create a
connection, but it seems that the fresh credentials should just be
passed every time we create connections without the pool having to be
aware of potential changes under the rug.
This commit makes it so the pool no longer holds a reference to the
account, but rather it is passed in every time we check out a connection
Test Plan: manual. how did this even work before? :(
Reviewers: evan, halla, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4370
Summary:
Previously, the generic IMAP auth screen presented one security option to
users: "Require SSL". This was ambiguous and difficult to translate into
the correct security options behind the scenes, causing confusion and problems
connecting some accounts.
This patch does the following:
* Separates security settings for IMAP and SMTP, as these different protocols
may also require different SSL/TLS settings
* Reworks the generic IMAP auth page to allow specifying security settings
with higher fidelity. We looked at various different email apps and decided
that the best solution to this problem was to allow more detailed
specification of security settings and to ease the burden of more options
by having sane defaults that work correctly in the majority of cases.
This new screen allows users to pick from "SSL / TLS", "STARTTLS", or "none"
for the security settings for a protocol, and also to instruct us that
they're OK with us using known insecure SSL settings to connect to their
server by checking a checkbox.
We default to port 993 / SSL/TLS for IMAP and port 587 / STARTTLS for SMTP.
These are the most common settings for providers these days and will work
for most folks.
* Significantly tightens our default security. Now that we can allow folks to
opt-in to bad security, by default we should protect folks as best we can.
* Removes some now-unnecessary jank like specifying the SSLv3 "cipher"
in some custom SMTP configs. I don't think this was actually necessary
as SSLv3 is a protocol and not a valid cipher, but these custom
configs may have been necessary because of how the ssl_required flag was
linked between IMAP and SMTP before (and thus to specify different
settings for SMTP you'd have to override the SMTP config).
* Removes hard-coding of Gmail & Office365 settings in several
locations. (This was a major headache while working on the patch.)
This depends on version 2.0.1 of imap-provider-settings, which has major
breaking changes from version 1.0. See commit for more info:
9851054f91
Among other things, I did a serious audit of the settings in this file and
"upgraded" a few servers which weren't using the SSL-enabled ports for their
provider to the secure ones. Hurray for nmap and openssl.
Test Plan: manual
Reviewers: evan, mark, juan, halla
Reviewed By: juan, halla
Differential Revision: https://phab.nylas.com/D4316
Summary:
Previously, calling the IMAPConnectionPool `onDone` callback when checking out a connection would just return the connection to the pool without re-establishing it the next time we checked it out of the pool.
When the `onDone` callback was called in an error scenario, this had the unintended consequence of returning a bad connection to the pool, and then re-using this bad connection. The only scenario when the connection was re-established was if `onConnected` threw an error, but if the connection was kept open outside the scope of onConnected, this logic would never run.
To make things simpler and more consistent, `onDone` will always destroy all connections and connections will always be re-established from scratch every time they are checked out of the pool. The pool acts more as a limit to the number of connections per account, than an actual shared pool of connections.
Test Plan: manual
Reviewers: evan, spang, halla, mark
Reviewed By: halla, mark
Differential Revision: https://phab.nylas.com/D4360
Summary:
This removes the old SignalFX reporter that we weren't using
Adding documentation around logging
Test Plan: manual
Reviewers: juan, halla, spang
Reviewed By: juan, halla, spang
Differential Revision: https://phab.nylas.com/D4341
Summary:
According to bunyan, https://github.com/trentm/node-bunyan, errors are
treated special. They either need to be the first argument of a log, or
they need to be attached to the special `err` (NOT `error`) key of the
logger object.
Test Plan: manual
Reviewers: halla, juan, spang
Reviewed By: halla, juan, spang
Differential Revision: https://phab.nylas.com/D4337
Summary:
Sometimes, when logging out of your NylasID and restarting the app, we
would continue making some requests from the worker window that required a
NylasID. This would make the app enter a restart loop and become
completely unresponsive because when we made a request without a
NylasID, we would force the user to log out and restart the app, and
then we would again make the requests without the id, ad infinitum.
To fix this, we make sure we have a NylasID before making any requests
that require it
Test Plan: manual
Reviewers: halla, evan
Reviewed By: halla, evan
Differential Revision: https://phab.nylas.com/D4344
Summary:
Use the wonderful `debug` library instead of all our random flags.
You can now do things like: `DEBUG="app:*,sync:*" npm start` to print out
everything.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4297
Summary:
This adds better logging to the DB
You can use `ENABLE_SEQUELIZE_DEBUG_LOGGING=true` and
`ENABLE_RXDB_DEBUG_LOGGING=true` to spit out the raw queries of both DBs.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4294
Summary:
This patch will prevent users from being able to connect accounts which sync
mail but fail to send.
This commit includes a couple pieces:
* Adds a call to nodemailer's `verify()` function in the /auth endpoint
* Adds Error object conversion for SMTP errors. Since we don't implement our
own connection object or connection pool for SMTP, we simply wrap the couple
places we call functions from nodemailer that connect to SMTP, namely
SendmailClient's _send() and the new verify() call in /auth.
* Moves RetryableError to the 'errors' module since it's now a base class for
retryable IMAP //and// SMTP errors.
* Moves the main `smtpConfig()` logic which used to live on the Account model
into AuthHelpers so it can be shared between the Account model and the verify
code.
* Converts a few different places to use `import` syntax instead of
`require` syntax for module imports. Apologies for not splitting this out
into a separate diff—would have been a fair amount of work and looks not too
difficult to skim over in the context of the rest of the patch.
* Fixing a bug in a previous commit where erroring sends would crash because of
using `this._transporter.options` instead of `this._transporter.transporter.options`
Test Plan: manual
Reviewers: evan, halla, juan
Reviewed By: halla, juan
Differential Revision: https://phab.nylas.com/D4200
Summary: Should help a fair bit with our redis connection pileup.
Test Plan: Tested manually.
Reviewers: mark, spang, juan, evan
Reviewed By: mark, spang, evan
Differential Revision: https://phab.nylas.com/D3916
Summary:
These only really matter for n1cloud, and are being added manually in
production, so no migration file necessary. I am adding them to the
model definitions to prevent them getting out of sync with production
for development.
See T7999 for full SQL review
Test Plan: ship it
Reviewers: evan, khamidou, juan
Reviewed By: juan
Subscribers: vlad, jerm
Differential Revision: https://phab.nylas.com/D4230
Summary:
Different clients can have different policies for retrying after
timeouts.
Test Plan: Run locally, run tests
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4247
Summary:
Found a funny send-later bug I didn't catch when testing on staging: sometimes the data we're saving in the metadata table overflows. That's because MySQL's TEXT column are at most 64k, which is easy to reach when you have a draft + clearbit information and additional stuff.
To work around this, I decided to switch the database type of the metadata table to LONGTEXT. Since it can store 4Gb of text, we should be good. This diff makes those code changes. Obviously, we'll have to run migrations both on staging and prod.
Test Plan: Ran a basic smoke test. Shouldn't break anything.
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4250
Summary:
When querying transactions for the delta stream, if no cursor is provided,
we should default to 0. Otherwise this will generate a query like:
```
WHERE `transaction`.`id` > ‘null’
```
Which is obviously wrong
Test Plan: manual
Reviewers: evan, halla, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D4242
Summary:
We were using a version that was ~9 months old and a lot of development has
happened since.
v3 is not compatible w/v2, but it looks like we aren't using any of the
features that had breaking changes:
http://nodemailer.com/about/migrate/
I chose not to switch to the new built-in OAuth2 token refresh support
because we already have a mechanism for refreshing oauth tokens and
adding a different implementation specifically for SMTP would introduce
more opportunities for bugs.
Since mailcomposer is no longer a dependency of nodemailer, I added this
dependency as well.
Test Plan: manual - sent a message, sent a message w/an attachment
Reviewers: evan, khamidou, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4201
Summary:
Previously we limited it to 3. Gmail supports up to 15, many IMAP
servers support 10 by default. So let's bump it for those folks.
Test Plan: Run locally, verify things still work.
Reviewers: evan, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4175
Summary:
If an error is thrown while opening an imap box, we need to make sure to
set the `_isOpening` back to false
Test Plan: manual
Reviewers: mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D4134
Summary:
In many cases when send fails, the server settings may be incorrect.
Send this data to Sentry with the original error in order to help us
diagnose.
I think the grouping will only contain settings for a single user
getting the same error, but should still be good enough to do further
investigation, e.g. https://sentry.io/nylas/nylas-mail/issues/230529222/
We can always go look up the requests in the n1Cloud logs if more info is
needed.
Test Plan: yolo
Reviewers: mark, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D4131
Arc land messed up and landed a not fully merged branch. (Seriously – I
had merged a copy of my branch with master to see how easy it would be.
Because I didn't want to merge the whole thing, I blindly committed my
changes and switched back to my real branch). To my great surprise, arc
decided to use the wrong branch when landing it.
Original commit message:
Summary:
Finally, here it is! Send later, with support for open tracking but
without support for attachments yet. It took me some time to find the
right way to do things.
**The send later dilemna**
There's two ways we could handle send later:
1. do everything on the client
2. process the message in the cloud
1. is very tempting because it would make the cloud server very
simple. Unfortunately, it has some serious limitations, for example,
setting the "Date" message header. That's why I chose to go with 2. When
a user presses the "Send Later" button, we save the open/link tracking
metadata and fills in all the required fields. I added a custom endpoint
to the K2 API to do this, `/drafts/build`. After that, we save the JSON
contents of the message as metadata.
When we process metadata, we simply create a MIME message from the
JSON and send it.
**Limitations**
Right now, send later doesn't support and attachments. There's also
some minor code duplication which needs to be refactored away.
Test Plan: Tested manually. Checked that regular send still worked, too.
Reviewers: mark, spang, halla, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4054
Summary:
Finally, here it is! Send later, with support for open tracking but without support for attachments yet. It took me some time to find the right way to do things.
**The send later dilemna**
There's two ways we could handle send later:
1. do everything on the client
2. process the message in the cloud
1. is very tempting because it would make the cloud server very simple. Unfortunately, it has some serious limitations, for example, setting the "Date" message header. That's why I chose to go with 2. When a user presses the "Send Later" button, we save the open/link tracking metadata and fills in all the required fields. I added a custom endpoint to the K2 API to do this, `/drafts/build`. After that, we save the JSON contents of the message as metadata.
When we process metadata, we simply create a MIME message from the JSON and send it.
**Limitations**
Right now, send later doesn't support and attachments. There's also some minor code duplication which needs to be refactored away.
Test Plan: Tested manually. Checked that regular send still worked, too.
Reviewers: mark, spang, halla, juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D4054
Summary:
If it's expensive for us to SELECT a folder (which happens a lot for
large gmail accounts), we should try to sync more messages so that we
don't waste a ton of time SELECT-ing during initial sync. This speeds up
initial sync quite a bit.
Test Plan: Run locally, verify batches are properly computed
Reviewers: evan, juan, spang
Reviewed By: juan, spang
Differential Revision: https://phab.nylas.com/D4050
Summary:
Isomorphic-core should be functional outside of the client environment
Depends on D4056
Test Plan: Run the test suite
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4062
Summary:
Move the base Jasmine spec runner into isomorphic-core to prevent
code duplication. Jasmine will look for the config file relative to
the directory it's being run in though, so we need to symlink the
config file into each package that will need it.
Test Plan: Run tests once the suites are integrated
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4056
Summary:
Convert the isomorphic-core specs to Jasmine 1 and symlink them into
client-app/internal_packages so they are run as part of the client
test suite.
Test Plan: Ran the suite with fdescribes in iso-core to ensure they were being called properly
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D4055
