From 2bd03dc44f6b2713014c21531dd7b6dc9a053098 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Thu, 10 Mar 2016 11:06:06 -0800 Subject: [PATCH] fix(sync): Pull down and associate metadata during initial sync Summary: Snooze should wait for categories on all accounts Fix authPlugin to rembmer `plugin+accountId`, not pluginId, add specs categories() returned [], categories(acctId) returned {} dry up sync worker, fetch metadata before anything else Test Plan: Run tests Reviewers: drew, juan Reviewed By: juan Differential Revision: https://phab.nylas.com/D2693 --- .../thread-snooze/lib/snooze-utils.js | 6 +- .../worker-sync/lib/nylas-sync-worker.coffee | 85 ++++++++++---- .../spec/nylas-sync-worker-spec.coffee | 62 ++++++++--- spec/nylas-api-spec.coffee | 104 ++++++++++++++++++ src/flux/models/model-with-metadata.es6 | 7 ++ src/flux/nylas-api.coffee | 63 +++++++---- src/flux/stores/category-store.coffee | 5 +- 7 files changed, 269 insertions(+), 63 deletions(-) diff --git a/internal_packages/thread-snooze/lib/snooze-utils.js b/internal_packages/thread-snooze/lib/snooze-utils.js index 41bea86d5..2629f0519 100644 --- a/internal_packages/thread-snooze/lib/snooze-utils.js +++ b/internal_packages/thread-snooze/lib/snooze-utils.js @@ -58,8 +58,8 @@ const SnoozeUtils = { }) }, - whenCategoriesReady() { - const categoriesReady = ()=> CategoryStore.categories().length > 0 + whenCategoriesReady(accountId) { + const categoriesReady = ()=> CategoryStore.categories(accountId).length > 0; if (!categoriesReady()) { return new Promise((resolve)=> { const unsubscribe = CategoryStore.listen(()=> { @@ -74,7 +74,7 @@ const SnoozeUtils = { }, getSnoozeCategory(accountId, categoryName = SNOOZE_CATEGORY_NAME) { - return SnoozeUtils.whenCategoriesReady() + return SnoozeUtils.whenCategoriesReady(accountId) .then(()=> { const allCategories = CategoryStore.categories(accountId) const category = _.findWhere(allCategories, {displayName: categoryName}) diff --git a/internal_packages/worker-sync/lib/nylas-sync-worker.coffee b/internal_packages/worker-sync/lib/nylas-sync-worker.coffee index bcbbf87e9..691a23c33 100644 --- a/internal_packages/worker-sync/lib/nylas-sync-worker.coffee +++ b/internal_packages/worker-sync/lib/nylas-sync-worker.coffee @@ -106,23 +106,51 @@ class NylasSyncWorker # we'll backoff and restart the timer. @_resumeTimer.cancel() - @fetchCollection('threads') - if @_account.usesLabels() - @fetchCollection('labels', {initialPageSize: 1000}) - if @_account.usesFolders() - @fetchCollection('folders', {initialPageSize: 1000}) - @fetchCollection('drafts') - @fetchCollection('contacts') - @fetchCollection('calendars') - @fetchCollection('events') + needed = [ + {model: 'threads'}, + {model: "#{@_account.organizationUnit}s", initialPageSize: 1000} + {model: 'drafts'}, + {model: 'contacts'}, + {model: 'calendars'}, + {model: 'events'}, + ].filter ({model}) => + @shouldFetchCollection(model) - fetchCollection: (model, options = {}) -> - return unless @_state + return if needed.length is 0 + + @fetchAllMetadata => + needed.forEach ({model, initialPageSize}) => + @fetchCollection(model, initialPageSize) + + fetchAllMetadata: (finished) -> + @_metadata = {} + makeMetadataRequest = (offset) => + limit = 200 + @_fetchWithErrorHandling + path: "/metadata" + qs: {limit, offset} + success: (data) => + for metadatum in data + @_metadata[metadatum.object_id] ?= [] + @_metadata[metadatum.object_id].push(metadatum) + if data.length is limit + makeMetadataRequest(offset + limit) + else + console.log("Retrieved #{offset + data.length} metadata objects") + finished() + + makeMetadataRequest(0) + + shouldFetchCollection: (model) -> + return false unless @_state state = @_state[model] ? {} - return if state.complete and not options.force? - return if state.busy + return false if state.complete + return false if state.busy + return true + fetchCollection: (model, initialPageSize = INITIAL_PAGE_SIZE) -> + state = @_state[model] ? {} state.complete = false state.error = null state.busy = true @@ -138,7 +166,7 @@ class NylasSyncWorker @fetchCollectionPage(model, {limit, offset}) else @fetchCollectionPage(model, { - limit: options.initialPageSize ? INITIAL_PAGE_SIZE, + limit: initialPageSize, offset: 0 }) @@ -146,23 +174,17 @@ class NylasSyncWorker @writeState() fetchCollectionCount: (model) -> - @_api.makeRequest - accountId: @_account.id + @_fetchWithErrorHandling path: "/#{model}" - returnsModel: false - qs: - view: 'count' + qs: {view: 'count'} success: (response) => - return if @_terminated @updateTransferState(model, count: response.count) - error: (err) => - return if @_terminated - @_resumeTimer.backoff() - @_resumeTimer.start() fetchCollectionPage: (model, params = {}) -> requestStartTime = Date.now() requestOptions = + metadataToAttach: @_metadata + error: (err) => return if @_terminated @_fetchCollectionPageError(model, params, err) @@ -203,6 +225,21 @@ class NylasSyncWorker _hasNoInbox: (json) -> return not _.any(json, (obj) -> obj.name is "inbox") + _fetchWithErrorHandling: ({path, qs, success, error}) -> + @_api.makeRequest + accountId: @_account.id + returnsModel: false + path: path + qs: qs + success: (response) => + return if @_terminated + success(response) if success + error: (err) => + return if @_terminated + @_resumeTimer.backoff() + @_resumeTimer.start() + error(err) if error + _fetchCollectionPageError: (model, params, err) -> @_resumeTimer.backoff() @_resumeTimer.start() diff --git a/internal_packages/worker-sync/spec/nylas-sync-worker-spec.coffee b/internal_packages/worker-sync/spec/nylas-sync-worker-spec.coffee index bff7365bf..95e5efbef 100644 --- a/internal_packages/worker-sync/spec/nylas-sync-worker-spec.coffee +++ b/internal_packages/worker-sync/spec/nylas-sync-worker-spec.coffee @@ -17,6 +17,7 @@ describe "NylasSyncWorker", -> @apiRequests.push({account, model:'threads', params, requestOptions}) @apiCursorStub = undefined + spyOn(NylasSyncWorker.prototype, 'fetchAllMetadata').andCallFake (cb) -> cb() spyOn(DatabaseTransaction.prototype, 'persistJSONBlob').andReturn(Promise.resolve()) spyOn(DatabaseStore, 'findJSONBlob').andCallFake (key) => if key is "NylasSyncWorker:#{TEST_ACCOUNT_ID}" @@ -37,6 +38,7 @@ describe "NylasSyncWorker", -> @account = new Account(clientId: TEST_ACCOUNT_CLIENT_ID, serverId: TEST_ACCOUNT_ID, organizationUnit: 'label') @worker = new NylasSyncWorker(@api, @account) + @worker._metadata = {"a": [{"id":"b"}]} @connection = @worker.connection() spyOn(@connection, 'start') advanceClock() @@ -177,35 +179,58 @@ describe "NylasSyncWorker", -> expect(nextState.threads.count).toEqual(1001) describe "resumeFetches", -> - it "should fetch collections", -> + it "should fetch metadata first and fetch other collections when metadata is ready", -> + fetchAllMetadataCallback = null + jasmine.unspy(NylasSyncWorker.prototype, 'fetchAllMetadata') + spyOn(NylasSyncWorker.prototype, 'fetchAllMetadata').andCallFake (cb) => + fetchAllMetadataCallback = cb spyOn(@worker, 'fetchCollection') + @worker._state = {} @worker.resumeFetches() - expect(@worker.fetchCollection.calls.map (call) -> call.args[0]).toEqual(['threads', 'labels', 'drafts', 'contacts', 'calendars', 'events']) + expect(@worker.fetchAllMetadata).toHaveBeenCalled() + expect(@worker.fetchCollection.calls.length).toBe(0) + fetchAllMetadataCallback() + expect(@worker.fetchCollection.calls.length).not.toBe(0) + + it "should fetch collections for which `shouldFetchCollection` returns true", -> + spyOn(@worker, 'fetchCollection') + spyOn(@worker, 'shouldFetchCollection').andCallFake (collection) => + return collection in ['threads', 'labels', 'drafts'] + @worker.resumeFetches() + expect(@worker.fetchCollection.calls.map (call) -> call.args[0]).toEqual(['threads', 'labels', 'drafts']) it "should be called when Actions.retryInitialSync is received", -> spyOn(@worker, 'resumeFetches').andCallThrough() Actions.retryInitialSync() expect(@worker.resumeFetches).toHaveBeenCalled() - describe "fetchCollection", -> - beforeEach -> - @apiRequests = [] - - it "should not start if the collection sync is already in progress", -> + describe "shouldFetchCollection", -> + it "should return false if the collection sync is already in progress", -> @worker._state.threads = { 'busy': true 'complete': false } - @worker.fetchCollection('threads') - expect(@apiRequests.length).toBe(0) + expect(@worker.shouldFetchCollection('threads')).toBe(false) - it "should not start if the collection sync is already complete", -> + it "should return false if the collection sync is already complete", -> @worker._state.threads = { 'busy': false 'complete': true } - @worker.fetchCollection('threads') - expect(@apiRequests.length).toBe(0) + expect(@worker.shouldFetchCollection('threads')).toBe(false) + + it "should return true otherwise", -> + @worker._state.threads = { + 'busy': false + 'complete': false + } + expect(@worker.shouldFetchCollection('threads')).toBe(true) + @worker._state.threads = undefined + expect(@worker.shouldFetchCollection('threads')).toBe(true) + + describe "fetchCollection", -> + beforeEach -> + @apiRequests = [] it "should start the request for the model count", -> @worker._state.threads = { @@ -216,7 +241,16 @@ describe "NylasSyncWorker", -> expect(@apiRequests[0].requestOptions.path).toBe('/threads') expect(@apiRequests[0].requestOptions.qs.view).toBe('count') - describe "when there is no errorRequestRange saved", -> + it "should pass any metadata it preloaded", -> + @worker._state.threads = { + 'busy': false + 'complete': false + } + @worker.fetchCollection('threads') + expect(@apiRequests[1].model).toBe('threads') + expect(@apiRequests[1].requestOptions.metadataToAttach).toBe(@worker._metadata) + + describe "when there is not a previous page failure (`errorRequestRange`)", -> it "should start the first request for models", -> @worker._state.threads = { 'busy': false @@ -226,7 +260,7 @@ describe "NylasSyncWorker", -> expect(@apiRequests[1].model).toBe('threads') expect(@apiRequests[1].params.offset).toBe(0) - describe "when there is an errorRequestRange saved", -> + describe "when there is a previous page failure (`errorRequestRange`)", -> beforeEach -> @worker._state.threads = 'count': 1200 diff --git a/spec/nylas-api-spec.coffee b/spec/nylas-api-spec.coffee index e9059ac42..21b0d1b1d 100644 --- a/spec/nylas-api-spec.coffee +++ b/spec/nylas-api-spec.coffee @@ -8,6 +8,110 @@ DatabaseStore = require '../src/flux/stores/database-store' DatabaseTransaction = require '../src/flux/stores/database-transaction' describe "NylasAPI", -> + describe "authPlugin", -> + beforeEach -> + NylasAPI.pluginsSupported = true + @authGetResponse = null + @authPostResponse = null + @error = null + @resolved = false + spyOn(NylasEnv.config, 'set') + spyOn(NylasEnv.config, 'get').andReturn(null) + spyOn(NylasAPI, 'makeRequest').andCallFake (options) => + return @authGetResponse if options.method is 'GET' and @authGetResponse + return @authPostResponse if options.method is 'POST' and @authPostResponse + return new Promise (resolve, reject) -> #never respond + + it "should reject if the current environment does not support plugins", -> + NylasAPI.pluginsSupported = false + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).catch (err) => @error = err + waitsFor => + @error + runs => + expect(@error.message).toEqual('Sorry, this feature is only available when N1 is running against the hosted version of the Nylas Sync Engine.') + + it "should reject if no account can be found for the given accountOrId", -> + NylasAPI.authPlugin('PID', 'PSECRET', 'randomAccountId').catch (err) => @error = err + waitsFor => + @error + runs => + expect(@error.message).toEqual('Invalid account') + + it "should resolve if the plugin has been successfully authed with accountOrId already", -> + jasmine.unspy(NylasEnv.config, 'get') + spyOn(NylasEnv.config, 'get').andCallFake (key) => + return Date.now() if key is "plugins.PID.lastAuth.#{TEST_ACCOUNT_ID}" + return null + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).then (err) => + @resolved = true + waitsFor => + @resolved + expect(NylasAPI.makeRequest).not.toHaveBeenCalled() + + describe "check for existing auth", -> + it "should GET /auth/plugin to check if the plugin has been authed", -> + @authGetResponse = Promise.resolve({authed: true}) + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID) + advanceClock() + expect(NylasAPI.makeRequest).toHaveBeenCalledWith({ + returnsModel: false, + method: 'GET', + accountId: 'test-account-server-id', + path: '/auth/plugin?client_id=PID' + }) + + it "should record a successful auth in the config and resolve without making a POST", -> + @authGetResponse = Promise.resolve({authed: true}) + @authPostResponse = null + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).then => @resolved = true + waitsFor => + @resolved + runs => + expect(NylasAPI.makeRequest).toHaveBeenCalled() + expect(NylasEnv.config.set.mostRecentCall.args[0]).toEqual("plugins.PID.lastAuth.#{TEST_ACCOUNT_ID}") + + it "should propagate any network errors back to the caller", -> + @authGetResponse = Promise.reject(new Error("Network failure!")) + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).catch (err) => @error = err + advanceClock() + advanceClock() + expect(@error.message).toBe("Network failure!") + expect(NylasEnv.config.set).not.toHaveBeenCalled() + + describe "request for auth", -> + it "should POST to /auth/plugin with the client id and record a successful auth", -> + @authGetResponse = Promise.resolve({authed: false}) + @authPostResponse = Promise.resolve({authed: true}) + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).then => @resolved = true + waitsFor => + @resolved + runs => + expect(NylasAPI.makeRequest.calls[0].args[0]).toEqual({ + returnsModel: false, + method: 'GET', + accountId: 'test-account-server-id', + path: '/auth/plugin?client_id=PID' + }) + expect(NylasAPI.makeRequest.calls[1].args[0]).toEqual({ + returnsModel: false, + method: 'POST', + accountId: 'test-account-server-id', + path: '/auth/plugin', + body: {client_id: 'PID'}, + json: true + }) + setCall = NylasEnv.config.set.mostRecentCall + expect(setCall.args[0]).toEqual("plugins.PID.lastAuth.#{TEST_ACCOUNT_ID}") + + it "should propagate any network errors back to the caller", -> + @authGetResponse = Promise.resolve({authed: false}) + @authPostResponse = Promise.reject(new Error("Network failure!")) + NylasAPI.authPlugin('PID', 'PSECRET', TEST_ACCOUNT_ID).catch (err) => @error = err + waitsFor => + @error + runs => + expect(@error.message).toBe("Network failure!") + describe "handleModel404", -> it "should unpersist the model from the cache that was requested", -> model = new Thread(id: 'threadidhere') diff --git a/src/flux/models/model-with-metadata.es6 b/src/flux/models/model-with-metadata.es6 index 3f3c61176..975e87f01 100644 --- a/src/flux/models/model-with-metadata.es6 +++ b/src/flux/models/model-with-metadata.es6 @@ -23,6 +23,13 @@ class PluginMetadata extends Model { this.version = this.version || 0; } + fromJSON(json) { + super.fromJSON(json); + + // application_id is used in JSON coming down from the API + this.pluginId = this.pluginId || json.application_id; + } + get id() { return this.pluginId } diff --git a/src/flux/nylas-api.coffee b/src/flux/nylas-api.coffee index 46c966d92..1b86137d5 100644 --- a/src/flux/nylas-api.coffee +++ b/src/flux/nylas-api.coffee @@ -121,7 +121,6 @@ class NylasAPI SampleTemporaryErrorCode: SampleTemporaryErrorCode constructor: -> - @_workers = [] @_lockTracker = new NylasAPIChangeLockTracker() NylasEnv.config.onDidChange('env', @_onConfigChanged) @@ -131,6 +130,7 @@ class NylasAPI prev = {@AppID, @APIRoot, @APITokens} if NylasEnv.inSpecMode() + @pluginsSupported = true env = "testing" else env = NylasEnv.config.get('env') @@ -143,12 +143,15 @@ class NylasAPI if env in ['production'] @AppID = 'eco3rpsghu81xdc48t5qugwq7' @APIRoot = 'https://api.nylas.com' + @pluginsSupported = true else if env in ['staging', 'development'] @AppID = '54miogmnotxuo5st254trcmb9' @APIRoot = 'https://api-staging.nylas.com' + @pluginsSupported = true else if env in ['experimental'] @AppID = 'c5dis00do2vki9ib6hngrjs18' @APIRoot = 'https://api-staging-experimental.nylas.com' + @pluginsSupported = true else if env in ['local'] @AppID = NylasEnv.config.get('syncEngine.AppID') or 'n/a' @APIRoot = 'http://localhost:5555' @@ -321,6 +324,12 @@ class NylasAPI .then -> return Promise.resolve(responseModels) + _attachMetadataToResponse: (jsons, metadataToAttach) -> + return unless metadataToAttach + for obj in jsons + if metadataToAttach[obj.id] + obj.metadata = metadataToAttach[obj.id] + _apiObjectToClassMap: "file": require('./models/file') "event": require('./models/event') @@ -341,6 +350,7 @@ class NylasAPI if result.messages messages = messages.concat(result.messages) if messages.length > 0 + @_attachMetadataToResponse(messages, requestOptions.metadataToAttach) @_handleModelResponse(messages) if requestSuccess requestSuccess(json) @@ -350,11 +360,17 @@ class NylasAPI getCollection: (accountId, collection, params={}, requestOptions={}) -> throw (new Error "getCollection requires accountId") unless accountId + requestSuccess = requestOptions.success @makeRequest _.extend requestOptions, path: "/#{collection}" accountId: accountId qs: params - returnsModel: true + returnsModel: false + success: (jsons) => + @_attachMetadataToResponse(jsons, requestOptions.metadataToAttach) + @_handleModelResponse(jsons) + if requestSuccess + requestSuccess(jsons) incrementRemoteChangeLock: (klass, id) -> @_lockTracker.increment(klass, id) @@ -385,14 +401,19 @@ class NylasAPI # the plugin server couldn't be reached or failed to respond properly when authing # the account, or that the Nylas API couldn't be reached. authPlugin: (pluginId, pluginName, accountOrId) -> - account = if accountOrId instanceof Account - accountOrId + unless @pluginsSupported + return Promise.reject(new Error('Sorry, this feature is only available when N1 is running against the hosted version of the Nylas Sync Engine.')) + + if accountOrId instanceof Account + account = accountOrId else AccountStore ?= require './stores/account-store' - AccountStore.accountForId(accountOrId) - Promise.reject(new Error('Invalid account')) unless account + account = AccountStore.accountForId(accountOrId) - cacheKey = "plugins.#{pluginId}.lastAuthTimestamp" + unless account + return Promise.reject(new Error('Invalid account')) + + cacheKey = "plugins.#{pluginId}.lastAuth.#{account.id}" if NylasEnv.config.get(cacheKey) return Promise.resolve() @@ -401,22 +422,24 @@ class NylasAPI method: "GET", accountId: account.id, path: "/auth/plugin?client_id=#{pluginId}" - }) - .then (result) => + + }).then (result) => if result.authed NylasEnv.config.set(cacheKey, Date.now()) return Promise.resolve() - else - # Enable to show a prompt to the user - # return @_requestPluginAuth(pluginName, account).then => - return @makeRequest({ - returnsModel: false, - method: "POST", - accountId: account.id, - path: "/auth/plugin", - body: {client_id: pluginId}, - json: true - }) + + # Enable to show a prompt to the user + # return @_requestPluginAuth(pluginName, account).then => + return @makeRequest({ + returnsModel: false, + method: "POST", + accountId: account.id, + path: "/auth/plugin", + body: {client_id: pluginId}, + json: true + }).then (result) => + NylasEnv.config.set(cacheKey, Date.now()) if result.authed + return Promise.resolve() _requestPluginAuth: (pluginName, account) -> {dialog} = require('electron').remote diff --git a/src/flux/stores/category-store.coffee b/src/flux/stores/category-store.coffee index 595d83376..318a3d159 100644 --- a/src/flux/stores/category-store.coffee +++ b/src/flux/stores/category-store.coffee @@ -32,7 +32,8 @@ class CategoryStore extends NylasStore .subscribe(@_onCategoriesChanged) byId: (accountOrId, categoryId) -> - @categories(accountOrId)[categoryId] + categories = @_categoryCache[asAccountId(accountOrId)] ? {} + categories[categoryId] # Public: Returns an array of all categories for an account, both # standard and user generated. The items returned by this function will be @@ -40,7 +41,7 @@ class CategoryStore extends NylasStore # categories: (accountOrId = null) -> if accountOrId - @_categoryCache[asAccountId(accountOrId)] ? {} + _.values(@_categoryCache[asAccountId(accountOrId)]) ? [] else all = [] for accountId, categories of @_categoryCache