From 7118768fe594f84d60bab23175f457ea0221e3f9 Mon Sep 17 00:00:00 2001 From: Dani Palou Date: Mon, 12 Aug 2024 09:47:37 +0200 Subject: [PATCH] MOBILE-4640 core: Fix revision not removed from pluginfile URLs This means that every time a file revision changed, the app downloaded it into a new file when it should replace the existing file with the new version. --- src/core/classes/delegate.ts | 4 +- src/core/services/filepool.ts | 172 +++++++++++++----- src/core/services/plugin-file-delegate.ts | 2 + .../tests/plugin-file-delegate.test.ts | 97 ++++++++++ src/core/singletons/tests/url.test.ts | 13 ++ src/core/singletons/url.ts | 23 +++ 6 files changed, 265 insertions(+), 46 deletions(-) create mode 100644 src/core/services/tests/plugin-file-delegate.test.ts diff --git a/src/core/classes/delegate.ts b/src/core/classes/delegate.ts index c98cd6f3a..c18aa061d 100644 --- a/src/core/classes/delegate.ts +++ b/src/core/classes/delegate.ts @@ -249,12 +249,12 @@ export class CoreDelegate { const key = handler[this.handlerNameProperty] || handler.name; if (this.handlers[key] !== undefined) { - this.logger.log(`Handler '${handler[this.handlerNameProperty]}' already registered`); + this.logger.log(`Handler '${key}' already registered`); return false; } - this.logger.log(`Registered handler '${handler[this.handlerNameProperty]}'`); + this.logger.log(`Registered handler '${key}'`); this.handlers[key] = handler; return true; diff --git a/src/core/services/filepool.ts b/src/core/services/filepool.ts index 1878700aa..7ec4147ca 100644 --- a/src/core/services/filepool.ts +++ b/src/core/services/filepool.ts @@ -113,6 +113,9 @@ export class CoreFilepoolProvider { protected packagesTables: LazyMap>>; protected queueTable = asyncInstance>(); + // To avoid fixing the same file ID twice at the same time. @deprecated since 4.5 + protected fixFileIdPromises: Record>> = {}; + constructor() { this.logger = CoreLogger.getInstance('CoreFilepoolProvider'); this.filesTables = lazyMap( @@ -770,7 +773,7 @@ export class CoreFilepoolProvider { const extension = CoreMimetypeUtils.guessExtensionFromUrl(fileUrl); const addExtension = filePath === undefined; - const path = filePath || (await this.getFilePath(siteId, fileId, extension)); + const path = filePath || (await this.getFilePath(siteId, fileId, extension, fileUrl)); if (poolFileObject && poolFileObject.fileId !== fileId) { this.logger.error('Invalid object to update passed'); @@ -1099,7 +1102,7 @@ export class CoreFilepoolProvider { }; try { - const fileObject = await this.hasFileInPool(siteId, fileId); + const fileObject = await this.hasFileInPool(siteId, fileId, fileUrl); const isOutdated = await this.checkFileOutdated(siteId, fileObject, options.revision, options.timemodified); let url: string; @@ -1355,6 +1358,43 @@ export class CoreFilepoolProvider { return this.addHashToFilename(url, filename); } + /** + * For a while, the getFileIdByUrl method had a bug that caused revision not to be removed from the URL. + * This function simulates that behaviour and returns the file ID without removing revision. + * This function is temporary and should be removed in the future, it's used to avoid files not being found + * after fixing getFileIdByUrl. + * + * @param fileUrl The absolute URL to the file. + * @returns The file ID. + * @deprecated since 4.5 + */ + protected getFiledIdByUrlBugged(fileUrl: string): string { + let url = fileUrl; + + // If site supports it, since 3.8 we use tokenpluginfile instead of pluginfile. + // For compatibility with files already downloaded, we need to use pluginfile to calculate the file ID. + url = url.replace(/\/tokenpluginfile\.php\/[^/]+\//, '/webservice/pluginfile.php/'); + + // Decode URL. + url = CoreTextUtils.decodeHTML(CoreTextUtils.decodeURIComponent(url)); + + if (url.indexOf('/webservice/pluginfile') !== -1) { + // Remove attributes that do not matter. + this.urlAttributes.forEach((regex) => { + url = url.replace(regex, ''); + }); + } + + // Remove the anchor. + url = CoreUrl.removeUrlParts(url, CoreUrlPartNames.Fragment); + + // Try to guess the filename the target file should have. + // We want to keep the original file name so people can easily identify the files after the download. + const filename = this.guessFilenameFromUrl(url); + + return this.addHashToFilename(url, filename); + } + /** * Get the links of a file. * @@ -1378,15 +1418,16 @@ export class CoreFilepoolProvider { * @param siteId The site ID. * @param fileId The file ID. * @param extension Previously calculated extension. Empty to not add any. Undefined to calculate it. + * @param fileUrl Tmp param to use the bugged file ID if the file isn't found. To be removed with getFiledIdByUrlBugged. * @returns The path to the file relative to storage root. */ - protected async getFilePath(siteId: string, fileId: string, extension?: string): Promise { + protected async getFilePath(siteId: string, fileId: string, extension?: string, fileUrl?: string): Promise { let path = this.getFilepoolFolderPath(siteId) + '/' + fileId; if (extension === undefined) { // We need the extension to be able to open files properly. try { - const entry = await this.hasFileInPool(siteId, fileId); + const entry = await this.hasFileInPool(siteId, fileId, fileUrl); if (entry.extension) { path += '.' + entry.extension; @@ -1412,7 +1453,7 @@ export class CoreFilepoolProvider { const file = await this.fixPluginfileURL(siteId, fileUrl); const fileId = this.getFileIdByUrl(CoreFileHelper.getFileUrl(file)); - return this.getFilePath(siteId, fileId); + return this.getFilePath(siteId, fileId, undefined, CoreFileHelper.getFileUrl(file)); } /** @@ -1531,7 +1572,7 @@ export class CoreFilepoolProvider { } catch (e) { // Check if the file is being downloaded right now. const extension = CoreMimetypeUtils.guessExtensionFromUrl(fileUrl); - filePath = filePath || (await this.getFilePath(siteId, fileId, extension)); + filePath = filePath || (await this.getFilePath(siteId, fileId, extension, fileUrl)); const downloadId = this.getFileDownloadId(fileUrl, filePath); @@ -1541,7 +1582,7 @@ export class CoreFilepoolProvider { try { // File is not being downloaded. Check if it's downloaded and if it's outdated. - const entry = await this.hasFileInPool(siteId, fileId); + const entry = await this.hasFileInPool(siteId, fileId, fileUrl); const isOutdated = await this.checkFileOutdated(siteId, entry, revision, timemodified); if (isOutdated) { @@ -1611,7 +1652,7 @@ export class CoreFilepoolProvider { const fileId = this.getFileIdByUrl(fileUrl); try { - const entry = await this.hasFileInPool(siteId, fileId); + const entry = await this.hasFileInPool(siteId, fileId, fileUrl); const isOutdated = await this.checkFileOutdated(siteId, entry, revision, timemodified); if (isOutdated && CoreNetwork.isOnline()) { @@ -1627,8 +1668,8 @@ export class CoreFilepoolProvider { try { // We found the file entry, now look for the file on disk. const path = mode === 'src' ? - await this.getInternalSrcById(siteId, fileId) : - await this.getInternalUrlById(siteId, fileId); + await this.getInternalSrcById(siteId, fileId, fileUrl) : + await this.getInternalUrlById(siteId, fileId, fileUrl); // Add the anchor to the local URL if any. const anchor = CoreUrl.getUrlAnchor(fileUrl); @@ -1652,14 +1693,15 @@ export class CoreFilepoolProvider { * * @param siteId The site ID. * @param fileId The file ID. + * @param fileUrl Tmp param to use the bugged file ID if the file isn't found. To be removed with getFiledIdByUrlBugged. * @returns Resolved with the internal URL. Rejected otherwise. */ - protected async getInternalSrcById(siteId: string, fileId: string): Promise { + protected async getInternalSrcById(siteId: string, fileId: string, fileUrl?: string): Promise { if (!CoreFile.isAvailable()) { throw new CoreError('File system cannot be used.'); } - const path = await this.getFilePath(siteId, fileId); + const path = await this.getFilePath(siteId, fileId, undefined, fileUrl); const fileEntry = await CoreFile.getFile(path); return CoreFile.convertFileSrc(CoreFile.getFileEntryURL(fileEntry)); @@ -1670,14 +1712,15 @@ export class CoreFilepoolProvider { * * @param siteId The site ID. * @param fileId The file ID. + * @param fileUrl Tmp param to use the bugged file ID if the file isn't found. To be removed with getFiledIdByUrlBugged. * @returns Resolved with the URL. Rejected otherwise. */ - protected async getInternalUrlById(siteId: string, fileId: string): Promise { + protected async getInternalUrlById(siteId: string, fileId: string, fileUrl?: string): Promise { if (!CoreFile.isAvailable()) { throw new CoreError('File system cannot be used.'); } - const path = await this.getFilePath(siteId, fileId); + const path = await this.getFilePath(siteId, fileId, undefined, fileUrl); const fileEntry = await CoreFile.getFile(path); // This URL is usually used to launch files or put them in HTML. @@ -1715,7 +1758,7 @@ export class CoreFilepoolProvider { const file = await this.fixPluginfileURL(siteId, fileUrl); const fileId = this.getFileIdByUrl(CoreFileHelper.getFileUrl(file)); - return this.getInternalSrcById(siteId, fileId); + return this.getInternalSrcById(siteId, fileId, CoreFileHelper.getFileUrl(file)); } /** @@ -1733,7 +1776,7 @@ export class CoreFilepoolProvider { const file = await this.fixPluginfileURL(siteId, fileUrl); const fileId = this.getFileIdByUrl(CoreFileHelper.getFileUrl(file)); - return this.getInternalUrlById(siteId, fileId); + return this.getInternalUrlById(siteId, fileId, CoreFileHelper.getFileUrl(file)); } /** @@ -1888,29 +1931,6 @@ export class CoreFilepoolProvider { } } - /** - * Return the array of arguments of the pluginfile url. - * - * @param url URL to get the args. - * @returns The args found, undefined if not a pluginfile. - */ - protected getPluginFileArgs(url: string): string[] | undefined { - if (!CoreUrl.isPluginFileUrl(url)) { - // Not pluginfile, return. - return; - } - - const relativePath = url.substring(url.indexOf('/pluginfile.php') + 16); - const args = relativePath.split('/'); - - if (args.length < 3) { - // To be a plugin file it should have at least contextId, Component and Filearea. - return; - } - - return args; - } - /** * Get the deferred object for a file in the queue. * @@ -2007,7 +2027,7 @@ export class CoreFilepoolProvider { * @returns Revision number. */ protected getRevisionFromUrl(url: string): number { - const args = this.getPluginFileArgs(url); + const args = CoreUrl.getPluginFileArgs(url); if (!args) { // Not a pluginfile, no revision will be found. return 0; @@ -2193,10 +2213,74 @@ export class CoreFilepoolProvider { * * @param siteId The site ID. * @param fileId The file Id. + * @param fileUrl Tmp param to use the bugged file ID if the file isn't found. To be removed with getFiledIdByUrlBugged. * @returns Resolved with file object from DB on success, rejected otherwise. */ - protected async hasFileInPool(siteId: string, fileId: string): Promise { - return this.filesTables[siteId].getOneByPrimaryKey({ fileId }); + protected async hasFileInPool(siteId: string, fileId: string, fileUrl?: string): Promise { + try { + return await this.filesTables[siteId].getOneByPrimaryKey({ fileId }); + } catch (error) { + if (!fileUrl) { + throw error; + } + + // Entry not found. Check if it's stored with the "bugged" file ID. + const buggedFileId = this.getFiledIdByUrlBugged(fileUrl); // eslint-disable-line deprecation/deprecation + if (buggedFileId === fileId) { + throw error; + } + + const fileEntry = await this.filesTables[siteId].getOneByPrimaryKey({ fileId: buggedFileId }); + + try { + await this.fixBuggedFileId(siteId, fileEntry, fileId); // eslint-disable-line deprecation/deprecation + } catch (error) { + // Ignore errors when fixing the ID, it shouldn't happen. + } + + return fileEntry; + } + } + + /** + * Fix a file entry's wrong file ID. + * + * @param siteId Site ID. + * @param fileEntry File entry to fix. + * @param newFileId New file ID. + * @returns Promise resolved when done. + * @deprecated since 4.5 + */ + protected async fixBuggedFileId(siteId: string, fileEntry: CoreFilepoolFileEntry, newFileId: string): Promise { + if (this.fixFileIdPromises[siteId] && this.fixFileIdPromises[siteId][newFileId] !== undefined) { + return this.fixFileIdPromises[siteId][newFileId]; + } + + const fixFileId = async (): Promise => { + const buggedFileId = fileEntry.fileId; + + const [currentFilePath, newFilePath] = await Promise.all([ + this.getFilePath(siteId, buggedFileId, fileEntry.extension), + this.getFilePath(siteId, newFileId, fileEntry.extension), + ]); + + // Move the file first, it's the step that's easier to fail. + await CoreFile.moveFile(currentFilePath, newFilePath); + + await Promise.all([ + this.filesTables[siteId].update({ fileId: newFileId }, { fileId: buggedFileId }), + CoreUtils.ignoreErrors(this.linksTables[siteId].update({ fileId: newFileId }, { fileId: buggedFileId })), + ]); + + fileEntry.fileId = newFileId; + + delete this.fixFileIdPromises[siteId][newFileId]; + }; + + this.fixFileIdPromises[siteId] = this.fixFileIdPromises[siteId] ?? {}; + this.fixFileIdPromises[siteId][newFileId] = fixFileId(); + + return this.fixFileIdPromises[siteId][newFileId]; } /** @@ -2602,7 +2686,7 @@ export class CoreFilepoolProvider { // Check if the file is already in pool. try { - entry = await this.hasFileInPool(siteId, fileId); + entry = await this.hasFileInPool(siteId, fileId, fileUrl); } catch (error) { // File not in pool. } @@ -2800,7 +2884,7 @@ export class CoreFilepoolProvider { * The revision is used to know if a file has changed. We remove it from the URL to prevent storing a file per revision. */ protected removeRevisionFromUrl(url: string): string { - const args = this.getPluginFileArgs(url); + const args = CoreUrl.getPluginFileArgs(url); if (!args) { // Not a pluginfile, no revision will be found. return url; diff --git a/src/core/services/plugin-file-delegate.ts b/src/core/services/plugin-file-delegate.ts index 456476f74..f7aa0012a 100644 --- a/src/core/services/plugin-file-delegate.ts +++ b/src/core/services/plugin-file-delegate.ts @@ -29,6 +29,8 @@ import { CoreFileHelper } from './file-helper'; @Injectable({ providedIn: 'root' }) export class CorePluginFileDelegateService extends CoreDelegate { + protected handlerNameProperty = 'component'; + constructor() { super('CorePluginFileDelegate'); } diff --git a/src/core/services/tests/plugin-file-delegate.test.ts b/src/core/services/tests/plugin-file-delegate.test.ts new file mode 100644 index 000000000..9fafa1324 --- /dev/null +++ b/src/core/services/tests/plugin-file-delegate.test.ts @@ -0,0 +1,97 @@ +// (C) Copyright 2015 Moodle Pty Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { mock, mockSingleton } from '@/testing/utils'; +import { CoreSite } from '@classes/sites/site'; +import { CorePluginFileDelegateService, CorePluginFileHandler } from '@services/plugin-file-delegate'; +import { CoreSites } from '@services/sites'; +import { CoreUrl } from '@singletons/url'; + +describe('CorePluginFileDelegate', () => { + + let pluginFileDelegate: CorePluginFileDelegateService; + + beforeEach(async () => { + const site = mock(new CoreSite('42', 'https://mysite.com', 'token'), { + isFeatureDisabled: () => false, + }); + + mockSingleton(CoreSites, { getCurrentSite: () => site, getCurrentSiteId: () => '42', isLoggedIn: () => true }); + + pluginFileDelegate = new CorePluginFileDelegateService(); + pluginFileDelegate.registerHandler(new ModFooRevisionHandler()); + + await pluginFileDelegate.updateHandlers(); + }); + + it('removes revision from a URL', () => { + const urlsToTest = [ + // Revision removed by mod_foo handler. + { + value: 'http://mysite.com/webservice/pluginfile.php/6/mod_foo/content/14/foo.txt', + expected: 'http://mysite.com/webservice/pluginfile.php/6/mod_foo/content/0/foo.txt', + }, + // Revision not removed because the component is not mod_foo. + { + value: 'http://mysite.com/webservice/pluginfile.php/6/mod_page/content/14/foo.txt', + expected: 'http://mysite.com/webservice/pluginfile.php/6/mod_page/content/14/foo.txt', + }, + // Revision not removed because it's not a pluginfile URL. + { + value: 'http://mysite.com/6/mod_foo/content/14/foo.txt', + expected: 'http://mysite.com/6/mod_foo/content/14/foo.txt', + }, + ]; + + urlsToTest.forEach(data => { + expect( + pluginFileDelegate.removeRevisionFromUrl(data.value, CoreUrl.getPluginFileArgs(data.value) ?? []), + ).toEqual(data.expected); + }); + }); + +}); + +class ModFooRevisionHandler implements CorePluginFileHandler { + + name = 'ModFooHandler'; + component = 'mod_foo'; + + /** + * @inheritdoc + */ + getComponentRevisionRegExp(args: string[]): RegExp | undefined { + // Check filearea. + if (args[2] == 'content') { + // Component + Filearea + Revision + return new RegExp('/mod_foo/content/([0-9]+)/'); + } + } + + /** + * @inheritdoc + */ + getComponentRevisionReplace(): string { + // Component + Filearea + Revision + return '/mod_foo/content/0/'; + } + + /** + * @inheritdoc + */ + async isEnabled(): Promise { + return true; + } + +} diff --git a/src/core/singletons/tests/url.test.ts b/src/core/singletons/tests/url.test.ts index 08410360b..0e4980e05 100644 --- a/src/core/singletons/tests/url.test.ts +++ b/src/core/singletons/tests/url.test.ts @@ -260,4 +260,17 @@ describe('CoreUrl singleton', () => { .toEqual(`${siteUrl}/media/player/vimeo/wsplayer.php?video=123456&token=${token}&h=foo`); }); + it('extracts args from pluginfile URLs', () => { + expect(CoreUrl.getPluginFileArgs('http://mysite.com/pluginfile.php/6/mod_foo/content/14/foo.txt')) + .toEqual(['6', 'mod_foo', 'content', '14', 'foo.txt']); + expect(CoreUrl.getPluginFileArgs('http://mysite.com/webservice/pluginfile.php/6/mod_foo/content/14/foo.txt')) + .toEqual(['6', 'mod_foo', 'content', '14', 'foo.txt']); + + // It doesn't work with tokenpluginfile or other URLs, and also when pluginfile doesn't have enough params. + expect(CoreUrl.getPluginFileArgs('http://mysite.com')).toEqual(undefined); + expect(CoreUrl.getPluginFileArgs('http://mysite.com/pluginfile.php/6/')).toEqual(undefined); + expect(CoreUrl.getPluginFileArgs('http://mysite.com/tokenpluginfile.php/abcdef123456/6/mod_foo/content/14/foo.txt')) + .toEqual(undefined); + }); + }); diff --git a/src/core/singletons/url.ts b/src/core/singletons/url.ts index d033268dc..e9080b1e3 100644 --- a/src/core/singletons/url.ts +++ b/src/core/singletons/url.ts @@ -642,6 +642,29 @@ export class CoreUrl { return path.split('/').pop() ?? ''; } + /** + * Return the array of arguments of the pluginfile url. + * + * @param url URL to get the args. + * @returns The args found, undefined if not a pluginfile. + */ + static getPluginFileArgs(url: string): string[] | undefined { + if (!CoreUrl.isPluginFileUrl(url)) { + // Not pluginfile, return. + return; + } + + const relativePath = url.substring(url.indexOf('/pluginfile.php') + 16); + const args = relativePath.split('/'); + + if (args.length < 3) { + // To be a plugin file it should have at least contextId, Component and Filearea. + return; + } + + return args; + } + /** * Get the protocol from a URL. * E.g. http://www.google.com returns 'http'.