Merge pull request #15661 from gavindsouza/thumbnail-for-images

fix: Thumbnail for external images (from URL)

frappe/core/doctype/file/file.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 
 """
@@ -7,7 +7,6 @@ record of files
 naming for same name files: file.gif, file-1.gif, file-2.gif etc
 """
 
-import base64
 import hashlib
 import imghdr
 import io
@@ -17,9 +16,10 @@ import os
 import re
 import shutil
 import zipfile
+from typing import TYPE_CHECKING, Tuple
 
 import requests
-import requests.exceptions
+from requests.exceptions import HTTPError, SSLError
 from PIL import Image, ImageFile, ImageOps
 from io import BytesIO
 from urllib.parse import quote, unquote
@@ -31,6 +31,11 @@ from frappe.utils import call_hook_method, cint, cstr, encode, get_files_path, g
 from frappe.utils.image import strip_exif_data, optimize_image
 from frappe.utils.file_manager import safe_b64decode
 
+if TYPE_CHECKING:
+	from PIL.ImageFile import ImageFile
+	from requests.models import Response
+
+
 class MaxFileSizeReachedError(frappe.ValidationError):
 	pass
 
@@ -276,7 +281,7 @@ class File(Document):
 					image, filename, extn = get_local_image(self.file_url)
 				else:
 					image, filename, extn = get_web_image(self.file_url)
-			except (requests.exceptions.HTTPError, requests.exceptions.SSLError, IOError, TypeError):
+			except (HTTPError, SSLError, IOError, TypeError):
 					return
 
 			size = width, height
@@ -648,9 +653,17 @@ def setup_folder_path(filename, new_parent):
 		from frappe.model.rename_doc import rename_doc
 		rename_doc("File", file.name, file.get_name_based_on_parent_folder(), ignore_permissions=True)
 
-def get_extension(filename, extn, content):
+def get_extension(filename, extn, content: bytes = None, response: "Response" = None) -> str:
 	mimetype = None
 
+	if response:
+		content_type = response.headers.get("Content-Type")
+
+		if content_type:
+			_extn = mimetypes.guess_extension(content_type)
+			if _extn:
+				return _extn[1:]
+
 	if extn:
 		# remove '?' char and parameters from extn if present
 		if '?' in extn:
@@ -693,14 +706,14 @@ def get_local_image(file_url):
 
 	return image, filename, extn
 
-def get_web_image(file_url):
+def get_web_image(file_url: str) -> Tuple["ImageFile", str, str]:
 	# download
 	file_url = frappe.utils.get_url(file_url)
 	r = requests.get(file_url, stream=True)
 	try:
 		r.raise_for_status()
-	except requests.exceptions.HTTPError as e:
-		if "404" in e.args[0]:
+	except HTTPError:
+		if r.status_code == 404:
 			frappe.msgprint(_("File '{0}' not found").format(file_url))
 		else:
 			frappe.msgprint(_("Unable to read file format for {0}").format(file_url))
@@ -719,7 +732,10 @@ def get_web_image(file_url):
 		filename = get_random_filename()
 		extn = None
 
-	extn = get_extension(filename, extn, r.content)
+	extn = get_extension(filename, extn, response=r)
+	if extn == "bin":
+		extn = get_extension(filename, extn, content=r.content) or "png"
+
 	filename = "/files/" + strip(unquote(filename))
 
 	return image, filename, extn

frappe/core/doctype/file/test_file.py

@@ -1,11 +1,11 @@
-# -*- coding: utf-8 -*-
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 import base64
 import json
 import frappe
 import os
 import unittest
+
 from frappe import _
 from frappe.core.doctype.file.file import File, get_attached_images, move_file, get_files_in_folder, unzip_file
 from frappe.utils import get_files_path
@@ -384,6 +384,16 @@ class TestFile(unittest.TestCase):
 		test_file.make_thumbnail()
 		self.assertEquals(test_file.thumbnail_url, '/files/image_small.jpg')
 
+		# test web image without extension
+		test_file = frappe.get_doc({
+			"doctype": "File",
+			"file_name": 'logo',
+			"file_url": frappe.utils.get_url('/_test/assets/image'),
+		}).insert(ignore_permissions=True)
+
+		test_file.make_thumbnail()
+		self.assertTrue(test_file.thumbnail_url.endswith("_small.jpeg"))
+
 		# test local image
 		test_file.db_set('thumbnail_url', None)
 		test_file.reload()

frappe/tests/test_website.py

@@ -1,7 +1,9 @@
 import unittest
