From 7ad06473b6d90c61a4bf83aefd915cfee6da9ed1 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Mon, 12 Dec 2022 21:02:25 +0000 Subject: [PATCH] uploads: Add LOCAL_AVATARS_DIR / LOCAL_FILES_DIR computed settings. This avoids strewing "avatars" and "files" constants throughout. --- zerver/lib/export.py | 10 ++++++---- zerver/lib/import_realm.py | 12 +++++------ zerver/lib/test_helpers.py | 6 ++++-- zerver/lib/test_runner.py | 6 ++++-- zerver/lib/transfer.py | 9 ++++++--- zerver/lib/upload/local.py | 10 +++++----- ...0149_realm_emoji_drop_unique_constraint.py | 6 ++++-- zerver/tests/test_import_export.py | 13 ++++++------ zerver/tests/test_upload.py | 20 +++++++++---------- zproject/computed_settings.py | 4 ++++ zproject/default_settings.py | 2 ++ zproject/dev_urls.py | 4 ++-- 12 files changed, 60 insertions(+), 42 deletions(-) diff --git a/zerver/lib/export.py b/zerver/lib/export.py index a2f2d4dfc2..f62ec4cbbb 100644 --- a/zerver/lib/export.py +++ b/zerver/lib/export.py @@ -1409,23 +1409,25 @@ def export_uploads_and_avatars( realm_emojis = list(RealmEmoji.objects.filter(author_id=user.id)) if settings.LOCAL_UPLOADS_DIR: + assert settings.LOCAL_FILES_DIR + assert settings.LOCAL_AVATARS_DIR # Small installations and developers will usually just store files locally. export_uploads_from_local( realm, - local_dir=os.path.join(settings.LOCAL_UPLOADS_DIR, "files"), + local_dir=settings.LOCAL_FILES_DIR, output_dir=uploads_output_dir, attachments=attachments, ) export_avatars_from_local( realm, - local_dir=os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars"), + local_dir=settings.LOCAL_AVATARS_DIR, output_dir=avatars_output_dir, users=users, handle_system_bots=handle_system_bots, ) export_emoji_from_local( realm, - local_dir=os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars"), + local_dir=settings.LOCAL_AVATARS_DIR, output_dir=emoji_output_dir, realm_emojis=realm_emojis, ) @@ -1433,7 +1435,7 @@ def export_uploads_and_avatars( if user is None: export_realm_icons( realm, - local_dir=os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars"), + local_dir=settings.LOCAL_AVATARS_DIR, output_dir=realm_icons_output_dir, ) else: diff --git a/zerver/lib/import_realm.py b/zerver/lib/import_realm.py index 02a377704a..fc9ee47b25 100644 --- a/zerver/lib/import_realm.py +++ b/zerver/lib/import_realm.py @@ -725,11 +725,9 @@ def process_avatars(record: Dict[str, Any]) -> None: if record["s3_path"].endswith(".original"): user_profile = get_user_profile_by_id(record["user_profile_id"]) - if settings.LOCAL_UPLOADS_DIR is not None: + if settings.LOCAL_AVATARS_DIR is not None: avatar_path = user_avatar_path_from_ids(user_profile.id, record["realm_id"]) - medium_file_path = ( - os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", avatar_path) + "-medium.png" - ) + medium_file_path = os.path.join(settings.LOCAL_AVATARS_DIR, avatar_path) + "-medium.png" if os.path.exists(medium_file_path): # We remove the image here primarily to deal with # issues when running the import script multiple @@ -875,10 +873,12 @@ def import_uploads( ) else: assert settings.LOCAL_UPLOADS_DIR is not None + assert settings.LOCAL_AVATARS_DIR is not None + assert settings.LOCAL_FILES_DIR is not None if processing_avatars or processing_emojis or processing_realm_icons: - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", relative_path) + file_path = os.path.join(settings.LOCAL_AVATARS_DIR, relative_path) else: - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "files", relative_path) + file_path = os.path.join(settings.LOCAL_FILES_DIR, relative_path) orig_file_path = os.path.join(import_dir, record["path"]) os.makedirs(os.path.dirname(file_path), exist_ok=True) shutil.copy(orig_file_path, file_path) diff --git a/zerver/lib/test_helpers.py b/zerver/lib/test_helpers.py index cb646d8b0d..fcafe06261 100644 --- a/zerver/lib/test_helpers.py +++ b/zerver/lib/test_helpers.py @@ -225,9 +225,9 @@ def avatar_disk_path( avatar_url_path = avatar_url(user_profile, medium) assert avatar_url_path is not None assert settings.LOCAL_UPLOADS_DIR is not None + assert settings.LOCAL_AVATARS_DIR is not None avatar_disk_path = os.path.join( - settings.LOCAL_UPLOADS_DIR, - "avatars", + settings.LOCAL_AVATARS_DIR, avatar_url_path.split("/")[-2], avatar_url_path.split("/")[-1].split("?")[0], ) @@ -549,6 +549,8 @@ FuncT = TypeVar("FuncT", bound=Callable[..., None]) def use_s3_backend(method: FuncT) -> FuncT: @mock_s3 @override_settings(LOCAL_UPLOADS_DIR=None) + @override_settings(LOCAL_AVATARS_DIR=None) + @override_settings(LOCAL_FILES_DIR=None) def new_method(*args: Any, **kwargs: Any) -> Any: zerver.lib.upload.upload_backend = S3UploadBackend() try: diff --git a/zerver/lib/test_runner.py b/zerver/lib/test_runner.py index 3c95e7bb0d..f3600e4cba 100644 --- a/zerver/lib/test_runner.py +++ b/zerver/lib/test_runner.py @@ -212,9 +212,9 @@ def init_worker( from zproject.dev_urls import avatars_url assert settings.LOCAL_UPLOADS_DIR is not None + assert settings.LOCAL_AVATARS_DIR is not None assert avatars_url.default_args is not None - new_root = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars") - avatars_url.default_args["document_root"] = new_root + avatars_url.default_args["document_root"] = settings.LOCAL_AVATARS_DIR class ParallelTestSuite(django_runner.ParallelTestSuite): @@ -248,6 +248,8 @@ def initialize_worker_path(worker_id: int) -> None: "test_uploads", ) ) + settings.LOCAL_AVATARS_DIR = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars") + settings.LOCAL_FILES_DIR = os.path.join(settings.LOCAL_UPLOADS_DIR, "files") class Runner(DiscoverRunner): diff --git a/zerver/lib/transfer.py b/zerver/lib/transfer.py index b8c55b3cf1..e5dfc1cce9 100644 --- a/zerver/lib/transfer.py +++ b/zerver/lib/transfer.py @@ -25,7 +25,8 @@ def transfer_uploads_to_s3(processes: int) -> None: def _transfer_avatar_to_s3(user: UserProfile) -> None: avatar_path = user_avatar_path(user) assert settings.LOCAL_UPLOADS_DIR is not None - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", avatar_path) + ".original" + assert settings.LOCAL_AVATARS_DIR is not None + file_path = os.path.join(settings.LOCAL_AVATARS_DIR, avatar_path) + ".original" try: with open(file_path, "rb") as f: s3backend.upload_avatar_image(f, user, user) @@ -53,7 +54,8 @@ def transfer_avatars_to_s3(processes: int) -> None: def _transfer_message_files_to_s3(attachment: Attachment) -> None: assert settings.LOCAL_UPLOADS_DIR is not None - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "files", attachment.path_id) + assert settings.LOCAL_FILES_DIR is not None + file_path = os.path.join(settings.LOCAL_FILES_DIR, attachment.path_id) try: with open(file_path, "rb") as f: guessed_type = guess_type(attachment.file_name)[0] @@ -95,7 +97,8 @@ def _transfer_emoji_to_s3(realm_emoji: RealmEmoji) -> None: emoji_file_name=realm_emoji.file_name, ) assert settings.LOCAL_UPLOADS_DIR is not None - emoji_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", emoji_path) + ".original" + assert settings.LOCAL_AVATARS_DIR is not None + emoji_path = os.path.join(settings.LOCAL_AVATARS_DIR, emoji_path) + ".original" try: with open(emoji_path, "rb") as f: s3backend.upload_emoji_image(f, realm_emoji.file_name, realm_emoji.author) diff --git a/zerver/lib/upload/local.py b/zerver/lib/upload/local.py index 3ecbe85e11..989c308828 100644 --- a/zerver/lib/upload/local.py +++ b/zerver/lib/upload/local.py @@ -69,7 +69,7 @@ def delete_local_file(type: Literal["avatars", "files"], path: str) -> bool: def get_local_file_path(path_id: str) -> Optional[str]: - local_path = os.path.join(assert_is_not_none(settings.LOCAL_UPLOADS_DIR), "files", path_id) + local_path = os.path.join(assert_is_not_none(settings.LOCAL_FILES_DIR), path_id) assert_is_local_storage_path("files", local_path) if os.path.isfile(local_path): @@ -214,15 +214,15 @@ class LocalUploadBackend(ZulipUploadBackend): file_path = user_avatar_path(user_profile) output_path = os.path.join( - assert_is_not_none(settings.LOCAL_UPLOADS_DIR), - "avatars", + assert_is_not_none(settings.LOCAL_AVATARS_DIR), file_path + file_extension, ) if os.path.isfile(output_path): return image_path = os.path.join( - assert_is_not_none(settings.LOCAL_UPLOADS_DIR), "avatars", file_path + ".original" + assert_is_not_none(settings.LOCAL_AVATARS_DIR), + file_path + ".original", ) with open(image_path, "rb") as f: image_data = f.read() @@ -282,7 +282,7 @@ class LocalUploadBackend(ZulipUploadBackend): secrets.token_urlsafe(18), os.path.basename(tarball_path), ) - abs_path = os.path.join(assert_is_not_none(settings.LOCAL_UPLOADS_DIR), "avatars", path) + abs_path = os.path.join(assert_is_not_none(settings.LOCAL_AVATARS_DIR), path) os.makedirs(os.path.dirname(abs_path), exist_ok=True) shutil.copy(tarball_path, abs_path) public_url = realm.uri + "/user_avatars/" + path diff --git a/zerver/migrations/0149_realm_emoji_drop_unique_constraint.py b/zerver/migrations/0149_realm_emoji_drop_unique_constraint.py index ef43ddbf8c..9fd14633db 100644 --- a/zerver/migrations/0149_realm_emoji_drop_unique_constraint.py +++ b/zerver/migrations/0149_realm_emoji_drop_unique_constraint.py @@ -51,9 +51,11 @@ class LocalUploader(Uploader): def copy_files(self, src_path: str, dst_path: str) -> None: assert settings.LOCAL_UPLOADS_DIR is not None - src_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", src_path) + assert settings.LOCAL_AVATARS_DIR is not None + assert settings.LOCAL_FILES_DIR is not None + src_path = os.path.join(settings.LOCAL_AVATARS_DIR, src_path) self.mkdirs(src_path) - dst_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", dst_path) + dst_path = os.path.join(settings.LOCAL_AVATARS_DIR, dst_path) self.mkdirs(dst_path) shutil.copyfile(src_path, dst_path) diff --git a/zerver/tests/test_import_export.py b/zerver/tests/test_import_export.py index 9a0e653919..2cec639c2f 100644 --- a/zerver/tests/test_import_export.py +++ b/zerver/tests/test_import_export.py @@ -1329,9 +1329,10 @@ class RealmImportExportTest(ExportFile): self.assert_length(b"zulip!", uploaded_file.size) assert settings.LOCAL_UPLOADS_DIR is not None - attachment_file_path = os.path.join( - settings.LOCAL_UPLOADS_DIR, "files", uploaded_file.path_id - ) + assert settings.LOCAL_FILES_DIR is not None + assert settings.LOCAL_AVATARS_DIR is not None + + attachment_file_path = os.path.join(settings.LOCAL_FILES_DIR, uploaded_file.path_id) self.assertTrue(os.path.isfile(attachment_file_path)) # Test emojis @@ -1340,18 +1341,18 @@ class RealmImportExportTest(ExportFile): realm_id=imported_realm.id, emoji_file_name=realm_emoji.file_name, ) - emoji_file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", emoji_path) + emoji_file_path = os.path.join(settings.LOCAL_AVATARS_DIR, emoji_path) self.assertTrue(os.path.isfile(emoji_file_path)) # Test avatars user_profile = UserProfile.objects.get(full_name=user.full_name, realm=imported_realm) avatar_path_id = user_avatar_path(user_profile) + ".original" - avatar_file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", avatar_path_id) + avatar_file_path = os.path.join(settings.LOCAL_AVATARS_DIR, avatar_path_id) self.assertTrue(os.path.isfile(avatar_file_path)) # Test realm icon and logo upload_path = upload.upload_backend.realm_avatar_and_logo_path(imported_realm) - full_upload_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", upload_path) + full_upload_path = os.path.join(settings.LOCAL_AVATARS_DIR, upload_path) test_image_data = read_test_image_file("img.png") self.assertIsNotNone(test_image_data) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 5b5965af4f..89dba48cc1 100644 --- a/zerver/tests/test_upload.py +++ b/zerver/tests/test_upload.py @@ -1819,7 +1819,8 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): self.assertEqual(base, uri[: len(base)]) path_id = re.sub("/user_uploads/", "", uri) assert settings.LOCAL_UPLOADS_DIR is not None - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "files", path_id) + assert settings.LOCAL_FILES_DIR is not None + file_path = os.path.join(settings.LOCAL_FILES_DIR, path_id) self.assertTrue(os.path.isfile(file_path)) uploaded_file = Attachment.objects.get(owner=user_profile, path_id=path_id) @@ -1859,19 +1860,20 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): write_local_file("avatars", file_path + ".original", read_test_image_file("img.png")) assert settings.LOCAL_UPLOADS_DIR is not None - image_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", file_path + ".original") + assert settings.LOCAL_AVATARS_DIR is not None + image_path = os.path.join(settings.LOCAL_AVATARS_DIR, file_path + ".original") with open(image_path, "rb") as f: image_data = f.read() resized_avatar = resize_avatar(image_data) zerver.lib.upload.upload_backend.ensure_avatar_image(user_profile) - output_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", file_path + ".png") + output_path = os.path.join(settings.LOCAL_AVATARS_DIR, file_path + ".png") with open(output_path, "rb") as original_file: self.assertEqual(resized_avatar, original_file.read()) resized_avatar = resize_avatar(image_data, MEDIUM_AVATAR_SIZE) zerver.lib.upload.upload_backend.ensure_avatar_image(user_profile, is_medium=True) - output_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", file_path + "-medium.png") + output_path = os.path.join(settings.LOCAL_AVATARS_DIR, file_path + "-medium.png") with open(output_path, "rb") as original_file: self.assertEqual(resized_avatar, original_file.read()) @@ -1887,8 +1889,8 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): emoji_file_name=file_name, ) - assert settings.LOCAL_UPLOADS_DIR is not None - file_path = os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", emoji_path) + assert settings.LOCAL_AVATARS_DIR is not None + file_path = os.path.join(settings.LOCAL_AVATARS_DIR, emoji_path) with open(file_path + ".original", "rb") as original_file: self.assertEqual(read_test_image_file("img.png"), original_file.read()) @@ -1942,11 +1944,9 @@ class LocalStorageTest(UploadSerializeMixin, ZulipTestCase): with open(tarball_path, "w") as f: f.write("dummy") - assert settings.LOCAL_UPLOADS_DIR is not None + assert settings.LOCAL_AVATARS_DIR is not None uri = upload_export_tarball(user_profile.realm, tarball_path) - self.assertTrue( - os.path.isfile(os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars", tarball_path)) - ) + self.assertTrue(os.path.isfile(os.path.join(settings.LOCAL_AVATARS_DIR, tarball_path))) result = re.search(re.compile(r"([A-Za-z0-9\-_]{24})"), uri) if result is not None: diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 38e3e43f97..478c528b6d 100644 --- a/zproject/computed_settings.py +++ b/zproject/computed_settings.py @@ -40,6 +40,7 @@ from .configured_settings import ( EXTRA_INSTALLED_APPS, GOOGLE_OAUTH2_CLIENT_ID, IS_DEV_DROPLET, + LOCAL_UPLOADS_DIR, MEMCACHED_LOCATION, MEMCACHED_USERNAME, RATE_LIMITING_RULES, @@ -539,6 +540,9 @@ if PRODUCTION or IS_DEV_DROPLET or os.getenv("EXTERNAL_HOST") is not None: else: STATIC_URL = "http://localhost:9991/static/" +LOCAL_AVATARS_DIR = os.path.join(LOCAL_UPLOADS_DIR, "avatars") if LOCAL_UPLOADS_DIR else None +LOCAL_FILES_DIR = os.path.join(LOCAL_UPLOADS_DIR, "files") if LOCAL_UPLOADS_DIR else None + # ZulipStorage is a modified version of ManifestStaticFilesStorage, # and, like that class, it inserts a file hash into filenames # to prevent the browser from using stale files from cache. diff --git a/zproject/default_settings.py b/zproject/default_settings.py index 98f9be257c..5e08512ba9 100644 --- a/zproject/default_settings.py +++ b/zproject/default_settings.py @@ -141,6 +141,8 @@ S3_REGION: Optional[str] = None S3_ENDPOINT_URL: Optional[str] = None S3_SKIP_PROXY = True LOCAL_UPLOADS_DIR: Optional[str] = None +LOCAL_AVATARS_DIR: Optional[str] = None +LOCAL_FILES_DIR: Optional[str] = None MAX_FILE_UPLOAD_SIZE = 25 # Jitsi Meet video call integration; set to None to disable integration. diff --git a/zproject/dev_urls.py b/zproject/dev_urls.py index 080af83fea..d6fc220a23 100644 --- a/zproject/dev_urls.py +++ b/zproject/dev_urls.py @@ -125,10 +125,10 @@ i18n_urls = [ urls += i18n_urls # On a production instance, these files would be served by nginx. -if settings.LOCAL_UPLOADS_DIR is not None: +if settings.LOCAL_AVATARS_DIR is not None: avatars_url = path( "user_avatars/", serve, - {"document_root": os.path.join(settings.LOCAL_UPLOADS_DIR, "avatars")}, + {"document_root": os.path.join(settings.LOCAL_AVATARS_DIR)}, ) urls += [avatars_url]