From be6242a44cb8eaf490a6293b32e3a4353b034cf5 Mon Sep 17 00:00:00 2001 From: Juan Tejada Date: Sun, 21 Feb 2016 00:28:47 -0800 Subject: [PATCH] :art:(query-subscription): Update query subscription Summary: - Applies code changes that we discussed with @bengotow when debugging #1327 - Minor refactoring and reorganization + update specs Test Plan: - Unit tests Reviewers: evan, bengotow Reviewed By: bengotow Subscribers: bengotow Differential Revision: https://phab.nylas.com/D2612 --- spec/models/query-subscription-spec.coffee | 44 ++++++---- src/flux/models/query-subscription.coffee | 95 +++++++++++----------- 2 files changed, 79 insertions(+), 60 deletions(-) diff --git a/spec/models/query-subscription-spec.coffee b/spec/models/query-subscription-spec.coffee index 9dc25c5e9..c53132828 100644 --- a/spec/models/query-subscription-spec.coffee +++ b/spec/models/query-subscription-spec.coffee @@ -208,16 +208,16 @@ describe "QuerySubscription", -> describe "update", -> beforeEach -> - spyOn(QuerySubscription.prototype, '_fetchRange').andCallFake -> + spyOn(QuerySubscription.prototype, '_fetchMissingRange').andCallFake -> @_set ?= new MutableQueryResultSet() Promise.resolve() describe "when the query has an infinite range", -> - it "should call _fetchRange for the entire range", -> + it "should call _fetchMissingRange for the entire range", -> subscription = new QuerySubscription(DatabaseStore.findAll(Thread)) subscription.update() advanceClock() - expect(subscription._fetchRange).toHaveBeenCalledWith(QueryRange.infinite(), {entireModels: true, version: 1}) + expect(subscription._fetchMissingRange).toHaveBeenCalledWith(QueryRange.infinite(), {fetchEntireModels: true, version: 1}) it "should fetch full full models only when the previous set is empty", -> subscription = new QuerySubscription(DatabaseStore.findAll(Thread)) @@ -225,35 +225,51 @@ describe "QuerySubscription", -> subscription._set.addModelsInRange([new Thread()], new QueryRange(start: 0, end: 1)) subscription.update() advanceClock() - expect(subscription._fetchRange).toHaveBeenCalledWith(QueryRange.infinite(), {entireModels: false, version: 1}) + expect(subscription._fetchMissingRange).toHaveBeenCalledWith(QueryRange.infinite(), {fetchEntireModels: false, version: 1}) describe "when the query has a range", -> beforeEach -> @query = DatabaseStore.findAll(Thread).limit(10) describe "when we have no current range", -> - it "should call _fetchRange for the entire range and fetch full models", -> + it "should call _fetchMissingRange for the entire range and fetch full models", -> subscription = new QuerySubscription(@query) subscription._set = null subscription.update() advanceClock() - expect(subscription._fetchRange).toHaveBeenCalledWith(@query.range(), {entireModels: true, version: 1}) + expect(subscription._fetchMissingRange).toHaveBeenCalledWith(@query.range(), {fetchEntireModels: true, version: 1}) describe "when we have a previous range", -> - it "should call _fetchRange for the ranges representing the difference", -> - customRange1 = jasmine.createSpy('customRange1') - customRange2 = jasmine.createSpy('customRange2') - spyOn(QueryRange, 'rangesBySubtracting').andReturn [customRange1, customRange2] + it "should call _fetchMissingRange with the missingRange", -> + customRange = jasmine.createSpy('customRange1') + spyOn(QueryRange, 'rangesBySubtracting').andReturn [customRange] subscription = new QuerySubscription(@query) subscription._set = new MutableQueryResultSet() subscription._set.addModelsInRange([new Thread()], new QueryRange(start: 0, end: 1)) advanceClock() - subscription._fetchRange.reset() + subscription._fetchMissingRange.reset() subscription._updateInFlight = false subscription.update() advanceClock() - expect(subscription._fetchRange.callCount).toBe(2) - expect(subscription._fetchRange.calls[0].args).toEqual([customRange1, {entireModels: true, version: 1}]) - expect(subscription._fetchRange.calls[1].args).toEqual([customRange2, {entireModels: true, version: 1}]) + expect(subscription._fetchMissingRange.callCount).toBe(1) + expect(subscription._fetchMissingRange.calls[0].args).toEqual([customRange, {fetchEntireModels: true, version: 1}]) + + it "should call _fetchMissingRange for the entire query range when the missing range encompasses more than one range", -> + customRange1 = jasmine.createSpy('customRange1') + customRange2 = jasmine.createSpy('customRange2') + spyOn(QueryRange, 'rangesBySubtracting').andReturn [customRange1, customRange2] + + range = new QueryRange(start: 0, end: 1) + subscription = new QuerySubscription(@query) + subscription._set = new MutableQueryResultSet() + subscription._set.addModelsInRange([new Thread()], range) + + advanceClock() + subscription._fetchMissingRange.reset() + subscription._updateInFlight = false + subscription.update() + advanceClock() + expect(subscription._fetchMissingRange.callCount).toBe(1) + expect(subscription._fetchMissingRange.calls[0].args).toEqual([@query.range(), {fetchEntireModels: true, version: 1}]) diff --git a/src/flux/models/query-subscription.coffee b/src/flux/models/query-subscription.coffee index 20908281a..93eab2f79 100644 --- a/src/flux/models/query-subscription.coffee +++ b/src/flux/models/query-subscription.coffee @@ -59,6 +59,10 @@ class QuerySubscription @_queuedChangeRecords.push(record) @_processChangeRecords() unless @_updateInFlight + cancelPendingUpdate: => + @_queryVersion += 1 + @_updateInFlight = false + # Scan through change records and apply them to the last result set. # - Returns true if changes did / will result in new result set being created. # - Returns false if no changes were made. @@ -122,7 +126,7 @@ class QuerySubscription return true else if knownImpacts > 0 @_createResultAndTrigger() - return true + return false else return false @@ -138,71 +142,70 @@ class QuerySubscription return false update: => - desiredRange = @_query.range() - currentRange = @_set?.range() @_updateInFlight = true version = @_queryVersion + desiredRange = @_query.range() + currentRange = @_set?.range() + areNotInfinite = currentRange and not currentRange.isInfinite() and not desiredRange.isInfinite() + previousResultIsEmpty = not @_set or @_set.modelCacheCount() is 0 + missingRange = @_getMissingRange(desiredRange, currentRange) + fetchEntireModels = if areNotInfinite then true else previousResultIsEmpty + @_fetchMissingRange(missingRange, {version, fetchEntireModels}) + + _getMissingRange: (desiredRange, currentRange) => if currentRange and not currentRange.isInfinite() and not desiredRange.isInfinite() ranges = QueryRange.rangesBySubtracting(desiredRange, currentRange) - entireModels = true + missingRange = if ranges.length > 1 then desiredRange else ranges[0] else - ranges = [desiredRange] - entireModels = not @_set or @_set.modelCacheCount() is 0 + missingRange = desiredRange + return missingRange - Promise.each ranges, (range) => - return unless @_queryVersion is version - @_fetchRange(range, {entireModels, version}) - - .then => - return unless @_queryVersion is version - ids = @_set.ids().filter (id) => not @_set.modelWithId(id) - return if ids.length is 0 - return DatabaseStore.findAll(@_query._klass, {id: ids}).then (models) => - return unless @_queryVersion is version - @_set.replaceModel(m) for m in models - - .then => - return unless @_queryVersion is version - @_updateInFlight = false - - # Trigger if A) no changes came in during the update, or B) applying - # those changes has no effect on the result set, and this one is - # still good. - if @_queuedChangeRecords.length is 0 or not @_processChangeRecords() - @_createResultAndTrigger() - - cancelPendingUpdate: => - @_queryVersion += 1 - @_updateInFlight = false - - _fetchRange: (range, {entireModels, version} = {}) -> - rangeQuery = undefined - - unless range.isInfinite() + _getQueryForRange: (range, fetchEntireModels) => + rangeQuery = null + if not range.isInfinite() rangeQuery ?= @_query.clone() rangeQuery.offset(range.offset).limit(range.limit) - - unless entireModels + if not fetchEntireModels rangeQuery ?= @_query.clone() rangeQuery.idsOnly() - rangeQuery ?= @_query + return rangeQuery - DatabaseStore.run(rangeQuery, {format: false}).then (results) => + _fetchMissingRange: (missingRange, {version, fetchEntireModels}) -> + missingRangeQuery = @_getQueryForRange(missingRange, fetchEntireModels) + + DatabaseStore.run(missingRangeQuery, {format: false}) + .then (results) => return unless @_queryVersion is version - if @_set and not @_set.range().isContiguousWith(range) + if @_set and not @_set.range().isContiguousWith(missingRange) @_set = null @_set ?= new MutableQueryResultSet() - if entireModels - @_set.addModelsInRange(results, range) - else - @_set.addIdsInRange(results, range) + # Create result and trigger + # if A) no changes have come in during querying the missing range, + # or B) applying those changes has no effect on the result set, and this one is + # still good. + if @_queuedChangeRecords.length is 0 or @_processChangeRecords() is false + if fetchEntireModels + @_set.addModelsInRange(results, missingRange) + else + @_set.addIdsInRange(results, missingRange) - @_set.clipToRange(@_query.range()) + @_set.clipToRange(@_query.range()) + + ids = @_set.ids().filter (id) => not @_set.modelWithId(id) + if ids.length > 0 + return DatabaseStore.findAll(@_query._klass, {id: ids}).then (models) => + return unless @_queryVersion is version + @_set.replaceModel(m) for m in models + @_updateInFlight = false + @_createResultAndTrigger() + else + @_updateInFlight = false + @_createResultAndTrigger() _createResultAndTrigger: => allCompleteModels = @_set.isComplete()