mirror of
https://github.com/Foundry376/Mailspring.git
synced 2024-11-12 12:40:08 +08:00
ea76b7c442
Summary:
Until now, we've been hiding transactions beneath the surface. When you call persistModel, you're implicitly creating a transaction.
You could explicitly create them with `atomically`..., but there were several critical problems that are fixed in this diff:
- Calling persistModel / unpersistModel within a transaction could cause the DatabaseStore to trigger. This could result in other parts of the app making queries /during/
the transaction, potentially before the COMMIT occurred and saved the changes. The new, explicit inTransaction syntax holds all changes until after COMMIT and then triggers.
- Calling atomically and then calling persistModel inside that resulted in us having to check whether a transaction was present and was gross.
- Many parts of the code ran extensive logic inside a promise chained within `atomically`:
BAD:
```
DatabaseStore.atomically =>
DatabaseStore.persistModel(draft) =>
GoMakeANetworkRequestThatReturnsAPromise
```
OVERWHELMINGLY BETTER:
```
DatabaseStore.inTransaction (t) =>
t.persistModel(draft)
.then =>
GoMakeANetworkRequestThatReturnsAPromise
```
Having explicit transactions also puts us on equal footing with Sequelize and other ORMs. Note that you /have/ to call DatabaseStore.inTransaction (t) =>. There is no other way to access the methods that let you alter the database. :-)
Other changes:
- This diff removes Message.labels and the Message-Labels table. We weren't using Message-level labels anywhere, and the table could grow very large.
- This diff changes the page size during initial sync from 250 => 200 in an effort to make transactions a bit faster.
Test Plan: Run tests!
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D2353
177 lines
7.6 KiB
CoffeeScript
177 lines
7.6 KiB
CoffeeScript
_ = require 'underscore'
|
|
Label = require '../../src/flux/models/label'
|
|
Thread = require '../../src/flux/models/thread'
|
|
Message = require '../../src/flux/models/message'
|
|
Actions = require '../../src/flux/actions'
|
|
NylasAPI = require '../../src/flux/nylas-api'
|
|
DatabaseStore = require '../../src/flux/stores/database-store'
|
|
ChangeLabelsTask = require '../../src/flux/tasks/change-labels-task'
|
|
|
|
{APIError} = require '../../src/flux/errors'
|
|
{Utils} = require '../../src/flux/models/utils'
|
|
|
|
testLabels = {}
|
|
testThreads = {}
|
|
|
|
describe "ChangeLabelsTask", ->
|
|
beforeEach ->
|
|
# IMPORTANT: These specs do not run the performLocal logic of their superclass!
|
|
# Tests for that logic are in change-mail-task-spec.
|
|
spyOn(ChangeLabelsTask.__super__, 'performLocal').andCallFake =>
|
|
Promise.resolve()
|
|
|
|
spyOn(DatabaseStore, 'modelify').andCallFake (klass, items) =>
|
|
Promise.resolve items.map (item) =>
|
|
return testLabels[item] if testLabels[item]
|
|
return testThreads[item] if testThreads[item]
|
|
item
|
|
|
|
testLabels = @testLabels =
|
|
"l1": new Label({name: 'inbox', id: 'l1', displayName: "INBOX"}),
|
|
"l2": new Label({name: 'drafts', id: 'l2', displayName: "MyDrafts"})
|
|
"l3": new Label({name: null, id: 'l3', displayName: "My Label"})
|
|
|
|
testThreads = @testThreads =
|
|
't1': new Thread(id: 't1', labels: [@testLabels['l1']])
|
|
't2': new Thread(id: 't2', labels: _.values(@testLabels))
|
|
't3': new Thread(id: 't3', labels: [@testLabels['l2'], @testLabels['l3']])
|
|
|
|
@basicThreadTask = new ChangeLabelsTask
|
|
labelsToAdd: ["l1", "l2"]
|
|
labelsToRemove: ["l3"]
|
|
threads: ['t1']
|
|
|
|
describe "description", ->
|
|
it "should include the name of the added label if it's the only mutation and it was provided as an object", ->
|
|
task = new ChangeLabelsTask(labelsToAdd: ["l1"], labelsToRemove: [], threads: ['t1'])
|
|
expect(task.description()).toEqual("Changed labels on 1 thread")
|
|
task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: [], threads: ['t1'])
|
|
expect(task.description()).toEqual("Added LABEL to 1 thread")
|
|
task = new ChangeLabelsTask(labelsToAdd: [new Label(id: 'l1', displayName: 'LABEL')], labelsToRemove: ['l2'], threads: ['t1'])
|
|
expect(task.description()).toEqual("Changed labels on 1 thread")
|
|
|
|
it "should include the name of the removed label if it's the only mutation and it was provided as an object", ->
|
|
task = new ChangeLabelsTask(labelsToAdd: [], labelsToRemove: ["l1"], threads: ['t1'])
|
|
expect(task.description()).toEqual("Changed labels on 1 thread")
|
|
task = new ChangeLabelsTask(labelsToAdd: [], labelsToRemove: [new Label(id: 'l1', displayName: 'LABEL')], threads: ['t1'])
|
|
expect(task.description()).toEqual("Removed LABEL from 1 thread")
|
|
task = new ChangeLabelsTask(labelsToAdd: ['l2'], labelsToRemove: [new Label(id: 'l1', displayName: 'LABEL')], threads: ['t1'])
|
|
expect(task.description()).toEqual("Changed labels on 1 thread")
|
|
|
|
it "should pluralize properly", ->
|
|
task = new ChangeLabelsTask(labelsToAdd: ["l2"], labelsToRemove: ["l1"], threads: ['t1', 't2', 't3'])
|
|
expect(task.description()).toEqual("Changed labels on 3 threads")
|
|
|
|
describe "performLocal", ->
|
|
it "should throw an exception if task has not been given a label, has been given messages, or no threads", ->
|
|
badTasks = [
|
|
new ChangeLabelsTask(),
|
|
new ChangeLabelsTask(threads: [123]),
|
|
new ChangeLabelsTask(threads: [123], messages: ["foo"]),
|
|
new ChangeLabelsTask(labelsToAdd: ['l2'], labelsToRemove: ['l1'], messages: [123]),
|
|
new ChangeLabelsTask(threads: "Thread"),
|
|
]
|
|
goodTasks = [
|
|
new ChangeLabelsTask(
|
|
labelsToAdd: ['l2']
|
|
labelsToRemove: ['l1']
|
|
threads: ['t1']
|
|
)
|
|
]
|
|
caught = []
|
|
succeeded = []
|
|
|
|
runs ->
|
|
[].concat(badTasks, goodTasks).forEach (task) ->
|
|
task.performLocal()
|
|
.then -> succeeded.push(task)
|
|
.catch (err) -> caught.push(task)
|
|
waitsFor ->
|
|
succeeded.length + caught.length == 6
|
|
runs ->
|
|
expect(caught.length).toEqual(badTasks.length)
|
|
expect(succeeded.length).toEqual(goodTasks.length)
|
|
|
|
it 'calls through to super performLocal', ->
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: ['l2']
|
|
labelsToRemove: ['l1']
|
|
threads: ['t1']
|
|
waitsForPromise =>
|
|
task.performLocal().then =>
|
|
expect(task.constructor.__super__.performLocal).toHaveBeenCalled()
|
|
|
|
describe "when object IDs are provided", ->
|
|
beforeEach ->
|
|
@task = new ChangeLabelsTask
|
|
labelsToAdd: ['l2']
|
|
labelsToRemove: ['l1']
|
|
threads: ['t1']
|
|
|
|
it 'resolves the objects before calling super', ->
|
|
waitsForPromise =>
|
|
@task.performLocal().then =>
|
|
expect(@task.labelsToAdd).toEqual([testLabels['l2']])
|
|
expect(@task.labelsToRemove).toEqual([testLabels['l1']])
|
|
expect(@task.threads).toEqual([testThreads['t1']])
|
|
|
|
describe "when objects are provided", ->
|
|
beforeEach ->
|
|
@task = new ChangeLabelsTask
|
|
labelsToAdd: [testLabels['l2']]
|
|
labelsToRemove: [testLabels['l1']]
|
|
threads: [testThreads['t1']]
|
|
|
|
it 'still has the objects when calling super', ->
|
|
waitsForPromise =>
|
|
@task.performLocal().then =>
|
|
expect(@task.labelsToAdd).toEqual([testLabels['l2']])
|
|
expect(@task.labelsToRemove).toEqual([testLabels['l1']])
|
|
expect(@task.threads).toEqual([testThreads['t1']])
|
|
|
|
describe 'change methods', ->
|
|
describe "changesToModel", ->
|
|
it 'properly adds labels', ->
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: [testLabels['l1'], testLabels['l2']]
|
|
labelsToRemove: []
|
|
out = task.changesToModel(testThreads['t1'])
|
|
expect(out).toEqual(labels: [testLabels['l1'], testLabels['l2']])
|
|
|
|
it 'properly removes labels', ->
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: []
|
|
labelsToRemove: [testLabels['l1'], testLabels['l2']]
|
|
out = task.changesToModel(testThreads['t3'])
|
|
expect(out).toEqual(labels: [testLabels['l3']])
|
|
|
|
it 'properly adds and removes labels', ->
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: [testLabels['l1'], testLabels['l2']]
|
|
labelsToRemove: [testLabels['l2'], testLabels['l3']]
|
|
out = task.changesToModel(testThreads['t1'])
|
|
expect(out).toEqual(labels: [testLabels['l1']])
|
|
|
|
it 'should return an == array of labels when no changes have occurred', ->
|
|
thread = new Thread(id: '1', labels: [testLabels['l2'], testLabels['l3'], testLabels['l1']])
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: [testLabels['l3'], testLabels['l1'], testLabels['l2']]
|
|
labelsToRemove: []
|
|
out = task.changesToModel(thread)
|
|
expect(_.isEqual(thread.labels, out.labels)).toBe(true)
|
|
|
|
it 'should not modify the input thread in any way', ->
|
|
thread = new Thread(id: '1', labels: [testLabels['l2'], testLabels['l1']])
|
|
task = new ChangeLabelsTask
|
|
labelsToAdd: []
|
|
labelsToRemove: [testLabels['l2']]
|
|
out = task.changesToModel(thread)
|
|
expect(thread.labels.length).toBe(2)
|
|
expect(out.labels.length).toBe(1)
|
|
|
|
describe "requestBodyForModel", ->
|
|
it 'returns labels:<ids> for both threads and messages', ->
|
|
task = new ChangeLabelsTask()
|
|
|
|
out = task.requestBodyForModel(testThreads['t3'])
|
|
expect(out).toEqual(labels: ['l2', 'l3'])
|