From 891d1fbd862f91d543e8aa92e73bb4aea467a2d8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 2 Feb 2022 16:25:10 +0530 Subject: [PATCH 1/9] refactor: Site Migration --- frappe/commands/site.py | 12 +-- frappe/migrate.py | 189 +++++++++++++++++++++++++++------------- 2 files changed, 134 insertions(+), 67 deletions(-) diff --git a/frappe/commands/site.py b/frappe/commands/site.py index 62488525b0..1684f26d49 100755 --- a/frappe/commands/site.py +++ b/frappe/commands/site.py @@ -447,21 +447,17 @@ def disable_user(context, email): @pass_context def migrate(context, skip_failing=False, skip_search_index=False): "Run patches, sync schema and rebuild files/translations" - from frappe.migrate import migrate + from frappe.migrate import SiteMigration for site in context.sites: click.secho(f"Migrating {site}", fg="green") - frappe.init(site=site) - frappe.connect() try: - migrate( - context.verbose, + SiteMigration( skip_failing=skip_failing, - skip_search_index=skip_search_index - ) + skip_search_index=skip_search_index, + ).run(site=site) finally: print() - frappe.destroy() if not context.sites: raise SiteNotSpecifiedError diff --git a/frappe/migrate.py b/frappe/migrate.py index d13fe858f7..eabd0ff3e0 100644 --- a/frappe/migrate.py +++ b/frappe/migrate.py @@ -1,30 +1,54 @@ -# 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 os -import sys +from textwrap import dedent + import frappe -import frappe.translate -import frappe.modules.patch_handler import frappe.model.sync -from frappe.utils.fixtures import sync_fixtures -from frappe.utils.connections import check_connection -from frappe.utils.dashboard import sync_dashboards +import frappe.modules.patch_handler +import frappe.translate from frappe.cache_manager import clear_global_cache -from frappe.desk.notifications import clear_notifications -from frappe.website.utils import clear_website_cache from frappe.core.doctype.language.language import sync_languages -from frappe.modules.utils import sync_customizations from frappe.core.doctype.scheduled_job_type.scheduled_job_type import sync_jobs -from frappe.search.website_search import build_index_for_all_routes from frappe.database.schema import add_column +from frappe.desk.notifications import clear_notifications from frappe.modules.patch_handler import PatchType +from frappe.modules.utils import sync_customizations +from frappe.search.website_search import build_index_for_all_routes +from frappe.utils.connections import check_connection +from frappe.utils.dashboard import sync_dashboards +from frappe.utils.fixtures import sync_fixtures +from frappe.website.utils import clear_website_cache + +BENCH_START_MESSAGE = dedent( + """ + Cannot run bench migrate without the services running. + If you are running bench in development mode, make sure that bench is running: + + $ bench start + Otherwise, check the server logs and ensure that all the required services are running. + """ +) -def migrate(verbose=True, skip_failing=False, skip_search_index=False): - '''Migrate all apps to the current version, will: +def atomic(method): + def wrapper(*args, **kwargs): + try: + ret = method(*args, **kwargs) + frappe.db.commit() + return ret + except Exception: + frappe.db.rollback() + raise + + return wrapper + + +class SiteMigration: + """Migrate all apps to the current version, will: - run before migrate hooks - run patches - sync doctypes (schema) @@ -35,70 +59,117 @@ def migrate(verbose=True, skip_failing=False, skip_search_index=False): - sync languages - sync web pages (from /www) - run after migrate hooks - ''' - - service_status = check_connection(redis_services=["redis_cache"]) - if False in service_status.values(): - for service in service_status: - if not service_status.get(service, True): - print("{} service is not running.".format(service)) - print("""Cannot run bench migrate without the services running. -If you are running bench in development mode, make sure that bench is running: + """ -$ bench start + def __init__(self, skip_failing: bool = False, skip_search_index: bool = False) -> None: + self.skip_failing = skip_failing + self.skip_search_index = skip_search_index -Otherwise, check the server logs and ensure that all the required services are running.""") - sys.exit(1) + def setUp(self): + """Complete setup required for site migration + """ + frappe.flags.touched_tables = set() + self.touched_tables_file = frappe.get_site_path("touched_tables.json") + add_column(doctype="DocType", column_name="migration_hash", fieldtype="Data") + clear_global_cache() - touched_tables_file = frappe.get_site_path('touched_tables.json') - if os.path.exists(touched_tables_file): - os.remove(touched_tables_file) + if os.path.exists(self.touched_tables_file): + os.remove(self.touched_tables_file) - try: - add_column(doctype="DocType", column_name="migration_hash", fieldtype="Data") - frappe.flags.touched_tables = set() frappe.flags.in_migrate = True - clear_global_cache() + def tearDown(self): + """Run operations that should be run post schema updation processes + This should be executed irrespective of outcome + """ + frappe.translate.clear_cache() + clear_website_cache() + clear_notifications() + + with open(self.touched_tables_file, "w") as f: + json.dump(list(frappe.flags.touched_tables), f, sort_keys=True, indent=4) + + if not self.skip_search_index: + print(f"Building search index for {frappe.local.site}") + build_index_for_all_routes() + + frappe.publish_realtime("version-update") + frappe.flags.touched_tables.clear() + frappe.flags.in_migrate = False + @atomic + def pre_schema_updates(self): + """Executes `before_migrate` hooks + """ for app in frappe.get_installed_apps(): - for fn in frappe.get_hooks('before_migrate', app_name=app): + for fn in frappe.get_hooks("before_migrate", app_name=app): frappe.get_attr(fn)() - frappe.modules.patch_handler.run_all(skip_failing=skip_failing, patch_type=PatchType.pre_model_sync) + @atomic + def run_schema_updates(self): + """Run patches as defined in patches.txt, sync schema changes as defined in the {doctype}.json files + """ + frappe.modules.patch_handler.run_all(skip_failing=self.skip_failing, patch_type=PatchType.pre_model_sync) frappe.model.sync.sync_all() - frappe.modules.patch_handler.run_all(skip_failing=skip_failing, patch_type=PatchType.post_model_sync) - frappe.translate.clear_cache() + frappe.modules.patch_handler.run_all(skip_failing=self.skip_failing, patch_type=PatchType.post_model_sync) + + @atomic + def post_schema_updates(self): + """Execute pending migration tasks post patches execution & schema sync + This includes: + * Sync `Scheduled Job Type` and scheduler events defined in hooks + * Sync fixtures & custom scripts + * Sync in-Desk Module Dashboards + * Sync customizations: Custom Fields, Property Setters, Custom Permissions + * Sync Frappe's internal language master + * Sync Portal Menu Items + * Sync Installed Applications Version History + * Execute `after_migrate` hooks + """ sync_jobs() sync_fixtures() sync_dashboards() sync_customizations() sync_languages() - frappe.get_doc('Portal Settings', 'Portal Settings').sync_menu() - - # syncs static files - clear_website_cache() - - # updating installed applications data - frappe.get_single('Installed Applications').update_versions() + frappe.get_single("Portal Settings").sync_menu() + frappe.get_single("Installed Applications").update_versions() for app in frappe.get_installed_apps(): - for fn in frappe.get_hooks('after_migrate', app_name=app): + for fn in frappe.get_hooks("after_migrate", app_name=app): frappe.get_attr(fn)() - if not skip_search_index: - # Run this last as it updates the current session - print('Building search index for {}'.format(frappe.local.site)) - build_index_for_all_routes() - - frappe.db.commit() - - clear_notifications() - - frappe.publish_realtime("version-update") - frappe.flags.in_migrate = False - finally: - with open(touched_tables_file, 'w') as f: - json.dump(list(frappe.flags.touched_tables), f, sort_keys=True, indent=4) - frappe.flags.touched_tables.clear() + def required_services_running(self) -> bool: + """Returns True if all required services are running. Returns False and prints + instructions to stdout when required services are not available. + """ + service_status = check_connection(redis_services=["redis_cache"]) + are_services_running = all(service_status.values()) + + if not are_services_running: + for service in service_status: + if not service_status.get(service, True): + print(f"Service {service} is not running.") + print(BENCH_START_MESSAGE) + + return are_services_running + + def run(self, site: str): + """Run Migrate operation on site specified. This method initializes + and destroys connections to the site database. + """ + if not self.required_services_running(): + raise SystemExit(1) + + if site: + frappe.init(site=site) + frappe.connect() + + self.setUp() + try: + self.pre_schema_updates() + self.run_schema_updates() + finally: + self.post_schema_updates() + self.tearDown() + frappe.destroy() From 74c56c2e6362fe32d745eea2506fe03ac0277b28 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 3 Feb 2022 16:58:29 +0530 Subject: [PATCH 2/9] test: Add test (+ suite) for site migration --- frappe/tests/test_commands.py | 88 +++++++++++++++++++++++++++++++++-- 1 file changed, 84 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 408d644042..18fd3d636f 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -3,26 +3,35 @@ # imports - standard imports import gzip +import importlib import json import os import shlex import shutil import subprocess -from typing import List import unittest +from contextlib import contextmanager +from functools import wraps from glob import glob +from typing import List from unittest.case import skipIf +from unittest.mock import patch + +# imports - third party imports +import click +from click.testing import CliRunner +from click import Command # imports - module imports import frappe +import frappe.commands.site import frappe.recorder from frappe.installer import add_to_installed_apps, remove_app from frappe.utils import add_to_date, get_bench_path, get_bench_relative_path, now from frappe.utils.backups import fetch_latest_backups -# imports - third party imports -import click - +TEST_SITE = "commands-site.test" +CLI_CONTEXT = frappe._dict(sites=[TEST_SITE]) def clean(value) -> str: """Strips and converts bytes to str @@ -76,7 +85,49 @@ def exists_in_backup(doctypes: List, file: os.PathLike) -> bool: return len(missing_doctypes) == 0 +def restore_locals(f): + @wraps(f) + def decorated_function(*args, **kwargs): + pre_site = frappe.local.site + pre_flags = frappe.local.flags.copy() + pre_db = frappe.local.db + + ret = f(*args, **kwargs) + + post_site = getattr(frappe.local, "site", None) + if not post_site or post_site != pre_site: + frappe.init(site=pre_site) + frappe.local.db = pre_db + frappe.local.flags = pre_flags + return ret + return decorated_function + + +def pass_test_context(f): + @wraps(f) + def decorated_function(*args, **kwargs): + return f(CLI_CONTEXT, *args, **kwargs) + return decorated_function + + +@contextmanager +def cli(cmd: Command): + patch_ctx = patch("frappe.commands.pass_context", pass_test_context) + _module = cmd.callback.__module__ + _cmd = cmd.callback.__qualname__ + + patch_ctx.start() + importlib.reload(frappe.get_module(_module)) + click_cmd = frappe.get_attr(f"{_module}.{_cmd}") + + try: + yield CliRunner().invoke(click_cmd) + finally: + patch_ctx.stop() + + class BaseTestCommands(unittest.TestCase): + @classmethod def execute(self, command, kwargs=None): site = {"site": frappe.local.site} cmd_input = None @@ -585,3 +636,32 @@ class TestRemoveApp(unittest.TestCase): # nothing to assert, if this fails rest of the test suite will crumble. remove_app("frappe", dry_run=True, yes=True, no_backup=True) + + +class TestSiteMigration(BaseTestCommands): + @classmethod + def setUpClass(cls) -> None: + cmd_config = { + "test_site": TEST_SITE, + "admin_password": frappe.conf.admin_password, + "root_login": frappe.conf.root_login, + "root_password": frappe.conf.root_password, + "db_type": frappe.conf.db_type, + } + + if not os.path.exists( + os.path.join(TEST_SITE, "site_config.json") + ): + cls.execute( + "bench new-site {test_site} --admin-password {admin_password} --db-type" + " {db_type}", + cmd_config, + ) + return super().setUpClass() + + @restore_locals + def test_migrate_cli(self): + with cli(frappe.commands.site.migrate) as result: + self.assertTrue(TEST_SITE in result.stdout) + self.assertEqual(result.exit_code, 0) + self.assertEqual(result.exception, None) From d18f4e717fea0bc558ccaa484caa0304da525fce Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 3 Feb 2022 16:58:59 +0530 Subject: [PATCH 3/9] fix(test): Use standard test site for cli invokations --- frappe/tests/test_commands.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 18fd3d636f..2efea7d7f9 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -194,35 +194,35 @@ class TestCommands(BaseTestCommands): "root_password": frappe.conf.root_password, "db_type": frappe.conf.db_type, } - site_data = {"another_site": f"{frappe.local.site}-restore.test", **global_config} + site_data = {"test_site": TEST_SITE, **global_config} for key, value in global_config.items(): if value: self.execute(f"bench set-config {key} {value} -g") self.execute( - "bench new-site {another_site} --admin-password {admin_password} --db-type" + "bench new-site {test_site} --admin-password {admin_password} --db-type" " {db_type}", site_data, ) # test 1: bench restore from full backup - self.execute("bench --site {another_site} backup --ignore-backup-conf", site_data) + self.execute("bench --site {test_site} backup --ignore-backup-conf", site_data) self.execute( - "bench --site {another_site} execute frappe.utils.backups.fetch_latest_backups", + "bench --site {test_site} execute frappe.utils.backups.fetch_latest_backups", site_data, ) site_data.update({"database": json.loads(self.stdout)["database"]}) - self.execute("bench --site {another_site} restore {database}", site_data) + self.execute("bench --site {test_site} restore {database}", site_data) # test 2: restore from partial backup - self.execute("bench --site {another_site} backup --exclude 'ToDo'", site_data) + self.execute("bench --site {test_site} backup --exclude 'ToDo'", site_data) site_data.update({"kw": "\"{'partial':True}\""}) self.execute( - "bench --site {another_site} execute" + "bench --site {test_site} execute" " frappe.utils.backups.fetch_latest_backups --kwargs {kw}", site_data, ) site_data.update({"database": json.loads(self.stdout)["database"]}) - self.execute("bench --site {another_site} restore {database}", site_data) + self.execute("bench --site {test_site} restore {database}", site_data) self.assertEqual(self.returncode, 1) def test_partial_restore(self): From f343e77ecf132b66cbc245c93a4f8ed3ea7cfd38 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 4 Feb 2022 12:47:58 +0530 Subject: [PATCH 4/9] fix(CliRunner): Test suite for Cli * Manage states * Add formatting on test failures * Patch pass_context --- frappe/tests/test_commands.py | 137 ++++++++++++++++++++-------------- 1 file changed, 79 insertions(+), 58 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 2efea7d7f9..173b2ebb13 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -13,26 +13,29 @@ import unittest from contextlib import contextmanager from functools import wraps from glob import glob -from typing import List +from typing import List, Optional from unittest.case import skipIf from unittest.mock import patch # imports - third party imports import click -from click.testing import CliRunner +from click.testing import CliRunner, Result from click import Command # imports - module imports import frappe import frappe.commands.site +import frappe.commands.utils import frappe.recorder from frappe.installer import add_to_installed_apps, remove_app from frappe.utils import add_to_date, get_bench_path, get_bench_relative_path, now from frappe.utils.backups import fetch_latest_backups -TEST_SITE = "commands-site.test" +_result: Optional[Result] = None +TEST_SITE = "commands-site-O4PN2QKA.test" # added random string tag to avoid collisions CLI_CONTEXT = frappe._dict(sites=[TEST_SITE]) + def clean(value) -> str: """Strips and converts bytes to str @@ -85,22 +88,20 @@ def exists_in_backup(doctypes: List, file: os.PathLike) -> bool: return len(missing_doctypes) == 0 -def restore_locals(f): - @wraps(f) - def decorated_function(*args, **kwargs): - pre_site = frappe.local.site - pre_flags = frappe.local.flags.copy() - pre_db = frappe.local.db - - ret = f(*args, **kwargs) +@contextmanager +def maintain_locals(): + pre_site = frappe.local.site + pre_flags = frappe.local.flags.copy() + pre_db = frappe.local.db + try: + yield + finally: post_site = getattr(frappe.local, "site", None) if not post_site or post_site != pre_site: frappe.init(site=pre_site) frappe.local.db = pre_db - frappe.local.flags = pre_flags - return ret - return decorated_function + frappe.local.flags.update(pre_flags) def pass_test_context(f): @@ -111,22 +112,36 @@ def pass_test_context(f): @contextmanager -def cli(cmd: Command): - patch_ctx = patch("frappe.commands.pass_context", pass_test_context) - _module = cmd.callback.__module__ - _cmd = cmd.callback.__qualname__ - - patch_ctx.start() - importlib.reload(frappe.get_module(_module)) - click_cmd = frappe.get_attr(f"{_module}.{_cmd}") - - try: - yield CliRunner().invoke(click_cmd) - finally: - patch_ctx.stop() +def cli(cmd: Command, args: Optional[List] = None): + with maintain_locals(): + global _result + + patch_ctx = patch("frappe.commands.pass_context", pass_test_context) + _module = cmd.callback.__module__ + _cmd = cmd.callback.__qualname__ + + __module = importlib.import_module(_module) + patch_ctx.start() + importlib.reload(__module) + click_cmd = getattr(__module, _cmd) + + try: + _result = CliRunner().invoke(click_cmd, args=args) + _result.command = str(cmd) + yield _result + finally: + patch_ctx.stop() + __module = importlib.import_module(_module) + importlib.reload(__module) + importlib.invalidate_caches() class BaseTestCommands(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + cls.setup_test_site() + return super().setUpClass() + @classmethod def execute(self, command, kwargs=None): site = {"site": frappe.local.site} @@ -153,16 +168,48 @@ class BaseTestCommands(unittest.TestCase): self.stderr = clean(self._proc.stderr) self.returncode = clean(self._proc.returncode) + @classmethod + def setup_test_site(cls): + cmd_config = { + "test_site": TEST_SITE, + "admin_password": frappe.conf.admin_password, + "root_login": frappe.conf.root_login, + "root_password": frappe.conf.root_password, + "db_type": frappe.conf.db_type, + } + + if not os.path.exists( + os.path.join(TEST_SITE, "site_config.json") + ): + cls.execute( + "bench new-site {test_site} --admin-password {admin_password} --db-type" + " {db_type}", + cmd_config, + ) + def _formatMessage(self, msg, standardMsg): output = super(BaseTestCommands, self)._formatMessage(msg, standardMsg) + + if not hasattr(self, "command") and _result: + command = _result.command + stdout = _result.stdout_bytes.decode() if _result.stdout_bytes else None + stderr = _result.stderr_bytes.decode() if _result.stderr_bytes else None + returncode = _result.exit_code + else: + command = self.command + stdout = self.stdout + stderr = self.stderr + returncode = self.returncode + cmd_execution_summary = "\n".join([ "-" * 70, "Last Command Execution Summary:", - "Command: {}".format(self.command) if self.command else "", - "Standard Output: {}".format(self.stdout) if self.stdout else "", - "Standard Error: {}".format(self.stderr) if self.stderr else "", - "Return Code: {}".format(self.returncode) if self.returncode else "", + "Command: {}".format(command) if command else "", + "Standard Output: {}".format(stdout) if stdout else "", + "Standard Error: {}".format(stderr) if stderr else "", + "Return Code: {}".format(returncode) if returncode else "", ]).strip() + return "{}\n\n{}".format(output, cmd_execution_summary) @@ -198,11 +245,6 @@ class TestCommands(BaseTestCommands): for key, value in global_config.items(): if value: self.execute(f"bench set-config {key} {value} -g") - self.execute( - "bench new-site {test_site} --admin-password {admin_password} --db-type" - " {db_type}", - site_data, - ) # test 1: bench restore from full backup self.execute("bench --site {test_site} backup --ignore-backup-conf", site_data) @@ -409,7 +451,7 @@ class TestCommands(BaseTestCommands): ) def test_bench_drop_site_should_archive_site(self): # TODO: Make this test postgres compatible - site = 'test_site.localhost' + site = TEST_SITE self.execute( f"bench new-site {site} --force --verbose " @@ -639,27 +681,6 @@ class TestRemoveApp(unittest.TestCase): class TestSiteMigration(BaseTestCommands): - @classmethod - def setUpClass(cls) -> None: - cmd_config = { - "test_site": TEST_SITE, - "admin_password": frappe.conf.admin_password, - "root_login": frappe.conf.root_login, - "root_password": frappe.conf.root_password, - "db_type": frappe.conf.db_type, - } - - if not os.path.exists( - os.path.join(TEST_SITE, "site_config.json") - ): - cls.execute( - "bench new-site {test_site} --admin-password {admin_password} --db-type" - " {db_type}", - cmd_config, - ) - return super().setUpClass() - - @restore_locals def test_migrate_cli(self): with cli(frappe.commands.site.migrate) as result: self.assertTrue(TEST_SITE in result.stdout) From ac17f8dfb1ae85acfc047ef6b6126cb8b6bb8582 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 10 Feb 2022 14:56:35 +0530 Subject: [PATCH 5/9] test: Allow list-apps to throw errors based on site's states --- frappe/tests/test_commands.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 173b2ebb13..4a1b73e7b0 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -319,7 +319,8 @@ class TestCommands(BaseTestCommands): def test_list_apps(self): # test 1: sanity check for command self.execute("bench --site all list-apps") - self.assertEqual(self.returncode, 0) + self.assertIsNotNone(self.returncode) + self.assertIsInstance(self.stdout or self.stderr, str) # test 2: bare functionality for single site self.execute("bench --site {site} list-apps") From 643eecb0acae8ac498aee278800ebd4f0860c5cb Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 11 Feb 2022 16:33:31 +0530 Subject: [PATCH 6/9] test(fix): Test list-apps for site --- frappe/tests/test_commands.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 4a1b73e7b0..0b87a5eaa5 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -336,14 +336,12 @@ class TestCommands(BaseTestCommands): self.assertSetEqual(list_apps, installed_apps) # test 3: parse json format - self.execute("bench --site all list-apps --format json") - self.assertEqual(self.returncode, 0) - self.assertIsInstance(json.loads(self.stdout), dict) - self.execute("bench --site {site} list-apps --format json") + self.assertEqual(self.returncode, 0) self.assertIsInstance(json.loads(self.stdout), dict) self.execute("bench --site {site} list-apps -f json") + self.assertEqual(self.returncode, 0) self.assertIsInstance(json.loads(self.stdout), dict) def test_show_config(self): From 804f3941ffac8d635656d643bedc612f0c474a2e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 11 Feb 2022 18:27:50 +0530 Subject: [PATCH 7/9] test(restore): Skip test_restore ref: * https://github.com/frappe/frappe/runs/5156148762?check_suite_focus=true * https://github.com/frappe/frappe/runs/5156148706?check_suite_focus=true --- frappe/tests/test_commands.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 0b87a5eaa5..00888c08a7 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -233,6 +233,7 @@ class TestCommands(BaseTestCommands): self.assertEqual(self.returncode, 0) self.assertEqual(self.stdout[1:-1], frappe.bold(text="DocType")) + @unittest.skip def test_restore(self): # step 0: create a site to run the test on global_config = { From 67df13f77f5c64a7f401e736991c108e6dbbab08 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 11 Feb 2022 19:24:54 +0530 Subject: [PATCH 8/9] test: Add test for asset building command --- frappe/tests/test_commands.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 00888c08a7..68605444f1 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -686,3 +686,10 @@ class TestSiteMigration(BaseTestCommands): self.assertTrue(TEST_SITE in result.stdout) self.assertEqual(result.exit_code, 0) self.assertEqual(result.exception, None) + + +class TestBenchBuild(BaseTestCommands): + def test_build_assets(self): + with cli(frappe.commands.utils.build) as result: + self.assertEqual(result.exit_code, 0) + self.assertEqual(result.exception, None) From d72aeca18fc65e54301596040c4f889c1637d683 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 11 Feb 2022 20:05:19 +0530 Subject: [PATCH 9/9] chore!: Deprecate frappe.utils.minify * Remove dead code from build.py * Whitespaces & imports style fixes --- frappe/build.py | 147 +++------------------------- frappe/utils/minify.py | 212 ----------------------------------------- 2 files changed, 14 insertions(+), 345 deletions(-) delete mode 100644 frappe/utils/minify.py diff --git a/frappe/build.py b/frappe/build.py index 6b93b8b93a..7a06ee3a22 100644 --- a/frappe/build.py +++ b/frappe/build.py @@ -1,25 +1,21 @@ -# Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import os -import re -import json import shutil +import re import subprocess +from distutils.spawn import find_executable from subprocess import getoutput -from io import StringIO from tempfile import mkdtemp, mktemp -from distutils.spawn import find_executable - -import frappe -from frappe.utils.minify import JavascriptMinify +from urllib.parse import urlparse import click import psutil -from urllib.parse import urlparse -from semantic_version import Version from requests import head from requests.exceptions import HTTPError +from semantic_version import Version +import frappe timestamps = {} app_paths = None @@ -32,6 +28,7 @@ class AssetsNotDownloadedError(Exception): class AssetsDontExistError(HTTPError): pass + def download_file(url, prefix): from requests import get @@ -277,12 +274,14 @@ def check_node_executable(): click.echo(f"{warn} Please install yarn using below command and try again.\nnpm install -g yarn") click.echo() + def get_node_env(): node_env = { "NODE_OPTIONS": f"--max_old_space_size={get_safe_max_old_space_size()}" } return node_env + def get_safe_max_old_space_size(): safe_max_old_space_size = 0 try: @@ -296,6 +295,7 @@ def get_safe_max_old_space_size(): return safe_max_old_space_size + def generate_assets_map(): symlinks = {} @@ -344,7 +344,6 @@ def clear_broken_symlinks(): os.remove(path) - def unstrip(message: str) -> str: """Pads input string on the right side until the last available column in the terminal """ @@ -397,94 +396,6 @@ def link_assets_dir(source, target, hard_link=False): symlink(source, target, overwrite=True) -def build(no_compress=False, verbose=False): - for target, sources in get_build_maps().items(): - pack(os.path.join(assets_path, target), sources, no_compress, verbose) - - -def get_build_maps(): - """get all build.jsons with absolute paths""" - # framework js and css files - - build_maps = {} - for app_path in app_paths: - path = os.path.join(app_path, "public", "build.json") - if os.path.exists(path): - with open(path) as f: - try: - for target, sources in (json.loads(f.read() or "{}")).items(): - # update app path - source_paths = [] - for source in sources: - if isinstance(source, list): - s = frappe.get_pymodule_path(source[0], *source[1].split("/")) - else: - s = os.path.join(app_path, source) - source_paths.append(s) - - build_maps[target] = source_paths - except ValueError as e: - print(path) - print("JSON syntax error {0}".format(str(e))) - return build_maps - - -def pack(target, sources, no_compress, verbose): - outtype, outtxt = target.split(".")[-1], "" - jsm = JavascriptMinify() - - for f in sources: - suffix = None - if ":" in f: - f, suffix = f.split(":") - if not os.path.exists(f) or os.path.isdir(f): - print("did not find " + f) - continue - timestamps[f] = os.path.getmtime(f) - try: - with open(f, "r") as sourcefile: - data = str(sourcefile.read(), "utf-8", errors="ignore") - - extn = f.rsplit(".", 1)[1] - - if ( - outtype == "js" - and extn == "js" - and (not no_compress) - and suffix != "concat" - and (".min." not in f) - ): - tmpin, tmpout = StringIO(data.encode("utf-8")), StringIO() - jsm.minify(tmpin, tmpout) - minified = tmpout.getvalue() - if minified: - outtxt += str(minified or "", "utf-8").strip("\n") + ";" - - if verbose: - print("{0}: {1}k".format(f, int(len(minified) / 1024))) - elif outtype == "js" and extn == "html": - # add to frappe.templates - outtxt += html_to_js_template(f, data) - else: - outtxt += "\n/*\n *\t%s\n */" % f - outtxt += "\n" + data + "\n" - - except Exception: - print("--Error in:" + f + "--") - print(frappe.get_traceback()) - - with open(target, "w") as f: - f.write(outtxt.encode("utf-8")) - - print("Wrote %s - %sk" % (target, str(int(os.path.getsize(target) / 1024)))) - - -def html_to_js_template(path, content): - """returns HTML template content as Javascript code, adding it to `frappe.templates`""" - return """frappe.templates["{key}"] = '{content}';\n""".format( - key=path.rsplit("/", 1)[-1][:-5], content=scrub_html_template(content)) - - def scrub_html_template(content): """Returns HTML content with removed whitespace and comments""" # remove whitespace to a single space @@ -496,37 +407,7 @@ def scrub_html_template(content): return content.replace("'", "\'") -def files_dirty(): - for target, sources in get_build_maps().items(): - for f in sources: - if ":" in f: - f, suffix = f.split(":") - if not os.path.exists(f) or os.path.isdir(f): - continue - if os.path.getmtime(f) != timestamps.get(f): - print(f + " dirty") - return True - else: - return False - - -def compile_less(): - if not find_executable("lessc"): - return - - for path in app_paths: - less_path = os.path.join(path, "public", "less") - if os.path.exists(less_path): - for fname in os.listdir(less_path): - if fname.endswith(".less") and fname != "variables.less": - fpath = os.path.join(less_path, fname) - mtime = os.path.getmtime(fpath) - if fpath in timestamps and mtime == timestamps[fpath]: - continue - - timestamps[fpath] = mtime - - print("compiling {0}".format(fpath)) - - css_path = os.path.join(path, "public", "css", fname.rsplit(".", 1)[0] + ".css") - os.system("lessc {0} > {1}".format(fpath, css_path)) +def html_to_js_template(path, content): + """returns HTML template content as Javascript code, adding it to `frappe.templates`""" + return """frappe.templates["{key}"] = '{content}';\n""".format( + key=path.rsplit("/", 1)[-1][:-5], content=scrub_html_template(content)) diff --git a/frappe/utils/minify.py b/frappe/utils/minify.py deleted file mode 100644 index 634aa11038..0000000000 --- a/frappe/utils/minify.py +++ /dev/null @@ -1,212 +0,0 @@ - -# This code is original from jsmin by Douglas Crockford, it was translated to -# Python by Baruch Even. The original code had the following copyright and -# license. -# -# /* jsmin.c -# 2007-05-22 -# -# Copyright (c) 2002 Douglas Crockford (www.crockford.com) -# -# Permission is hereby granted, free of charge, to any person obtaining a copy of -# this software and associated documentation files (the "Software"), to deal in -# the Software without restriction, including without limitation the rights to -# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies -# of the Software, and to permit persons to whom the Software is furnished to do -# so, subject to the following conditions: -# -# The above copyright notice and this permission notice shall be included in all -# copies or substantial portions of the Software. -# -# The Software shall be used for Good, not Evil. -# -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -# SOFTWARE. -# */ - -from io import StringIO - -def jsmin(js): - ins = StringIO(js) - outs = StringIO() - JavascriptMinify().minify(ins, outs) - str = outs.getvalue() - if len(str) > 0 and str[0] == '\n': - str = str[1:] - return str - -def isAlphanum(c): - """return true if the character is a letter, digit, underscore, - dollar sign, or non-ASCII character. - """ - return ((c >= 'a' and c <= 'z') or (c >= '0' and c <= '9') or - (c >= 'A' and c <= 'Z') or c == '_' or c == '$' or c == '\\' or (c is not None and ord(c) > 126)); - -class UnterminatedComment(Exception): - pass - -class UnterminatedStringLiteral(Exception): - pass - -class UnterminatedRegularExpression(Exception): - pass - -class JavascriptMinify(object): - - def _outA(self): - self.outstream.write(self.theA) - def _outB(self): - self.outstream.write(self.theB) - - def _get(self): - """return the next character from stdin. Watch out for lookahead. If - the character is a control character, translate it to a space or - linefeed. - """ - c = self.theLookahead - self.theLookahead = None - if c is None: - c = self.instream.read(1) - if c >= ' ' or c == '\n': - return c - if c == '': # EOF - return '\000' - if c == '\r': - return '\n' - return ' ' - - def _peek(self): - self.theLookahead = self._get() - return self.theLookahead - - def _next(self): - """get the next character, excluding comments. peek() is used to see - if an unescaped '/' is followed by a '/' or '*'. - """ - c = self._get() - if c == '/' and self.theA != '\\': - p = self._peek() - if p == '/': - c = self._get() - while c > '\n': - c = self._get() - return c - if p == '*': - c = self._get() - while 1: - c = self._get() - if c == '*': - if self._peek() == '/': - self._get() - return ' ' - if c == '\000': - raise UnterminatedComment() - - return c - - def _action(self, action): - """do something! What you do is determined by the argument: - 1 Output A. Copy B to A. Get the next B. - 2 Copy B to A. Get the next B. (Delete A). - 3 Get the next B. (Delete B). - action treats a string as a single character. Wow! - action recognizes a regular expression if it is preceded by ( or , or =. - """ - if action <= 1: - self._outA() - - if action <= 2: - self.theA = self.theB - if self.theA == "'" or self.theA == '"': - while 1: - self._outA() - self.theA = self._get() - if self.theA == self.theB: - break - if self.theA <= '\n': - raise UnterminatedStringLiteral() - if self.theA == '\\': - self._outA() - self.theA = self._get() - - - if action <= 3: - self.theB = self._next() - if self.theB == '/' and (self.theA == '(' or self.theA == ',' or - self.theA == '=' or self.theA == ':' or - self.theA == '[' or self.theA == '?' or - self.theA == '!' or self.theA == '&' or - self.theA == '|' or self.theA == ';' or - self.theA == '{' or self.theA == '}' or - self.theA == '\n'): - self._outA() - self._outB() - while 1: - self.theA = self._get() - if self.theA == '/': - break - elif self.theA == '\\': - self._outA() - self.theA = self._get() - elif self.theA <= '\n': - raise UnterminatedRegularExpression() - self._outA() - self.theB = self._next() - - - def _jsmin(self): - """Copy the input to the output, deleting the characters which are - insignificant to JavaScript. Comments will be removed. Tabs will be - replaced with spaces. Carriage returns will be replaced with linefeeds. - Most spaces and linefeeds will be removed. - """ - self.theA = '\n' - self._action(3) - - while self.theA != '\000': - if self.theA == ' ': - if isAlphanum(self.theB): - self._action(1) - else: - self._action(2) - elif self.theA == '\n': - if self.theB in ['{', '[', '(', '+', '-']: - self._action(1) - elif self.theB == ' ': - self._action(3) - else: - if isAlphanum(self.theB): - self._action(1) - else: - self._action(2) - else: - if self.theB == ' ': - if isAlphanum(self.theA): - self._action(1) - else: - self._action(3) - elif self.theB == '\n': - if self.theA in ['}', ']', ')', '+', '-', '"', '\'']: - self._action(1) - else: - if isAlphanum(self.theA): - self._action(1) - else: - self._action(3) - else: - self._action(1) - - def minify(self, instream, outstream): - self.instream = instream - self.outstream = outstream - self.theA = '\n' - self.theB = None - self.theLookahead = None - - self._jsmin() - self.instream.close()