+from unittest.mock import patch
 
 import frappe
 from frappe.utils import set_request
+from frappe.website.page_renderers.static_page import StaticPage
 from frappe.website.serve import get_response, get_response_content
 from frappe.website.utils import (build_response, clear_website_cache, get_home_page)
 
@@ -97,6 +99,19 @@ class TestWebsite(unittest.TestCase):
 		response = get_response()
 		self.assertEqual(response.status_code, 200)
 
+		set_request(method="GET", path="/_test/assets/image.jpg")
+		response = get_response()
+		self.assertEqual(response.status_code, 200)
+
+		set_request(method="GET", path="/_test/assets/image")
+		response = get_response()
+		self.assertEqual(response.status_code, 200)
+
+		with patch.object(StaticPage, "render") as static_render:
+			set_request(method="GET", path="/_test/assets/image")
+			response = get_response()
+			static_render.assert_called()
+
 	def test_error_page(self):
 		set_request(method='GET', path='/_test/problematic_page')
 		response = get_response()
@@ -127,7 +142,6 @@ class TestWebsite(unittest.TestCase):
 		response = get_response()
 		self.assertEqual(response.status_code, 404)
 
-
 	def test_redirect(self):
 		import frappe.hooks
 		frappe.set_user('Administrator')

frappe/website/page_renderers/static_page.py

@@ -6,6 +6,7 @@ from werkzeug.wsgi import wrap_file
 
 import frappe
 from frappe.website.page_renderers.base_renderer import BaseRenderer
+from frappe.website.utils import is_binary_file
 
 UNSUPPORTED_STATIC_PAGE_TYPES = ('html', 'md', 'js', 'xml', 'css', 'txt', 'py', 'json')
 
@@ -20,21 +21,20 @@ class StaticPage(BaseRenderer):
 			return
 		for app in frappe.get_installed_apps():
 			file_path = frappe.get_app_path(app, 'www') + '/' + self.path
-			if os.path.isfile(file_path):
+			if os.path.isfile(file_path) and is_binary_file(file_path):
 				self.file_path = file_path
 
 	def can_render(self):
 		return self.is_valid_file_path() and self.file_path
 
 	def is_valid_file_path(self):
-		if ('.' not in self.path):
-			return False
 		extension = self.path.rsplit('.', 1)[-1]
 		if extension in UNSUPPORTED_STATIC_PAGE_TYPES:
 			return False
 		return True
 
 	def render(self):
+		# file descriptor to be left open, closed by middleware
 		f = open(self.file_path, 'rb')
 		response = Response(wrap_file(frappe.local.request.environ, f), direct_passthrough=True)
 		response.mimetype = mimetypes.guess_type(self.file_path)[0] or 'application/octet-stream'

frappe/website/page_renderers/template_page.py

@@ -7,7 +7,7 @@ from frappe.website.router import get_page_info
 from frappe.website.page_renderers.base_template_page import BaseTemplatePage
 from frappe.website.router import get_base_template
 from frappe.website.utils import (extract_comment_tag, extract_title, get_next_link,
-	get_toc, get_frontmatter, cache_html, get_sidebar_items)
+	get_toc, get_frontmatter, is_binary_file, cache_html, get_sidebar_items)
 
 WEBPAGE_PY_MODULE_PROPERTIES = ("base_template_path", "template", "no_cache", "sitemap", "condition_field")
 
@@ -39,7 +39,7 @@ class TemplatePage(BaseTemplatePage):
 			for dirname in folders:
 				search_path = os.path.join(app_path, dirname, self.path)
 				for file_path in self.get_index_path_options(search_path):
-					if os.path.isfile(file_path):
+					if os.path.isfile(file_path) and not is_binary_file(file_path):
 						self.app = app
 						self.app_path = app_path
 						self.file_dir = dirname

frappe/website/utils.py

@@ -1,10 +1,10 @@
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors
 # License: MIT. See LICENSE
 import json
 import mimetypes
 import os
 import re
-from functools import wraps
+from functools import cache, wraps
 from typing import Dict, Optional
 
 import yaml
@@ -511,3 +511,11 @@ def add_preload_headers(response):
 	except Exception:
 		import traceback
 		traceback.print_exc()
+
+@cache
+def is_binary_file(path):
+	# ref: https://stackoverflow.com/a/7392391/10309266
+	textchars = bytearray({7,8,9,10,12,13,27} | set(range(0x20, 0x100)) - {0x7f})
+	with open(path, 'rb') as f:
+		content = f.read(1024)
+		return bool(content.translate(None, textchars))