From 752d0dbce13cb50458a147bbde99edac751ca4f3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 18 Sep 2024 16:02:54 -0600 Subject: [PATCH 01/24] Update the AutocompleteCache - Allow manually refreshing the API data - Allow mutating data prior to it being stored in the cache - Allow formatting the data after it's extracted from the cache - ALWAYS retrieve from the cache (even on the first query) Why: - Sometimes, the data in a cache can get stale, and a page might need the ability to refresh it without waiting for a full page refresh. - Sometimes, consumers of the data from the AutocompleteCache have expectations about the structure of the data, so there needs to be a way to change what's stored so when it's retrieved consumer expectations are met. - Similarly, sometimes a consumer has unreasonable expectations about the format of data that it expects. Example: a consumer _requires_ being fed the raw output of a network fetch, and expects everything nested under the { "data": ... } key. Storing the data in this format would make it difficult to search properly, so we can instead format to this structure when the data is retrieved. - Finally, it's best to avoid multiple possible code execution paths and multiple potential sources of truth. By always retrieving from the cache (even on the first query when a network request is made), we can guarantee that each step is run appropriately, and just once. --- .../javascripts/issues/dashboard/utils.js | 61 ++++++++++++++++--- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/issues/dashboard/utils.js b/app/assets/javascripts/issues/dashboard/utils.js index 6fa95b38649944..24ad4827886ff5 100644 --- a/app/assets/javascripts/issues/dashboard/utils.js +++ b/app/assets/javascripts/issues/dashboard/utils.js @@ -5,19 +5,60 @@ import axios from '~/lib/utils/axios_utils'; export class AutocompleteCache { constructor() { this.cache = {}; + this.sources = new Map(); + this.mutators = new Map(); + this.formatters = new Map(); + this.searchProperties = new Map(); } - fetch({ url, cacheName, searchProperty, search }) { - if (this.cache[cacheName]) { - const data = search - ? fuzzaldrinPlus.filter(this.cache[cacheName], search, { key: searchProperty }) - : this.cache[cacheName].slice(0, MAX_LIST_SIZE); - return Promise.resolve(data); + setUpCache({ url, name, property, mutator, formatter }){ + this.sources.set(name, url); + this.mutators.set(name, mutator); + this.formatters.set(name, formatter); + this.searchProperties.set(name, property); + } + + async fetch({ url, cacheName, searchProperty, search, mutator, formatter }) { + const hasCache = Boolean(this.cache[cacheName]); + let finalData; + + this.setUpCache({ url, name: cacheName, property: searchProperty, mutator, formatter }); + + if (!hasCache) { + await this.updateLocalCache(cacheName); + } + + finalData = this.retrieveFromLocalCache(cacheName, search); + + return finalData; + } + + async updateLocalCache(name){ + const url = this.sources.get(name); + const mutator = this.mutators.get(name); + + return await axios + .get(url) + .then(({ data }) => { + if(mutator){ + data = mutator(data); + } + + this.cache[name] = data; + }); + } + + retrieveFromLocalCache(name, search){ + const searchProperty = this.searchProperties.get(name); + const formatter = this.formatters.get(name); + let result = search + ? fuzzaldrinPlus.filter(this.cache[name], search, { key: searchProperty }) + : this.cache[name].slice(0, MAX_LIST_SIZE) + + if( formatter ){ + result = formatter( result ); } - return axios.get(url).then(({ data }) => { - this.cache[cacheName] = data; - return data.slice(0, MAX_LIST_SIZE); - }); + return result; } } -- GitLab From a00f056c20049fc6104510698c0d5a9a2d1912d3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 18 Sep 2024 16:05:56 -0600 Subject: [PATCH 02/24] Use the AutocompleteCache to fetch branches --- .../components/merge_requests_list_app.vue | 48 +++++++++++++------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 09adf5a019f299..068a4d29df9a99 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -2,16 +2,15 @@ import { GlFilteredSearchToken, GlButton, GlLink, GlIcon, GlTooltipDirective } from '@gitlab/ui'; import { isEmpty } from 'lodash'; import ApprovalCount from 'ee_else_ce/merge_requests/components/approval_count.vue'; -import { createAlert } from '~/alert'; import Api from '~/api'; import * as Sentry from '~/sentry/sentry_browser_wrapper'; import { STATUS_ALL, STATUS_CLOSED, STATUS_OPEN, STATUS_MERGED } from '~/issues/constants'; import { fetchPolicies } from '~/lib/graphql'; import { isPositiveInteger } from '~/lib/utils/number_utils'; import { scrollUp } from '~/lib/utils/scroll_utils'; -import { getParameterByName } from '~/lib/utils/url_utility'; +import { getParameterByName, mergeUrlParams } from '~/lib/utils/url_utility'; import { TYPENAME_USER } from '~/graphql_shared/constants'; -import { convertToGraphQLId } from '~/graphql_shared/utils'; +import { convertToGraphQLId, getIdFromGraphQLId } from '~/graphql_shared/utils'; import IssuableList from '~/vue_shared/issuable/list/components/issuable_list_root.vue'; import IssuableMilestone from '~/vue_shared/issuable/list/components/issuable_milestone.vue'; import { DEFAULT_PAGE_SIZE, mergeRequestListTabs } from '~/vue_shared/issuable/list/constants'; @@ -129,6 +128,7 @@ export default { }, data() { return { + projectId: null, filterTokens: [], mergeRequests: [], mergeRequestCounts: {}, @@ -157,6 +157,7 @@ export default { if (!data) { return; } + this.projectId = getIdFromGraphQLId(data.project.id); this.pageInfo = data.project.mergeRequests?.pageInfo ?? {}; }, error(error) { @@ -336,7 +337,7 @@ export default { operators: OPERATORS_IS, fullPath: this.fullPath, isProject: true, - fetchBranches: this.fetchBranches, + fetchBranches: this.fetchTargetBranches, }, { type: TOKEN_TYPE_SOURCE_BRANCH, @@ -346,7 +347,7 @@ export default { operators: OPERATORS_IS, fullPath: this.fullPath, isProject: true, - fetchBranches: this.fetchBranches, + fetchBranches: this.fetchSourceBranches, }, { type: TOKEN_TYPE_LABEL, @@ -472,16 +473,33 @@ export default { issuableEventHub.$off('issuables:toggleBulkEdit', this.toggleBulkEditSidebar); }, methods: { - fetchBranches(search) { - return Api.branches(this.fullPath, search) - .then((response) => { - return response; - }) - .catch(() => { - createAlert({ - message: this.$options.i18n.errorFetchingBranches, - }); - }); + fetchBranches(type, search) { + const cacheName = `${type}Branches`; + const typeUrls = { + "source": '/-/autocomplete/merge_request_source_branches.json', + "target": '/-/autocomplete/merge_request_target_branches.json', + "*": Api + .buildUrl(Api.createBranchPath) + .replace(':id', encodeURIComponent(this.fullPath)) + } + const branchPath = mergeUrlParams({ + project_id: this.projectId + }, typeUrls[ type ] || typeUrls[ "*" ]); + + return this.autocompleteCache.fetch({ + mutator: ( branchList ) => branchList.map( ( branch, index ) => ({ ...branch, name: branch.title, id: index }) ), + formatter: ( results ) => ({ data: results }), + url: branchPath, + searchProperty: 'name', + cacheName, + search, + }) + }, + fetchTargetBranches(search){ + return this.fetchBranches('target', search); + }, + fetchSourceBranches(search){ + return this.fetchBranches('source', search); }, fetchEmojis(search) { return this.autocompleteCache.fetch({ -- GitLab From a56be11a378907766170aa7ce1a021bab85c348f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 18 Sep 2024 16:06:37 -0600 Subject: [PATCH 03/24] Manually force the AutocompleteCache to update without a page refresh --- .../list/components/merge_requests_list_app.vue | 9 ++++++++- app/assets/javascripts/merge_requests/list/constants.js | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 068a4d29df9a99..91fdebad9e3bc8 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -72,7 +72,7 @@ import { import CiIcon from '~/vue_shared/components/ci_icon/ci_icon.vue'; import setSortPreferenceMutation from '~/issues/list/queries/set_sort_preference.mutation.graphql'; import issuableEventHub from '~/issues/list/eventhub'; -import { i18n } from '../constants'; +import { i18n, BRANCH_LIST_REFRESH_INTERVAL } from '../constants'; import getMergeRequestsQuery from '../queries/get_merge_requests.query.graphql'; import getMergeRequestsCountsQuery from '../queries/get_merge_requests_counts.query.graphql'; import searchLabelsQuery from '../queries/search_labels.query.graphql'; @@ -129,6 +129,7 @@ export default { data() { return { projectId: null, + branchRefreshTimer: null, filterTokens: [], mergeRequests: [], mergeRequestCounts: {}, @@ -486,6 +487,12 @@ export default { project_id: this.projectId }, typeUrls[ type ] || typeUrls[ "*" ]); + if(this.branchRefreshTimer){ + clearInterval(this.branchRefreshTimer); + } + + this.branchRefreshTimer = setInterval(() => this.autocompleteCache.updateLocalCache(cacheName), BRANCH_LIST_REFRESH_INTERVAL); + return this.autocompleteCache.fetch({ mutator: ( branchList ) => branchList.map( ( branch, index ) => ({ ...branch, name: branch.title, id: index }) ), formatter: ( results ) => ({ data: results }), diff --git a/app/assets/javascripts/merge_requests/list/constants.js b/app/assets/javascripts/merge_requests/list/constants.js index 1fb431426dd2ec..04394a904fddf4 100644 --- a/app/assets/javascripts/merge_requests/list/constants.js +++ b/app/assets/javascripts/merge_requests/list/constants.js @@ -12,3 +12,5 @@ export const i18n = { downvots: __('Downvotes'), newMergeRequest: __('New merge request'), }; + +export const BRANCH_LIST_REFRESH_INTERVAL = 600_000; // 10 minutes (* 60 seconds, * 1000 milliseconds) -- GitLab From b01cdbf7018c0c0f8d8f15b8e34060a6058ca221 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 19 Sep 2024 01:23:06 -0600 Subject: [PATCH 04/24] Fix syntax style issues from Prettier --- .../javascripts/issues/dashboard/utils.js | 26 ++++++------- .../components/merge_requests_list_app.vue | 37 +++++++++++-------- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/app/assets/javascripts/issues/dashboard/utils.js b/app/assets/javascripts/issues/dashboard/utils.js index 24ad4827886ff5..8e94b49ca23df2 100644 --- a/app/assets/javascripts/issues/dashboard/utils.js +++ b/app/assets/javascripts/issues/dashboard/utils.js @@ -11,7 +11,7 @@ export class AutocompleteCache { this.searchProperties = new Map(); } - setUpCache({ url, name, property, mutator, formatter }){ + setUpCache({ url, name, property, mutator, formatter }) { this.sources.set(name, url); this.mutators.set(name, mutator); this.formatters.set(name, formatter); @@ -33,30 +33,28 @@ export class AutocompleteCache { return finalData; } - async updateLocalCache(name){ + async updateLocalCache(name) { const url = this.sources.get(name); const mutator = this.mutators.get(name); - return await axios - .get(url) - .then(({ data }) => { - if(mutator){ - data = mutator(data); - } + return await axios.get(url).then(({ data }) => { + if (mutator) { + data = mutator(data); + } - this.cache[name] = data; - }); + this.cache[name] = data; + }); } - retrieveFromLocalCache(name, search){ + retrieveFromLocalCache(name, search) { const searchProperty = this.searchProperties.get(name); const formatter = this.formatters.get(name); let result = search ? fuzzaldrinPlus.filter(this.cache[name], search, { key: searchProperty }) - : this.cache[name].slice(0, MAX_LIST_SIZE) + : this.cache[name].slice(0, MAX_LIST_SIZE); - if( formatter ){ - result = formatter( result ); + if (formatter) { + result = formatter(result); } return result; diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 91fdebad9e3bc8..a402144c26f5a1 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -477,35 +477,40 @@ export default { fetchBranches(type, search) { const cacheName = `${type}Branches`; const typeUrls = { - "source": '/-/autocomplete/merge_request_source_branches.json', - "target": '/-/autocomplete/merge_request_target_branches.json', - "*": Api - .buildUrl(Api.createBranchPath) - .replace(':id', encodeURIComponent(this.fullPath)) - } - const branchPath = mergeUrlParams({ - project_id: this.projectId - }, typeUrls[ type ] || typeUrls[ "*" ]); + source: '/-/autocomplete/merge_request_source_branches.json', + target: '/-/autocomplete/merge_request_target_branches.json', + '*': Api.buildUrl(Api.createBranchPath).replace(':id', encodeURIComponent(this.fullPath)), + }; + const branchPath = mergeUrlParams( + { + project_id: this.projectId, + }, + typeUrls[type] || typeUrls['*'], + ); - if(this.branchRefreshTimer){ + if (this.branchRefreshTimer) { clearInterval(this.branchRefreshTimer); } - this.branchRefreshTimer = setInterval(() => this.autocompleteCache.updateLocalCache(cacheName), BRANCH_LIST_REFRESH_INTERVAL); + this.branchRefreshTimer = setInterval( + () => this.autocompleteCache.updateLocalCache(cacheName), + BRANCH_LIST_REFRESH_INTERVAL, + ); return this.autocompleteCache.fetch({ - mutator: ( branchList ) => branchList.map( ( branch, index ) => ({ ...branch, name: branch.title, id: index }) ), - formatter: ( results ) => ({ data: results }), + mutator: (branchList) => + branchList.map((branch, index) => ({ ...branch, name: branch.title, id: index })), + formatter: (results) => ({ data: results }), url: branchPath, searchProperty: 'name', cacheName, search, - }) + }); }, - fetchTargetBranches(search){ + fetchTargetBranches(search) { return this.fetchBranches('target', search); }, - fetchSourceBranches(search){ + fetchSourceBranches(search) { return this.fetchBranches('source', search); }, fetchEmojis(search) { -- GitLab From 73fe8d6b7f21268462c6b39cd2e6b0bdc3b4ac14 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 19 Sep 2024 01:42:36 -0600 Subject: [PATCH 05/24] Fix linting issues --- app/assets/javascripts/issues/dashboard/utils.js | 13 ++++++------- .../list/components/merge_requests_list_app.vue | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/issues/dashboard/utils.js b/app/assets/javascripts/issues/dashboard/utils.js index 8e94b49ca23df2..ea213c6a373f5b 100644 --- a/app/assets/javascripts/issues/dashboard/utils.js +++ b/app/assets/javascripts/issues/dashboard/utils.js @@ -20,7 +20,6 @@ export class AutocompleteCache { async fetch({ url, cacheName, searchProperty, search, mutator, formatter }) { const hasCache = Boolean(this.cache[cacheName]); - let finalData; this.setUpCache({ url, name: cacheName, property: searchProperty, mutator, formatter }); @@ -28,21 +27,21 @@ export class AutocompleteCache { await this.updateLocalCache(cacheName); } - finalData = this.retrieveFromLocalCache(cacheName, search); - - return finalData; + return this.retrieveFromLocalCache(cacheName, search); } async updateLocalCache(name) { const url = this.sources.get(name); const mutator = this.mutators.get(name); - return await axios.get(url).then(({ data }) => { + return axios.get(url).then(({ data }) => { + let finalData = data; + if (mutator) { - data = mutator(data); + finalData = mutator(finalData); } - this.cache[name] = data; + this.cache[name] = finalData; }); } diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index a402144c26f5a1..27c2b6449cea6c 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -475,7 +475,7 @@ export default { }, methods: { fetchBranches(type, search) { - const cacheName = `${type}Branches`; + const cacheName = `${type}Branches`; // eslint-disable-line @gitlab/require-i18n-strings const typeUrls = { source: '/-/autocomplete/merge_request_source_branches.json', target: '/-/autocomplete/merge_request_target_branches.json', -- GitLab From 51857e2bef59df3f170eb3711d8776d06bfa9979 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 11:33:37 -0600 Subject: [PATCH 06/24] Move the AutocompleteCache to be a Merge Request-specific utility - Allow manually refreshing the API data - Allow mutating data prior to it being stored in the cache - Allow formatting the data after it's extracted from the cache - ALWAYS retrieve from the cache (even on the first query) Why: - Sometimes, the data in a cache can get stale, and a page might need the ability to refresh it without waiting for a full page refresh. - Sometimes, consumers of the data from the AutocompleteCache have expectations about the structure of the data, so there needs to be a way to change what's stored so when it's retrieved consumer expectations are met. - Similarly, sometimes a consumer has unreasonable expectations about the format of data that it expects. Example: a consumer _requires_ being fed the raw output of a network fetch, and expects everything nested under the { "data": ... } key. Storing the data in this format would make it difficult to search properly, so we can instead format to this structure when the data is retrieved. - Finally, it's best to avoid multiple possible code execution paths and multiple potential sources of truth. By always retrieving from the cache (even on the first query when a network request is made), we can guarantee that each step is run appropriately, and just once. --- .../javascripts/issues/dashboard/utils.js | 54 +++--------------- .../javascripts/merge_requests/constants.js | 1 + .../components/merge_requests_list_app.vue | 6 +- .../utils/autocomplete_cache.js | 57 +++++++++++++++++++ 4 files changed, 68 insertions(+), 50 deletions(-) create mode 100644 app/assets/javascripts/merge_requests/constants.js create mode 100644 app/assets/javascripts/merge_requests/utils/autocomplete_cache.js diff --git a/app/assets/javascripts/issues/dashboard/utils.js b/app/assets/javascripts/issues/dashboard/utils.js index ea213c6a373f5b..6fa95b38649944 100644 --- a/app/assets/javascripts/issues/dashboard/utils.js +++ b/app/assets/javascripts/issues/dashboard/utils.js @@ -5,57 +5,19 @@ import axios from '~/lib/utils/axios_utils'; export class AutocompleteCache { constructor() { this.cache = {}; - this.sources = new Map(); - this.mutators = new Map(); - this.formatters = new Map(); - this.searchProperties = new Map(); } - setUpCache({ url, name, property, mutator, formatter }) { - this.sources.set(name, url); - this.mutators.set(name, mutator); - this.formatters.set(name, formatter); - this.searchProperties.set(name, property); - } - - async fetch({ url, cacheName, searchProperty, search, mutator, formatter }) { - const hasCache = Boolean(this.cache[cacheName]); - - this.setUpCache({ url, name: cacheName, property: searchProperty, mutator, formatter }); - - if (!hasCache) { - await this.updateLocalCache(cacheName); + fetch({ url, cacheName, searchProperty, search }) { + if (this.cache[cacheName]) { + const data = search + ? fuzzaldrinPlus.filter(this.cache[cacheName], search, { key: searchProperty }) + : this.cache[cacheName].slice(0, MAX_LIST_SIZE); + return Promise.resolve(data); } - return this.retrieveFromLocalCache(cacheName, search); - } - - async updateLocalCache(name) { - const url = this.sources.get(name); - const mutator = this.mutators.get(name); - return axios.get(url).then(({ data }) => { - let finalData = data; - - if (mutator) { - finalData = mutator(finalData); - } - - this.cache[name] = finalData; + this.cache[cacheName] = data; + return data.slice(0, MAX_LIST_SIZE); }); } - - retrieveFromLocalCache(name, search) { - const searchProperty = this.searchProperties.get(name); - const formatter = this.formatters.get(name); - let result = search - ? fuzzaldrinPlus.filter(this.cache[name], search, { key: searchProperty }) - : this.cache[name].slice(0, MAX_LIST_SIZE); - - if (formatter) { - result = formatter(result); - } - - return result; - } } diff --git a/app/assets/javascripts/merge_requests/constants.js b/app/assets/javascripts/merge_requests/constants.js new file mode 100644 index 00000000000000..83293ad5a364c2 --- /dev/null +++ b/app/assets/javascripts/merge_requests/constants.js @@ -0,0 +1 @@ +export const MAX_LIST_SIZE = 10; diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 27c2b6449cea6c..3c8de698e9eae9 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -50,7 +50,6 @@ import { TOKEN_TYPE_ENVIRONMENT, TOKEN_TITLE_ENVIRONMENT, } from '~/vue_shared/components/filtered_search_bar/constants'; -import { AutocompleteCache } from '~/issues/dashboard/utils'; import { convertToApiParams, convertToSearchQuery, @@ -72,6 +71,7 @@ import { import CiIcon from '~/vue_shared/components/ci_icon/ci_icon.vue'; import setSortPreferenceMutation from '~/issues/list/queries/set_sort_preference.mutation.graphql'; import issuableEventHub from '~/issues/list/eventhub'; +import { AutocompleteCache } from '../../utils/autocomplete_cache'; import { i18n, BRANCH_LIST_REFRESH_INTERVAL } from '../constants'; import getMergeRequestsQuery from '../queries/get_merge_requests.query.graphql'; import getMergeRequestsCountsQuery from '../queries/get_merge_requests_counts.query.graphql'; @@ -475,7 +475,6 @@ export default { }, methods: { fetchBranches(type, search) { - const cacheName = `${type}Branches`; // eslint-disable-line @gitlab/require-i18n-strings const typeUrls = { source: '/-/autocomplete/merge_request_source_branches.json', target: '/-/autocomplete/merge_request_target_branches.json', @@ -493,7 +492,7 @@ export default { } this.branchRefreshTimer = setInterval( - () => this.autocompleteCache.updateLocalCache(cacheName), + () => this.autocompleteCache.updateLocalCache(branchPath), BRANCH_LIST_REFRESH_INTERVAL, ); @@ -503,7 +502,6 @@ export default { formatter: (results) => ({ data: results }), url: branchPath, searchProperty: 'name', - cacheName, search, }); }, diff --git a/app/assets/javascripts/merge_requests/utils/autocomplete_cache.js b/app/assets/javascripts/merge_requests/utils/autocomplete_cache.js new file mode 100644 index 00000000000000..70733670998c18 --- /dev/null +++ b/app/assets/javascripts/merge_requests/utils/autocomplete_cache.js @@ -0,0 +1,57 @@ +import fuzzaldrinPlus from 'fuzzaldrin-plus'; +import axios from '~/lib/utils/axios_utils'; + +import { MAX_LIST_SIZE } from '../constants'; + +export class AutocompleteCache { + constructor() { + this.cache = new Map(); + this.mutators = new Map(); + this.formatters = new Map(); + this.searchProperties = new Map(); + } + + setUpCache({ url, property, mutator, formatter }) { + this.mutators.set(url, mutator); + this.formatters.set(url, formatter); + this.searchProperties.set(url, property); + } + + async fetch({ url, searchProperty, search, mutator, formatter }) { + this.setUpCache({ url, property: searchProperty, mutator, formatter }); + + if (!this.cache.has(url)) { + await this.updateLocalCache(url); + } + + return this.retrieveFromLocalCache(url, search); + } + + async updateLocalCache(url) { + const mutator = this.mutators.get(url); + + return axios.get(url).then(({ data }) => { + let finalData = data; + + if (mutator) { + finalData = mutator(finalData); + } + + this.cache.set(url, finalData); + }); + } + + retrieveFromLocalCache(url, search) { + const searchProperty = this.searchProperties.get(url); + const formatter = this.formatters.get(url); + let result = search + ? fuzzaldrinPlus.filter(this.cache.get(url), search, { key: searchProperty }) + : this.cache.get(url).slice(0, MAX_LIST_SIZE); + + if (formatter) { + result = formatter(result); + } + + return result; + } +} -- GitLab From 0a4d1bf593281e78cb49bd836c51d136117f59fa Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 13:57:35 -0600 Subject: [PATCH 07/24] Test the Merge Request AutocompleteCache --- .../utils/autocomplete_cache_spec.js | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 spec/frontend/merge_requests/utils/autocomplete_cache_spec.js diff --git a/spec/frontend/merge_requests/utils/autocomplete_cache_spec.js b/spec/frontend/merge_requests/utils/autocomplete_cache_spec.js new file mode 100644 index 00000000000000..1eae23c4cc9264 --- /dev/null +++ b/spec/frontend/merge_requests/utils/autocomplete_cache_spec.js @@ -0,0 +1,170 @@ +import AxiosMockAdapter from 'axios-mock-adapter'; +import fuzzaldrinPlus from 'fuzzaldrin-plus'; +import { AutocompleteCache } from '~/merge_requests/utils/autocomplete_cache'; +import { MAX_LIST_SIZE } from '~/merge_requests/constants'; +import axios from '~/lib/utils/axios_utils'; +import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; + +function mutator(data) { + return data.map((entry, index) => ({ ...entry, id: index })); +} + +function formatter(data) { + return { + data, + }; +} + +describe('AutocompleteCache', () => { + const searchProperty = 'property'; + const url = 'url'; + const data = [ + { [searchProperty]: 'one' }, + { [searchProperty]: 'two' }, + { [searchProperty]: 'three' }, + { [searchProperty]: 'four' }, + { [searchProperty]: 'five' }, + { [searchProperty]: 'six' }, + { [searchProperty]: 'seven' }, + { [searchProperty]: 'eight' }, + { [searchProperty]: 'nine' }, + { [searchProperty]: 'ten' }, + { [searchProperty]: 'eleven' }, + { [searchProperty]: 'twelve' }, + { [searchProperty]: 'thirteen' }, + { [searchProperty]: 'fourteen' }, + { [searchProperty]: 'fifteen' }, + ]; + let autocompleteCache; + let axiosMock; + + beforeEach(() => { + autocompleteCache = new AutocompleteCache(); + axiosMock = new AxiosMockAdapter(axios); + }); + + afterEach(() => { + axiosMock.reset(); + }); + + describe('when there is no cached data', () => { + let response; + + beforeEach(async () => { + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, data); + response = await autocompleteCache.fetch({ url, searchProperty }); + }); + + it('fetches items via the API', () => { + expect(axiosMock.history.get[0].url).toBe(url); + }); + + it(`returns a maximum of ${MAX_LIST_SIZE} items`, () => { + expect(response).toHaveLength(MAX_LIST_SIZE); + }); + }); + + describe('when there is cached data', () => { + let response; + + beforeEach(async () => { + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, data); + jest.spyOn(fuzzaldrinPlus, 'filter'); + // Populate cache + await autocompleteCache.fetch({ url, searchProperty }); + // Execute filtering on cache data + response = await autocompleteCache.fetch({ url, searchProperty, search: 'een' }); + }); + + it('returns filtered items based on search characters', () => { + expect(response).toEqual([ + { [searchProperty]: 'fifteen' }, + { [searchProperty]: 'thirteen' }, + { [searchProperty]: 'fourteen' }, + { [searchProperty]: 'eleven' }, + { [searchProperty]: 'seven' }, + ]); + }); + + it('filters using fuzzaldrinPlus', () => { + expect(fuzzaldrinPlus.filter).toHaveBeenCalled(); + }); + + it('does not call the API', () => { + expect(axiosMock.history.get[1]).toBeUndefined(); + }); + }); + + describe('refreshing the local cache', () => { + const updatedData = [ + { [searchProperty]: 'one' }, + { [searchProperty]: 'two' }, + { [searchProperty]: 'three' }, + ]; + + beforeEach(async () => { + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, data); + // Populate cache + await autocompleteCache.fetch({ url, searchProperty }); + // Reduced entries later... + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, updatedData); + }); + + it('overwrites the cache with the new data from the endpoint', async () => { + // Initially confirm the cache was hydrated + expect(autocompleteCache.cache.get(url).length).toBe(data.length); + + await autocompleteCache.updateLocalCache(url); + + expect(autocompleteCache.cache.get(url).length).toBe(updatedData.length); + }); + }); + + describe('mutators', () => { + beforeEach(() => { + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, data); + }); + + it('does not touch the data if no mutator is provided', async () => { + await autocompleteCache.updateLocalCache(url); + + expect(autocompleteCache.cache.get(url)).toBe(data); + }); + + it('modifies data before storing it if a mutator is provided', async () => { + autocompleteCache.setUpCache({ url, mutator }); + + await autocompleteCache.updateLocalCache(url); + + expect(autocompleteCache.cache.get(url)).not.toBe(data); + expect(autocompleteCache.cache.get(url)[0]).toEqual({ ...data[0], id: 0 }); + }); + }); + + describe('formatters', () => { + const expectedOutput = data.slice(0, MAX_LIST_SIZE); + + beforeEach(async () => { + axiosMock.onGet(url).replyOnce(HTTP_STATUS_OK, data); + + await autocompleteCache.updateLocalCache(url); + }); + + it('returns the data directly from the cache if no formatter is provided', () => { + expect(autocompleteCache.retrieveFromLocalCache(url)).toEqual(expectedOutput); + }); + + it('modifies the data before returning it if a formatter is provided', () => { + autocompleteCache.setUpCache({ url, formatter }); + + expect(autocompleteCache.retrieveFromLocalCache(url)).toEqual({ data: expectedOutput }); + }); + + it('does not modify the source (cached) data at all if a formatter is provided', () => { + autocompleteCache.setUpCache({ url, formatter }); + autocompleteCache.retrieveFromLocalCache(url); + + expect(autocompleteCache.cache.get(url)).toBe(data); + }); + }); +}); -- GitLab From 2db2ff9c858320d4c5e2462f7ac2818e137fd757 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 14:45:00 -0600 Subject: [PATCH 08/24] Never try to request from the source/target APIs without a projectId --- .../list/components/merge_requests_list_app.vue | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 3c8de698e9eae9..257de48fdc37d6 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -480,12 +480,9 @@ export default { target: '/-/autocomplete/merge_request_target_branches.json', '*': Api.buildUrl(Api.createBranchPath).replace(':id', encodeURIComponent(this.fullPath)), }; - const branchPath = mergeUrlParams( - { - project_id: this.projectId, - }, - typeUrls[type] || typeUrls['*'], - ); + const url = typeUrls[type]; + const branchPath = + url && this.projectId ? mergeUrlParams({ project_id: this.projectId }, url) : typeUrls['*']; if (this.branchRefreshTimer) { clearInterval(this.branchRefreshTimer); -- GitLab From 60609aa030da66dac727815cc7cf5a99bac836de Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 15:54:36 -0600 Subject: [PATCH 09/24] Set a refresh interval for each endpoint separately --- .../list/components/merge_requests_list_app.vue | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 257de48fdc37d6..5137ee6d532d7c 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -129,7 +129,7 @@ export default { data() { return { projectId: null, - branchRefreshTimer: null, + branchRefreshTimers: {}, filterTokens: [], mergeRequests: [], mergeRequestCounts: {}, @@ -474,21 +474,24 @@ export default { issuableEventHub.$off('issuables:toggleBulkEdit', this.toggleBulkEditSidebar); }, methods: { - fetchBranches(type, search) { + fetchBranches(type = 'other', search) { const typeUrls = { source: '/-/autocomplete/merge_request_source_branches.json', target: '/-/autocomplete/merge_request_target_branches.json', - '*': Api.buildUrl(Api.createBranchPath).replace(':id', encodeURIComponent(this.fullPath)), + other: Api.buildUrl(Api.createBranchPath).replace(':id', encodeURIComponent(this.fullPath)), }; const url = typeUrls[type]; const branchPath = - url && this.projectId ? mergeUrlParams({ project_id: this.projectId }, url) : typeUrls['*']; + url && this.projectId + ? mergeUrlParams({ project_id: this.projectId }, url) + : typeUrls.other; + const timer = this.branchRefreshTimers[type]; - if (this.branchRefreshTimer) { - clearInterval(this.branchRefreshTimer); + if (timer) { + clearInterval(timer); } - this.branchRefreshTimer = setInterval( + this.branchRefreshTimers[type] = setInterval( () => this.autocompleteCache.updateLocalCache(branchPath), BRANCH_LIST_REFRESH_INTERVAL, ); -- GitLab From 88f1e356f2b46db71507a66a5eb183c902d6eddf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 15:53:10 -0600 Subject: [PATCH 10/24] Test the new - more complicated - fetchBranches in the MR list app --- .../merge_requests_list_app_spec.js | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index 6c9041cb719c51..08d0eac603a391 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -1,6 +1,9 @@ +import AxiosMockAdapter from 'axios-mock-adapter'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import VueRouter from 'vue-router'; +import axios from '~/lib/utils/axios_utils'; +import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; @@ -136,6 +139,101 @@ describe('Merge requests list app', () => { }); }); + describe('fetching branches', () => { + const apiVersion = 1; + const projectId = 2; + const fullPath = 'gitlab-org/gitlab'; + const allBranchesPath = `/api/${apiVersion}/projects/${encodeURIComponent(fullPath)}/repository/branches`; + const sourceBranchPath = `/-/autocomplete/merge_request_source_branches.json?project_id=${projectId}`; + const targetBranchPath = `/-/autocomplete/merge_request_target_branches.json?project_id=${projectId}`; + let axiosMock; + + beforeEach(() => { + axiosMock = new AxiosMockAdapter(axios); + + window.gon = { api_version: apiVersion }; + axiosMock.onGet().reply(HTTP_STATUS_OK, []); + }); + + describe('with no projectId', () => { + it('uses the generic "all branches" endpoint', async () => { + createComponent(); + + await wrapper.vm.fetchBranches(); + + expect(axiosMock.history.get[0].url).toBe(allBranchesPath); + }); + }); + + describe('with projectId', () => { + it.each` + branchPath | fetchArgs + ${targetBranchPath} | ${['target']} + ${sourceBranchPath} | ${['source']} + ${allBranchesPath} | ${['']} + `( + 'selects the correct path ($branchPath) given the arguments $fetchArgs', + async ({ branchPath, fetchArgs }) => { + createComponent(); + wrapper.vm.projectId = projectId; + + await wrapper.vm.fetchBranches(...fetchArgs); + + expect(axiosMock.history.get[0].url).toBe(branchPath); + }, + ); + }); + + describe('auto-update timer', () => { + const mockIntervalId = 99; + const testIntervalId = 1; + let clearSpy; + + beforeEach(() => { + jest.spyOn(window, 'setInterval').mockReturnValue(mockIntervalId); + clearSpy = jest.spyOn(window, 'clearInterval').mockImplementation(); + }); + + it('has no timers normally', () => { + createComponent(); + + expect(wrapper.vm.branchRefreshTimers).toEqual({}); + }); + + it.each` + type + ${'source'} + ${'target'} + ${'other'} + `('only sets the timer for the type of branch request ($type)', async ({ type }) => { + createComponent(); + + await wrapper.vm.fetchBranches(type); + + expect(wrapper.vm.branchRefreshTimers).toEqual({ [type]: expect.any(Number) }); + }); + + it('clears the old timer before setting a new one', async () => { + createComponent(); + + wrapper.vm.branchRefreshTimers.target = testIntervalId; + await wrapper.vm.fetchBranches('target'); + + expect(clearSpy).toHaveBeenCalledWith(testIntervalId); + expect(wrapper.vm.branchRefreshTimers).toEqual({ target: mockIntervalId }); + }); + }); + + it('uses the AutocompleteCache', async () => { + createComponent(); + const fetchSpy = jest.spyOn(wrapper.vm.autocompleteCache, 'fetch'); + + await wrapper.vm.fetchBranches(); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + }); + describe('tokens', () => { const mockCurrentUser = { id: 1, -- GitLab From d178b476ec4670e8de4b3bc385048d89b9f67302 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 23 Sep 2024 16:13:02 -0600 Subject: [PATCH 11/24] In classic fashion, Webpack doesn't understand standard JavaScript --- app/assets/javascripts/merge_requests/list/constants.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_requests/list/constants.js b/app/assets/javascripts/merge_requests/list/constants.js index 04394a904fddf4..651152a93cdef0 100644 --- a/app/assets/javascripts/merge_requests/list/constants.js +++ b/app/assets/javascripts/merge_requests/list/constants.js @@ -13,4 +13,4 @@ export const i18n = { newMergeRequest: __('New merge request'), }; -export const BRANCH_LIST_REFRESH_INTERVAL = 600_000; // 10 minutes (* 60 seconds, * 1000 milliseconds) +export const BRANCH_LIST_REFRESH_INTERVAL = 600000; // 10 minutes (* 60 seconds, * 1000 milliseconds) -- GitLab From 6564e58b06a02d529ebd2e5eed08912ee7385274 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 26 Sep 2024 15:24:59 -0600 Subject: [PATCH 12/24] Split up the logic of the fetchBranches method to be more focused --- .../components/merge_requests_list_app.vue | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 5137ee6d532d7c..2a5f3fbe04c411 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -474,27 +474,34 @@ export default { issuableEventHub.$off('issuables:toggleBulkEdit', this.toggleBulkEditSidebar); }, methods: { - fetchBranches(type = 'other', search) { + getBranchPath(branchType = 'other') { const typeUrls = { source: '/-/autocomplete/merge_request_source_branches.json', target: '/-/autocomplete/merge_request_target_branches.json', other: Api.buildUrl(Api.createBranchPath).replace(':id', encodeURIComponent(this.fullPath)), }; - const url = typeUrls[type]; - const branchPath = - url && this.projectId - ? mergeUrlParams({ project_id: this.projectId }, url) - : typeUrls.other; - const timer = this.branchRefreshTimers[type]; + const url = typeUrls[branchType]; + + return url && this.projectId + ? mergeUrlParams({ project_id: this.projectId }, url) + : typeUrls.other; + }, + updateBranchRefreshTimer(branchType, path) { + const timer = this.branchRefreshTimers[branchType]; if (timer) { clearInterval(timer); } - this.branchRefreshTimers[type] = setInterval( - () => this.autocompleteCache.updateLocalCache(branchPath), + this.branchRefreshTimers[branchType] = setInterval( + () => this.autocompleteCache.updateLocalCache(path), BRANCH_LIST_REFRESH_INTERVAL, ); + }, + fetchBranches(type = 'other', search) { + const branchPath = this.getBranchPath(type); + + this.updateBranchRefreshTimer(type, branchPath); return this.autocompleteCache.fetch({ mutator: (branchList) => -- GitLab From acc7e2027f3ce45ce4ca8cee23de822ed8c9b3a1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Oct 2024 20:21:57 -0600 Subject: [PATCH 13/24] If the branch data object already has a `name`, prefer it to `title` --- .../list/components/merge_requests_list_app.vue | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 2a5f3fbe04c411..ef9628fd5a1b5d 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -505,7 +505,11 @@ export default { return this.autocompleteCache.fetch({ mutator: (branchList) => - branchList.map((branch, index) => ({ ...branch, name: branch.title, id: index })), + branchList.map((branch, index) => ({ + ...branch, + name: branch.name || branch.title, + id: index, + })), formatter: (results) => ({ data: results }), url: branchPath, searchProperty: 'name', -- GitLab From e2bcb507cf72d111989ddaf46b6cff59ff9e67d8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Oct 2024 20:32:46 -0600 Subject: [PATCH 14/24] Test that Apollo, Vue, and vue-apollo are all functioning properly --- .../list/components/merge_requests_list_app_spec.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index 08d0eac603a391..b8ebdc9078fff1 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -174,8 +174,11 @@ describe('Merge requests list app', () => { `( 'selects the correct path ($branchPath) given the arguments $fetchArgs', async ({ branchPath, fetchArgs }) => { - createComponent(); - wrapper.vm.projectId = projectId; + const queryResponse = getQueryResponse; + queryResponse.data.project.id = projectId; + + createComponent({ response: queryResponse }); + await waitForPromises(); await wrapper.vm.fetchBranches(...fetchArgs); -- GitLab From f0f2ed44507a4aedfad5ec2c48dd8e9fed958b5b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Oct 2024 22:02:39 -0600 Subject: [PATCH 15/24] Don't use a promise race condition to ensure the projectId isn't set --- .../list/components/merge_requests_list_app_spec.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index b8ebdc9078fff1..b9bf059303c8cf 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -157,8 +157,12 @@ describe('Merge requests list app', () => { describe('with no projectId', () => { it('uses the generic "all branches" endpoint', async () => { - createComponent(); + const queryResponse = getQueryResponse; + queryResponse.data.project.id = null; + + createComponent({ response: queryResponse }); + await waitForPromises(); await wrapper.vm.fetchBranches(); expect(axiosMock.history.get[0].url).toBe(allBranchesPath); -- GitLab From 625be70442fb85492ff2e9cefb7f8d766d396b58 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Oct 2024 23:47:44 -0600 Subject: [PATCH 16/24] Switch from a cache refresh interval to an expiration age --- .../components/merge_requests_list_app.vue | 23 ++++++++------- .../merge_requests_list_app_spec.js | 28 ++++++++----------- 2 files changed, 22 insertions(+), 29 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index ef9628fd5a1b5d..6d1eb4e29ecc67 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -129,7 +129,7 @@ export default { data() { return { projectId: null, - branchRefreshTimers: {}, + branchCacheAges: {}, filterTokens: [], mergeRequests: [], mergeRequestCounts: {}, @@ -486,22 +486,21 @@ export default { ? mergeUrlParams({ project_id: this.projectId }, url) : typeUrls.other; }, - updateBranchRefreshTimer(branchType, path) { - const timer = this.branchRefreshTimers[branchType]; + updateBranchCache(branchType, path) { + const now = Date.now(); + const lastCheck = this.branchCacheAges[branchType]; + const cacheExpired = !lastCheck || lastCheck + BRANCH_LIST_REFRESH_INTERVAL < now; - if (timer) { - clearInterval(timer); - } + if (cacheExpired) { + this.branchCacheAges[branchType] = now; - this.branchRefreshTimers[branchType] = setInterval( - () => this.autocompleteCache.updateLocalCache(path), - BRANCH_LIST_REFRESH_INTERVAL, - ); + this.autocompleteCache.updateLocalCache(path); + } }, - fetchBranches(type = 'other', search) { + async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); - this.updateBranchRefreshTimer(type, branchPath); + this.updateBranchCache(type, branchPath); return this.autocompleteCache.fetch({ mutator: (branchList) => diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index b9bf059303c8cf..c720b5d42a38e6 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -5,6 +5,7 @@ import VueRouter from 'vue-router'; import axios from '~/lib/utils/axios_utils'; import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper'; +import { setGlobalDateToFakeDate, setGlobalDateToRealDate } from 'helpers/fake_date/fake_date'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import setWindowLocation from 'helpers/set_window_location_helper'; @@ -191,20 +192,23 @@ describe('Merge requests list app', () => { ); }); - describe('auto-update timer', () => { + describe('cache expiration', () => { const mockIntervalId = 99; const testIntervalId = 1; let clearSpy; beforeEach(() => { - jest.spyOn(window, 'setInterval').mockReturnValue(mockIntervalId); - clearSpy = jest.spyOn(window, 'clearInterval').mockImplementation(); + setGlobalDateToFakeDate(); }); - it('has no timers normally', () => { + afterEach(() => { + setGlobalDateToRealDate(); + }); + + it('has no cache ages normally', () => { createComponent(); - expect(wrapper.vm.branchRefreshTimers).toEqual({}); + expect(wrapper.vm.branchCacheAges).toEqual({}); }); it.each` @@ -212,22 +216,12 @@ describe('Merge requests list app', () => { ${'source'} ${'target'} ${'other'} - `('only sets the timer for the type of branch request ($type)', async ({ type }) => { + `('only sets the cache age for the type of branch request ($type)', async ({ type }) => { createComponent(); await wrapper.vm.fetchBranches(type); - expect(wrapper.vm.branchRefreshTimers).toEqual({ [type]: expect.any(Number) }); - }); - - it('clears the old timer before setting a new one', async () => { - createComponent(); - - wrapper.vm.branchRefreshTimers.target = testIntervalId; - await wrapper.vm.fetchBranches('target'); - - expect(clearSpy).toHaveBeenCalledWith(testIntervalId); - expect(wrapper.vm.branchRefreshTimers).toEqual({ target: mockIntervalId }); + expect(wrapper.vm.branchCacheAges).toEqual({ [type]: expect.any(Number) }); }); }); -- GitLab From 133544523631a6665671a00b08850a54a61099ee Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Oct 2024 23:50:09 -0600 Subject: [PATCH 17/24] Test the cache age guard clause --- .../list/components/merge_requests_list_app_spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index c720b5d42a38e6..aa27c565a70430 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -223,6 +223,8 @@ describe('Merge requests list app', () => { expect(wrapper.vm.branchCacheAges).toEqual({ [type]: expect.any(Number) }); }); + + it.skip('only requests fresh data if the cache has become stale', () => {}); }); it('uses the AutocompleteCache', async () => { -- GitLab From a6a8e40b0cb88010d5932c0e1603d5e004da9b33 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 2 Oct 2024 02:05:19 -0600 Subject: [PATCH 18/24] Test the expiration age cache refresh logic --- .../components/merge_requests_list_app.vue | 6 +-- .../merge_requests_list_app_spec.js | 44 ++++++++++++++----- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 6d1eb4e29ecc67..8cd010face3e9f 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -486,7 +486,7 @@ export default { ? mergeUrlParams({ project_id: this.projectId }, url) : typeUrls.other; }, - updateBranchCache(branchType, path) { + async updateBranchCache(branchType, path) { const now = Date.now(); const lastCheck = this.branchCacheAges[branchType]; const cacheExpired = !lastCheck || lastCheck + BRANCH_LIST_REFRESH_INTERVAL < now; @@ -494,13 +494,13 @@ export default { if (cacheExpired) { this.branchCacheAges[branchType] = now; - this.autocompleteCache.updateLocalCache(path); + await this.autocompleteCache.updateLocalCache(path); } }, async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); - this.updateBranchCache(type, branchPath); + await this.updateBranchCache(type, branchPath); return this.autocompleteCache.fetch({ mutator: (branchList) => diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index aa27c565a70430..6d72f76b20a4a9 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -5,7 +5,6 @@ import VueRouter from 'vue-router'; import axios from '~/lib/utils/axios_utils'; import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; import { shallowMountExtended, mountExtended } from 'helpers/vue_test_utils_helper'; -import { setGlobalDateToFakeDate, setGlobalDateToRealDate } from 'helpers/fake_date/fake_date'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; import setWindowLocation from 'helpers/set_window_location_helper'; @@ -40,6 +39,7 @@ import { import { mergeRequestListTabs } from '~/vue_shared/issuable/list/constants'; import { getSortOptions } from '~/issues/list/utils'; import MergeRequestsListApp from '~/merge_requests/list/components/merge_requests_list_app.vue'; +import { BRANCH_LIST_REFRESH_INTERVAL } from '~/merge_requests/list/constants'; import getMergeRequestsQuery from '~/merge_requests/list/queries/get_merge_requests.query.graphql'; import getMergeRequestsCountQuery from '~/merge_requests/list/queries/get_merge_requests_counts.query.graphql'; import IssuableList from '~/vue_shared/issuable/list/components/issuable_list_root.vue'; @@ -193,21 +193,27 @@ describe('Merge requests list app', () => { }); describe('cache expiration', () => { - const mockIntervalId = 99; - const testIntervalId = 1; - let clearSpy; + const queryResponse = getQueryResponse; + let now = Date.now(); + let originalDate; - beforeEach(() => { - setGlobalDateToFakeDate(); + queryResponse.data.project.id = projectId; + + beforeEach(async () => { + originalDate = window.Date; + window.Date = { now: () => now }; + axiosMock.resetHistory(); + + createComponent(); + + return await waitForPromises(); }); afterEach(() => { - setGlobalDateToRealDate(); + window.Date = originalDate; }); it('has no cache ages normally', () => { - createComponent(); - expect(wrapper.vm.branchCacheAges).toEqual({}); }); @@ -217,14 +223,28 @@ describe('Merge requests list app', () => { ${'target'} ${'other'} `('only sets the cache age for the type of branch request ($type)', async ({ type }) => { - createComponent(); - await wrapper.vm.fetchBranches(type); expect(wrapper.vm.branchCacheAges).toEqual({ [type]: expect.any(Number) }); }); - it.skip('only requests fresh data if the cache has become stale', () => {}); + it('only requests fresh data if the cache has become stale', async () => { + // Prime the target cache + await wrapper.vm.fetchBranches('target'); + expect(axiosMock.history.get.length).toBe(1); + + now += 1000; + + // Only load from the cache since it has not expired yet + await wrapper.vm.fetchBranches('target'); + expect(axiosMock.history.get.length).toBe(1); + + now += BRANCH_LIST_REFRESH_INTERVAL; + + // Refresh the cache since the expiration date has passed + await wrapper.vm.fetchBranches('target'); + expect(axiosMock.history.get.length).toBe(2); + }); }); it('uses the AutocompleteCache', async () => { -- GitLab From cbe49a8134093513059cc238f700cc62dbd52d61 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 2 Oct 2024 09:35:46 -0600 Subject: [PATCH 19/24] Remove redundant await --- .../list/components/merge_requests_list_app_spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index 6d72f76b20a4a9..6a50010003cbbd 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -199,14 +199,14 @@ describe('Merge requests list app', () => { queryResponse.data.project.id = projectId; - beforeEach(async () => { + beforeEach(() => { originalDate = window.Date; window.Date = { now: () => now }; axiosMock.resetHistory(); createComponent(); - return await waitForPromises(); + return waitForPromises(); }); afterEach(() => { -- GitLab From 0c8588035b0ef28efbb3972c112e5516ef336bbc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 3 Oct 2024 13:40:15 -0600 Subject: [PATCH 20/24] Use jest.advanceTimersByTime --- .../list/components/merge_requests_list_app_spec.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index 6a50010003cbbd..adef7531f30648 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -194,14 +194,10 @@ describe('Merge requests list app', () => { describe('cache expiration', () => { const queryResponse = getQueryResponse; - let now = Date.now(); - let originalDate; queryResponse.data.project.id = projectId; beforeEach(() => { - originalDate = window.Date; - window.Date = { now: () => now }; axiosMock.resetHistory(); createComponent(); @@ -209,10 +205,6 @@ describe('Merge requests list app', () => { return waitForPromises(); }); - afterEach(() => { - window.Date = originalDate; - }); - it('has no cache ages normally', () => { expect(wrapper.vm.branchCacheAges).toEqual({}); }); @@ -233,13 +225,13 @@ describe('Merge requests list app', () => { await wrapper.vm.fetchBranches('target'); expect(axiosMock.history.get.length).toBe(1); - now += 1000; + jest.advanceTimersByTime(1000); // Only load from the cache since it has not expired yet await wrapper.vm.fetchBranches('target'); expect(axiosMock.history.get.length).toBe(1); - now += BRANCH_LIST_REFRESH_INTERVAL; + jest.advanceTimersByTime(BRANCH_LIST_REFRESH_INTERVAL); // Refresh the cache since the expiration date has passed await wrapper.vm.fetchBranches('target'); -- GitLab From b1f9f87cf64224c8ddfbf07202ac7d0c94a19885 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 4 Oct 2024 01:13:42 -0600 Subject: [PATCH 21/24] The first time we fetch branches, don't try to do a refresh - We wait for at least one full-fledged "fetch" to occur by skipping a refresh if there isn't a last check time. - During that fetch, any mutators, searchProperties, formatters, etc. will be set up - After that fetch, we set the age of the cache. - Future requests for branches will trigger a refresh, which will check the - now available - last check time, and proceed ahead if it has expired. --- .../list/components/merge_requests_list_app.vue | 13 +++++++++---- .../components/merge_requests_list_app_spec.js | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 8cd010face3e9f..8f6bd0098b5db2 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -489,20 +489,19 @@ export default { async updateBranchCache(branchType, path) { const now = Date.now(); const lastCheck = this.branchCacheAges[branchType]; - const cacheExpired = !lastCheck || lastCheck + BRANCH_LIST_REFRESH_INTERVAL < now; + const cacheExpired = lastCheck + BRANCH_LIST_REFRESH_INTERVAL < now; if (cacheExpired) { - this.branchCacheAges[branchType] = now; - await this.autocompleteCache.updateLocalCache(path); } }, async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); + let fetch; await this.updateBranchCache(type, branchPath); - return this.autocompleteCache.fetch({ + fetch = this.autocompleteCache.fetch({ mutator: (branchList) => branchList.map((branch, index) => ({ ...branch, @@ -514,6 +513,12 @@ export default { searchProperty: 'name', search, }); + + fetch.then(() => { + this.branchCacheAges[type] = Date.now(); + }); + + return fetch; }, fetchTargetBranches(search) { return this.fetchBranches('target', search); diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index adef7531f30648..d922edde4382c2 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -209,6 +209,21 @@ describe('Merge requests list app', () => { expect(wrapper.vm.branchCacheAges).toEqual({}); }); + it('does not try to refresh the cache on the very first attempt for a branch type', async () => { + const updateSpy = jest.spyOn(wrapper.vm.autocompleteCache, 'updateLocalCache'); + + await wrapper.vm.fetchBranches('target'); + // This call happens internally in the AutocompleteCache .fetch only when it is first set up + expect(updateSpy).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTime(BRANCH_LIST_REFRESH_INTERVAL + 1); + + await wrapper.vm.fetchBranches('target'); + // Now the MR List app attempts to refresh ahead of time, because the cache has expired + // (Note the AutocompleteCache does not, since it doesn't expire caches at all internally) + expect(updateSpy).toHaveBeenCalledTimes(2); + }); + it.each` type ${'source'} -- GitLab From 7accd0b035180231cafbfe176f7f4539ed6a69c8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 10:04:44 -0600 Subject: [PATCH 22/24] Only update the cache age if the cache has expired --- .../list/components/merge_requests_list_app.vue | 15 +++++++++++---- .../components/merge_requests_list_app_spec.js | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 8f6bd0098b5db2..257da74cf40ac1 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -93,6 +93,12 @@ const EmojiToken = () => import('~/vue_shared/components/filtered_search_bar/tokens/emoji_token.vue'); const DateToken = () => import('~/vue_shared/components/filtered_search_bar/tokens/date_token.vue'); +function cacheIsExpired(cacheAge) { + const now = Date.now(); + + return cacheAge + BRANCH_LIST_REFRESH_INTERVAL <= now; +} + export default { name: 'MergeRequestsListApp', i18n, @@ -487,16 +493,15 @@ export default { : typeUrls.other; }, async updateBranchCache(branchType, path) { - const now = Date.now(); const lastCheck = this.branchCacheAges[branchType]; - const cacheExpired = lastCheck + BRANCH_LIST_REFRESH_INTERVAL < now; - if (cacheExpired) { + if (cacheIsExpired(lastCheck)) { await this.autocompleteCache.updateLocalCache(path); } }, async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); + const cacheAge = this.branchCacheAges[type]; let fetch; await this.updateBranchCache(type, branchPath); @@ -515,7 +520,9 @@ export default { }); fetch.then(() => { - this.branchCacheAges[type] = Date.now(); + if (!cacheAge || cacheIsExpired(cacheAge)) { + this.branchCacheAges[type] = Date.now(); + } }); return fetch; diff --git a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js index d922edde4382c2..97416f3b47dc99 100644 --- a/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js +++ b/spec/frontend/merge_requests/list/components/merge_requests_list_app_spec.js @@ -216,7 +216,7 @@ describe('Merge requests list app', () => { // This call happens internally in the AutocompleteCache .fetch only when it is first set up expect(updateSpy).toHaveBeenCalledTimes(1); - await jest.advanceTimersByTime(BRANCH_LIST_REFRESH_INTERVAL + 1); + await jest.advanceTimersByTime(BRANCH_LIST_REFRESH_INTERVAL); await wrapper.vm.fetchBranches('target'); // Now the MR List app attempts to refresh ahead of time, because the cache has expired -- GitLab From fb0b7e4a92aacbad550df6a971d9f782f95002f1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 10:20:07 -0600 Subject: [PATCH 23/24] Make Prettier happy --- .../list/components/merge_requests_list_app.vue | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 257da74cf40ac1..9ef3ba5b07742d 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -502,11 +502,10 @@ export default { async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); const cacheAge = this.branchCacheAges[type]; - let fetch; await this.updateBranchCache(type, branchPath); - fetch = this.autocompleteCache.fetch({ + const fetch = this.autocompleteCache.fetch({ mutator: (branchList) => branchList.map((branch, index) => ({ ...branch, @@ -519,11 +518,15 @@ export default { search, }); - fetch.then(() => { - if (!cacheAge || cacheIsExpired(cacheAge)) { - this.branchCacheAges[type] = Date.now(); - } - }); + fetch + .then(() => { + if (!cacheAge || cacheIsExpired(cacheAge)) { + this.branchCacheAges[type] = Date.now(); + } + }) + .catch(() => { + // An error has occurred, but there's nothing the user can do about it, so... we're swallowing it. + }); return fetch; }, -- GitLab From 7d294718b637c2e4fa6c6f23d165346db58b0628 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 7 Oct 2024 18:40:22 -0600 Subject: [PATCH 24/24] Set the cache expiration compare date early This helps reduce the risk of a race condition where the cache update does NOT fire, but the expiration date is updated if it ticks over in the time it takes to fetch from the cache. --- .../list/components/merge_requests_list_app.vue | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue index 9ef3ba5b07742d..a182dd8b75bb78 100644 --- a/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue +++ b/app/assets/javascripts/merge_requests/list/components/merge_requests_list_app.vue @@ -93,10 +93,8 @@ const EmojiToken = () => import('~/vue_shared/components/filtered_search_bar/tokens/emoji_token.vue'); const DateToken = () => import('~/vue_shared/components/filtered_search_bar/tokens/date_token.vue'); -function cacheIsExpired(cacheAge) { - const now = Date.now(); - - return cacheAge + BRANCH_LIST_REFRESH_INTERVAL <= now; +function cacheIsExpired(cacheAge, compareTo = Date.now()) { + return cacheAge + BRANCH_LIST_REFRESH_INTERVAL <= compareTo; } export default { @@ -502,6 +500,7 @@ export default { async fetchBranches(type = 'other', search) { const branchPath = this.getBranchPath(type); const cacheAge = this.branchCacheAges[type]; + const runTime = Date.now(); await this.updateBranchCache(type, branchPath); @@ -520,7 +519,7 @@ export default { fetch .then(() => { - if (!cacheAge || cacheIsExpired(cacheAge)) { + if (!cacheAge || cacheIsExpired(cacheAge, runTime)) { this.branchCacheAges[type] = Date.now(); } }) -- GitLab