-
Notifications
You must be signed in to change notification settings - Fork 168
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.
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
fix(queries): Ignore duplicate recent queries COMPASS-2237 #3171
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,16 +2,15 @@ import Reflux from 'reflux'; | |
import StateMixin from 'reflux-state-mixin'; | ||
import { remote } from 'electron'; | ||
import _ from 'lodash'; | ||
import { isDeepStrictEqual } from 'util'; | ||
|
||
import { formatQuery } from '../utils'; | ||
import { formatQuery, comparableQuery } from '../utils'; | ||
import { RecentQuery, RecentQueryCollection } from '../models'; | ||
import { createLoggerAndTelemetry } from '@mongodb-js/compass-logging'; | ||
const { track } = createLoggerAndTelemetry('COMPASS-QUERY-HISTORY-UI'); | ||
|
||
const TOTAL_RECENTS = 30; | ||
const ALLOWED = ['filter', 'project', 'sort', 'skip', 'limit', 'collation']; | ||
|
||
|
||
/** | ||
* Query History Recent List store. | ||
*/ | ||
|
@@ -25,7 +24,9 @@ const configureStore = (options = {}) => { | |
* Filter attributes that aren't query fields or have default/empty values. | ||
* @param {object} attributes | ||
*/ | ||
_filterDefaults(attributes) { | ||
_filterDefaults(_attributes) { | ||
// don't mutate the passed in parameter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lost some time here. My test that added the same query twice actually never executed the code that would check if it is a duplicate because this stripped ns from the query the first time, causing it to early return for a different reason. I think it is just better style to not implicitly modify things that came in from the outside. |
||
const attributes = _.clone(_attributes); | ||
for (const key in attributes) { | ||
if (attributes.hasOwnProperty(key)) { | ||
if (!attributes[key] || ALLOWED.indexOf(key) === -1) { | ||
|
@@ -35,6 +36,7 @@ const configureStore = (options = {}) => { | |
} | ||
} | ||
} | ||
return attributes; | ||
}, | ||
|
||
onConnected() { | ||
|
@@ -58,7 +60,7 @@ const configureStore = (options = {}) => { | |
const ns = recent.ns; | ||
|
||
/* Ignore empty or default queries */ | ||
this._filterDefaults(recent); | ||
recent = this._filterDefaults(recent); | ||
if (_.isEmpty(recent)) { | ||
return; | ||
} | ||
|
@@ -67,6 +69,15 @@ const configureStore = (options = {}) => { | |
return r._ns === ns; | ||
}); | ||
|
||
/* Ignore duplicate queries */ | ||
const existingQuery = this.state.items.find((item) => _.isEqual(comparableQuery(item), recent)); | ||
lerouxb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (existingQuery) { | ||
// update the existing query's lastExecuted to move it to the top | ||
existingQuery._lastExecuted = Date.now(); | ||
existingQuery.save(); | ||
return; | ||
} | ||
|
||
/* Keep length of each recent list to TOTAL_RECENTS */ | ||
if (filtered.length >= TOTAL_RECENTS) { | ||
const lastRecent = filtered[TOTAL_RECENTS - 1]; | ||
|
@@ -92,14 +103,8 @@ const configureStore = (options = {}) => { | |
}, | ||
|
||
runQuery(query) { | ||
// Loosely match queries against known history entries, because | ||
// currently we do not distinguish between favorites and recents | ||
// when running queries. This way, we do track some queries twice | ||
// (because there are more options than just .filter), but that | ||
// is probably fine as a temporary measure. | ||
// https://jira.mongodb.org/browse/COMPASS-5243 | ||
if (this.state.items.map(item => item.serialize()).some(item => { | ||
return isDeepStrictEqual(item.filter, query.filter); | ||
return _.isEqual(comparableQuery(item), query); | ||
})) { | ||
track('Query History Recent Used'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
function comparableQuery(item) { | ||
const query = {}; | ||
for (const [k, v] of Object.entries(item.serialize())) { | ||
if (k.startsWith('_')) { | ||
continue; | ||
} | ||
query[k] = v; | ||
} | ||
return query; | ||
} | ||
|
||
export default comparableQuery; | ||
export { comparableQuery }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import AppRegistry from 'hadron-app-registry'; | ||
import configureStore from '../../src/stores/recent-list-store'; | ||
import { comparableQuery } from './'; | ||
|
||
describe('comparableQuery', () => { | ||
let store; | ||
let appRegistry; | ||
|
||
beforeEach(() => { | ||
appRegistry = new AppRegistry(); | ||
store = configureStore({ localAppRegistry: appRegistry }); | ||
}); | ||
|
||
it('strips ampersand properties', () => { | ||
const recent = { ns: 'foo', filter: { foo: 1 } }; | ||
|
||
store.addRecent(recent); | ||
expect(store.state.items.length).to.equal(1); | ||
|
||
const query = store.state.items.at(0); | ||
|
||
// make sure it has the things we're going to strip | ||
const serialized = query.serialize(); | ||
expect(serialized).to.haveOwnProperty('_id'); | ||
expect(serialized).to.haveOwnProperty('_lastExecuted'); | ||
expect(serialized).to.haveOwnProperty('_ns'); | ||
|
||
expect(comparableQuery(query)).to.deep.equal({ | ||
filter: { foo: 1 } | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import formatQuery from './format-query'; | ||
import comparableQuery from './comparable-query'; | ||
import { FavoriteQueryStorage } from './favorite-query-storage'; | ||
|
||
export { formatQuery, FavoriteQueryStorage }; | ||
export { formatQuery, comparableQuery, FavoriteQueryStorage }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a second copy of this logic / a third place that could benefit from the new function.