From 5c654364c1f96a36893019eb0dd06d382db2d6f2 Mon Sep 17 00:00:00 2001 From: Miguel Alatzar Date: Fri, 3 May 2019 09:24:40 -0700 Subject: [PATCH] [MM-15401] Fix setting of extension when caching files (#2762) * Get extension from Content-Disposition first * User ImageCacheManager.cache over getCacheFile * Add unit tests for ImageCacheManager.getCacheFile * Add unit tests for ImageCacheManager.cache * Use exports and require to be able to mock isDownloading * Chain mockReturnValueOnce calls * Fix getExtensionFromContentDisposition and its unit tests --- .../file_attachment_list.js | 14 +- app/utils/file.js | 20 ++ app/utils/file.test.js | 47 +++ app/utils/image_cache_manager.js | 35 ++- app/utils/image_cache_manager.test.js | 295 ++++++++++++++++++ test/setup.js | 10 + 6 files changed, 401 insertions(+), 20 deletions(-) create mode 100644 app/utils/file.test.js create mode 100644 app/utils/image_cache_manager.test.js diff --git a/app/components/file_attachment_list/file_attachment_list.js b/app/components/file_attachment_list/file_attachment_list.js index c8a129330..c87609916 100644 --- a/app/components/file_attachment_list/file_attachment_list.js +++ b/app/components/file_attachment_list/file_attachment_list.js @@ -5,7 +5,6 @@ import React, {Component} from 'react'; import PropTypes from 'prop-types'; import { Keyboard, - Platform, ScrollView, StyleSheet, } from 'react-native'; @@ -13,9 +12,10 @@ import { import {Client4} from 'mattermost-redux/client'; import {isDocument, isGif, isVideo} from 'app/utils/file'; -import {getCacheFile} from 'app/utils/image_cache_manager'; +import ImageCacheManager from 'app/utils/image_cache_manager'; import {previewImageAtIndex} from 'app/utils/images'; import {preventDoubleTap} from 'app/utils/tap'; +import {emptyFunction} from 'app/utils/general'; import FileAttachment from './file_attachment'; @@ -99,18 +99,12 @@ export default class FileAttachmentList extends Component { } let uri; - let cache; if (file.localPath) { uri = file.localPath; } else if (isGif(file)) { - cache = await getCacheFile(file.name, Client4.getFileUrl(file.id)); // eslint-disable-line no-await-in-loop + uri = await ImageCacheManager.cache(file.name, Client4.getFileUrl(file.id), emptyFunction); // eslint-disable-line no-await-in-loop } else { - cache = await getCacheFile(file.name, Client4.getFilePreviewUrl(file.id)); // eslint-disable-line no-await-in-loop - } - - if (cache) { - const prefix = Platform.OS === 'android' ? 'file://' : ''; - uri = `${prefix}${cache.path}`; + uri = await ImageCacheManager.cache(file.name, Client4.getFilePreviewUrl(file.id), emptyFunction); // eslint-disable-line no-await-in-loop } results.push({ diff --git a/app/utils/file.js b/app/utils/file.js index 91653adaf..586571908 100644 --- a/app/utils/file.js +++ b/app/utils/file.js @@ -10,6 +10,7 @@ import {lookupMimeType} from 'mattermost-redux/utils/file_utils'; import {DeviceTypes} from 'app/constants/'; const EXTRACT_TYPE_REGEXP = /^\s*([^;\s]*)(?:;|\s|$)/; +const CONTENT_DISPOSITION_REGEXP = /inline;filename=".*\.(?[a-z]+)";/i; const {DOCUMENTS_PATH, IMAGES_PATH, VIDEOS_PATH} = DeviceTypes; const DEFAULT_SERVER_MAX_FILE_SIZE = 50 * 1024 * 1024;// 50 Mb @@ -234,3 +235,22 @@ export function getLocalFilePathFromFile(dir, file) { return null; } + +export function getExtensionFromContentDisposition(contentDisposition) { + const match = CONTENT_DISPOSITION_REGEXP.exec(contentDisposition); + let extension = match && match.groups.ext; + if (extension) { + if (!Object.keys(types).length) { + populateMaps(); + } + + extension = extension.toLowerCase(); + if (types[extension]) { + return extension; + } + + return null; + } + + return null; +} diff --git a/app/utils/file.test.js b/app/utils/file.test.js new file mode 100644 index 000000000..ddefabaf0 --- /dev/null +++ b/app/utils/file.test.js @@ -0,0 +1,47 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {getExtensionFromContentDisposition} from 'app/utils/file'; + +describe('getExtensionFromContentDisposition', () => { + it('should return the extracted the extension', () => { + const exts = [ + 'png', + 'PNG', + 'gif', + 'GIF', + 'bmp', + 'BMP', + 'jpg', + 'JPG', + 'jpeg', + 'JPEG', + ]; + + exts.forEach((ext) => { + const contentDisposition = `inline;filename="test_image.${ext}"; filename*=UTF-8''test_image.${ext}"`; + const extension = getExtensionFromContentDisposition(contentDisposition); + expect(extension).toBe(ext.toLowerCase()); + }); + }); + + it('should return null for an invalid extension', () => { + const invalidExt = 'invalid'; + const contentDisposition = `inline;filename="test_image.${invalidExt}"; filename*=UTF-8''test_image.${invalidExt}"`; + const extension = getExtensionFromContentDisposition(contentDisposition); + expect(extension).toBe(null); + }); + + it('should return null for no content disposition', () => { + const contentDispositions = [ + undefined, + null, + '', + ]; + + contentDispositions.forEach((contentDisposition) => { + const extension = getExtensionFromContentDisposition(contentDisposition); + expect(extension).toBe(null); + }); + }); +}); diff --git a/app/utils/image_cache_manager.js b/app/utils/image_cache_manager.js index 64f3ee973..1f0630b4b 100644 --- a/app/utils/image_cache_manager.js +++ b/app/utils/image_cache_manager.js @@ -9,7 +9,10 @@ import RNFetchBlob from 'rn-fetch-blob'; import {Client4} from 'mattermost-redux/client'; import {DeviceTypes} from 'app/constants'; -import {getExtensionFromMime} from 'app/utils/file'; +import { + getExtensionFromMime, + getExtensionFromContentDisposition, +} from 'app/utils/file'; import mattermostBucket from 'app/mattermost_bucket'; const {IMAGES_PATH} = DeviceTypes; @@ -26,10 +29,12 @@ export default class ImageCacheManager { const {path, exists} = await getCacheFile(filename, uri); const prefix = Platform.OS === 'android' ? 'file://' : ''; - if (isDownloading(uri)) { + let pathWithPrefix = `${prefix}${path}`; + + if (exports.isDownloading(uri)) { addListener(uri, listener); } else if (exists) { - listener(`${prefix}${path}`); + listener(pathWithPrefix); } else { addListener(uri, listener); if (uri.startsWith('http')) { @@ -55,20 +60,27 @@ export default class ImageCacheManager { throw new Error(); } + const contentDisposition = this.downloadTask.respInfo.headers['Content-Disposition']; const mimeType = this.downloadTask.respInfo.headers['Content-Type']; - const ext = `.${getExtensionFromMime(mimeType) || getExtensionFromMime(DEFAULT_MIME_TYPE)}`; - if (path.endsWith(ext)) { - notifyAll(uri, `${prefix}${path}`); - } else { + const ext = `.${ + getExtensionFromContentDisposition(contentDisposition) || + getExtensionFromMime(mimeType) || + getExtensionFromMime(DEFAULT_MIME_TYPE) + }`; + + if (!path.endsWith(ext)) { const oldExt = path.substring(path.lastIndexOf('.')); const newPath = path.replace(oldExt, ext); await RNFetchBlob.fs.mv(path, newPath); - notifyAll(uri, `${prefix}${newPath}`); + pathWithPrefix = `${prefix}${newPath}`; } + + notifyAll(uri, pathWithPrefix); } catch (e) { - RNFetchBlob.fs.unlink(`${prefix}${path}`); + RNFetchBlob.fs.unlink(pathWithPrefix); notifyAll(uri, uri); + return null; } } else { // In case the uri we are trying to cache is already a local file just notify and return @@ -77,6 +89,8 @@ export default class ImageCacheManager { unsubscribe(uri); } + + return pathWithPrefix; }; } @@ -84,6 +98,7 @@ export const getCacheFile = async (name, uri) => { const filename = name || uri.substring(uri.lastIndexOf('/'), uri.indexOf('?') === -1 ? uri.length : uri.indexOf('?')); const defaultExt = `.${getExtensionFromMime(DEFAULT_MIME_TYPE)}`; const ext = filename.indexOf('.') === -1 ? defaultExt : filename.substring(filename.lastIndexOf('.')); + let path = `${IMAGES_PATH}/${Math.abs(hashCode(uri))}${ext}`; try { @@ -115,7 +130,7 @@ export const setSiteUrl = (url) => { siteUrl = url; }; -const isDownloading = (uri) => Boolean(ImageCacheManager.listeners[uri]); +export const isDownloading = (uri) => Boolean(ImageCacheManager.listeners[uri]); const addListener = (uri, listener) => { if (!ImageCacheManager.listeners[uri]) { diff --git a/app/utils/image_cache_manager.test.js b/app/utils/image_cache_manager.test.js new file mode 100644 index 000000000..d562d7420 --- /dev/null +++ b/app/utils/image_cache_manager.test.js @@ -0,0 +1,295 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +import RNFetchBlob from 'rn-fetch-blob'; + +import ImageCacheManager, {getCacheFile} from 'app/utils/image_cache_manager'; +import {emptyFunction} from 'app/utils/general'; +import * as fileUtils from 'app/utils/file'; +import mattermostBucket from 'app/mattermost_bucket'; + +fileUtils.getExtensionFromMime = jest.fn(); + +describe('getCacheFile', () => { + it('should return a path with correct extension for a non-cached file using file name', async () => { + RNFetchBlob.fs.exists.mockReturnValue(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null); + + const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg']; + const fileUri = 'https://file-uri'; + for (const ext of extensions) { + const fileName = `file${ext}`; + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + + expect(exists).toEqual(false); + expect(path.endsWith(ext)).toEqual(true); + } + }); + + it('should return a path with correct extension for a non-cached file using file uri', async () => { + RNFetchBlob.fs.exists.mockReturnValue(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null); + + const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg']; + const fileName = ''; + for (const ext of extensions) { + const fileUri = `https://file-uri/file${ext}`; + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + + expect(exists).toEqual(false); + expect(path.endsWith(ext)).toEqual(true); + } + }); + + it('should return a path with correct extension for a cached file using file name', async () => { + RNFetchBlob.fs.exists.mockReturnValue(true); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null); + + const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg']; + const fileUri = 'https://file-uri'; + for (const ext of extensions) { + const fileName = `file${ext}`; + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + + expect(exists).toEqual(true); + expect(path.endsWith(ext)).toEqual(true); + } + }); + + it('should return a path with correct extension for a cached file using file uri', async () => { + RNFetchBlob.fs.exists.mockReturnValue(true); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null); + + const extensions = ['.pdf', '.png', '.bmp', '.jpg', '.jpeg']; + const fileName = ''; + for (const ext of extensions) { + const fileUri = `https://file-uri/file${ext}`; + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + + expect(exists).toEqual(true); + expect(path.endsWith(ext)).toEqual(true); + } + }); + + it('should return a path with default extension for a file and uri without an extension', async () => { + RNFetchBlob.fs.exists.mockReturnValue(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(null); + + const defaultExt = 'png'; + fileUtils.getExtensionFromMime.mockReturnValue(defaultExt); + + const fileNames = ['', 'file']; + const fileUris = ['', 'https://file-uri/file']; + for (const fileName of fileNames) { + for (const fileUri of fileUris) { + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + + expect(exists).toEqual(false); + expect(path.endsWith(`.${defaultExt}`)).toEqual(true); + } + } + }); + + it('should return a path with a different extension for a file cached with the different extension', async () => { + const pathWithDiffExt = '/path/to/file.jpeg'; + RNFetchBlob.fs.exists.mockReturnValue(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValue(pathWithDiffExt); + + const fileNamesUris = [ + {fileName: '', fileUri: 'https://file-uri/file.png'}, + {fileName: 'file.png', fileUri: ''}, + ]; + + for (const {fileName, fileUri} of fileNamesUris) { + const {exists, path} = await getCacheFile(fileName, fileUri); // eslint-disable-line no-await-in-loop + expect(exists).toEqual(true); + expect(path).toEqual(pathWithDiffExt); + } + }); +}); + +describe('ImageCacheManager.cache', () => { + const imageCacheManagerUtils = require('app/utils/image_cache_manager'); + imageCacheManagerUtils.isDownloading = jest.fn(); + RNFetchBlob.config.mockReturnValue(RNFetchBlob); + mattermostBucket.getPreference = jest.fn().mockReturnValue({}); + + beforeEach(() => { + ImageCacheManager.listeners = {}; + }); + + it('should cache a file from an http uri', async () => { + const fileName = ''; + const fileUris = ['http://file-uri', 'https://file-uri']; + for (const fileUri of fileUris) { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}}); + + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path).not.toEqual(null); + } + }); + + it('should get the extension from the content disposition', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const ext = '.bmp'; + const headers = {'Content-Disposition': `inline;filename="file${ext}"; filename*=UTF-8''file${ext}`}; + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}}); + + const fileName = ''; + const fileUri = 'https://file-uri'; + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path.endsWith(ext)).toEqual(true); + }); + + it('should get the extension from the content type', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const ext = 'jpg'; + const headers = {'Content-Type': 'image/jpeg'}; + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}}); + fileUtils.getExtensionFromMime. + mockReturnValueOnce(ext). // first call in getCacheFile + mockReturnValueOnce(ext); // seconds call in cache using MIME type from header + + const fileName = ''; + const fileUri = 'https://file-uri'; + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path.endsWith(`.${ext}`)).toEqual(true); + }); + + it('should default to .png', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const ext = 'png'; + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}}); + fileUtils.getExtensionFromMime. + mockReturnValueOnce(ext). // first call in getCacheFile + mockReturnValueOnce(null). // second call in cache using MIME type from header + mockReturnValueOnce(ext); // third call in cache using DEFAULT_MIME_TYPE + + const fileName = ''; + const fileUri = 'https://file-uri'; + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path.endsWith(`.${ext}`)).toEqual(true); + }); + + it('should move file if path extension and extracted extension don\'t match', async () => { + const oldExt = 'png'; + const fileNameUris = [ + {fileName: '', fileUri: `https://file-uri/file.${oldExt}`}, + {fileName: `file${oldExt}`, fileUri: `https://file-uri/file.${oldExt}`}, + ]; + + for (const {fileName, fileUri} of fileNameUris) { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const ext = 'bmp'; + const headers = {'Content-Type': 'image/bmp'}; + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers}}); + fileUtils.getExtensionFromMime. + mockReturnValueOnce(ext). // first call in getCacheFile + mockReturnValueOnce(ext); // second call in cache using MIME type from header + + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path.endsWith(`.${ext}`)).toEqual(true); + + const oldPath = path.replace(ext, oldExt); + expect(RNFetchBlob.fs.mv).toHaveBeenCalledWith(oldPath, path); + } + }); + + it('should return cached file when it exists', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(true); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const ext = '.png'; + const fileName = `file${ext}`; + const fileUri = `http://file-uri/file${ext}`; + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path.endsWith(ext)).toEqual(true); + }); + + it('should not cache the file when respInfo.respType === "text"', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: 'text', headers: {}}}); + + const fileName = ''; + const fileUri = 'https://file-uri'; + const path = await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + expect(path).toEqual(null); + expect(RNFetchBlob.fs.unlink).toHaveBeenCalled(); + }); + + it('should add the listener if it is downloading', async () => { + imageCacheManagerUtils.isDownloading.mockReturnValueOnce(true); + + const fileName = 'file.png'; + const fileUri = 'http://file-uri/file.png'; + await ImageCacheManager.cache(fileName, fileUri, emptyFunction); // eslint-disable-line no-await-in-loop + + const expectedListeners = {[fileUri]: [emptyFunction]}; + expect(ImageCacheManager.listeners).toMatchObject(expectedListeners); + }); + + it('should call listener with path if file exists', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(true); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + const fileName = 'file.png'; + const fileUri = 'http://file-uri/file.png'; + const listener = jest.fn(); + const path = await ImageCacheManager.cache(fileName, fileUri, listener); // eslint-disable-line no-await-in-loop + + expect(ImageCacheManager.listeners).toMatchObject({}); + expect(listener).toHaveBeenCalledWith(path); + }); + + it('should call all listeners with path after downloading file then remove listeners', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + RNFetchBlob.fetch.mockReturnValueOnce({respInfo: {respType: '', headers: {}}}); + + // Ensure isDownloading returns false so that we can proceed to + // download the file even if there are existing listeners. + imageCacheManagerUtils.isDownloading.mockReturnValueOnce(false); + + const fileName = 'file.png'; + const fileUri = 'http://file-uri/file.png'; + const existingListener = jest.fn(); + ImageCacheManager.listeners = {[fileUri]: [existingListener]}; + const newListener = jest.fn(); + const path = await ImageCacheManager.cache(fileName, fileUri, newListener); // eslint-disable-line no-await-in-loop + + expect(ImageCacheManager.listeners).toMatchObject({}); + expect(existingListener).toHaveBeenCalledWith(path); + expect(newListener).toHaveBeenCalledWith(path); + }); + + it('should call all listeners with the local file', async () => { + RNFetchBlob.fs.exists.mockReturnValueOnce(false); + RNFetchBlob.fs.existsWithDiffExt.mockReturnValueOnce(null); + + // Ensure isDownloading returns false so that we can proceed to + // download the file even if there are existing listeners. + imageCacheManagerUtils.isDownloading.mockReturnValueOnce(false); + + const fileName = 'file.png'; + const fileUri = 'file://file-uri/file.png'; + const existingListener = jest.fn(); + ImageCacheManager.listeners = {[fileUri]: [existingListener]}; + const newListener = jest.fn(); + await ImageCacheManager.cache(fileName, fileUri, newListener); // eslint-disable-line no-await-in-loop + + expect(ImageCacheManager.listeners).toMatchObject({}); + expect(existingListener).toHaveBeenCalledWith(fileUri); + expect(newListener).toHaveBeenCalledWith(fileUri); + }); +}); diff --git a/test/setup.js b/test/setup.js index 2b82b76b2..5682cade5 100644 --- a/test/setup.js +++ b/test/setup.js @@ -82,7 +82,13 @@ jest.mock('rn-fetch-blob', () => ({ DocumentDir: () => jest.fn(), CacheDir: () => jest.fn(), }, + exists: jest.fn(), + existsWithDiffExt: jest.fn(), + unlink: jest.fn(), + mv: jest.fn(), }, + fetch: jest.fn(), + config: jest.fn(), })); jest.mock('rn-fetch-blob/fs', () => ({ @@ -90,6 +96,10 @@ jest.mock('rn-fetch-blob/fs', () => ({ DocumentDir: () => jest.fn(), CacheDir: () => jest.fn(), }, + exists: jest.fn(), + existsWithDiffExt: jest.fn(), + unlink: jest.fn(), + mv: jest.fn(), })); global.requestAnimationFrame = (callback) => {