Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Query.idMap to count ids, to be defensive against duplicate ids in query results #253

Merged
merged 4 commits into from
Jul 28, 2017
Merged

Change Query.idMap to count ids, to be defensive against duplicate ids in query results #253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d6b566
Change Query.idMap to count ids, to be defensive against duplicate id…
ericyhwang Jul 28, 2017
e4cc334
Switch query.idMap check to use "> 0"
ericyhwang Jul 28, 2017
34d63dc
Query#_addMapIds: Consolidate idMap update to one line
ericyhwang Jul 28, 2017
51673d7
Query#_removeMapIds: Refactoring to remove a conditional check
ericyhwang Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/Model/Query.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function Query(model, collectionName, expression, options) {
// because otherwise maybeUnload would be looping through the entire results
// set of each query on the same collection for every doc checked
//
// Map of id -> true
// Map of id -> count of ids
this.idMap = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that subscriptions.js does a plain property check on idMap instead of using hasOwnProperty, which means that the check will be incorrect if an id happens to be the name of an Object prototype property. Do you think that matters enough to fix? Racer-generated ids are UUIDs, so it's only a problem for manually-specified ids.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. The problem would result in a doc not getting locally cleaned up, so honestly, the impact isn't likely to be a concern. I'd probably not worry about it for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, since idMap is now a number, we could use the check idMap[id] > 0, which might be better since it is more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

}

Expand Down Expand Up @@ -293,7 +293,12 @@ Query.prototype._shareSubscribe = function(options, cb) {
Query.prototype._removeMapIds = function(ids) {
for (var i = ids.length; i--;) {
var id = ids[i];
delete this.idMap[id];
if (this.idMap[id] > 0) {
this.idMap[id]--;
}
if (this.idMap[id] === 0) {
delete this.idMap[id];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is slightly more clear to write:

if (this.idMap[id] > 1) {
  this.idMap[id]--;
} else {
  delete this.idMap[id];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two are not quite the same.

In your proposal, any ids that hit zero wouldn't get cleaned up in the map unless _removeMapIds happened to get called again with one of those zero-ed out ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, never mind. The condition is > 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is done now too, thanks for the suggestion! It's one fewer conditional check too, which is nice.

}
// Technically this isn't quite right and we might not wait the full unload
// delay if someone else calls maybeUnload for the same doc id. However,
Expand All @@ -312,7 +317,8 @@ Query.prototype._removeMapIds = function(ids) {
Query.prototype._addMapIds = function(ids) {
for (var i = ids.length; i--;) {
var id = ids[i];
this.idMap[id] = true;
this.idMap[id] = this.idMap[id] || 0;
this.idMap[id]++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equivalently, you can write this: this.idMap[id] = (this.idMap[id] || 0) + 1;

It is slightly more complexity on one line, and I don't even know if it makes a difference once optimized, but avoiding two different property gets + assignments might be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
};
Query.prototype._diffMapIds = function(ids) {
Expand Down
26 changes: 26 additions & 0 deletions test/Model/query.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var expect = require('../util').expect;
var racer = require('../../lib');
var Model = require('../../lib/Model');

describe('query', function() {
Expand All @@ -14,4 +15,29 @@ describe('query', function() {
expect(query.expression).eql([{x: null}, {x: {y: null, z: 0}}]);
});
});

describe('idMap', function() {
beforeEach('create in-memory backend and model', function() {
this.backend = racer.createBackend();
this.model = this.backend.createModel();
});
it('handles insert and remove of a duplicate id', function() {
var query = this.model.query('myCollection', {key: 'myVal'});
query.subscribe();
query.shareQuery.emit('insert', [
{id: 'a'},
{id: 'b'},
{id: 'c'},
], 0);
// Add and immediately remove a duplicate id.
query.shareQuery.emit('insert', [
{id: 'a'},
], 3);
query.shareQuery.emit('remove', [
{id: 'a'},
], 3);
// 'a' is still present once in the results, should still be in the map.
expect(query.idMap).to.only.have.keys(['a', 'b', 'c']);
});
});
});