From 726fcfdb796407e71c7a459a255b201417ef8c87 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sun, 25 Dec 2022 23:19:11 +0530 Subject: [PATCH 01/58] refactor: qb.engine - simplify - qb.engine.get_query -> qb.get_query - qb.engine.build_conditions -> qb.get_query --- frappe/__init__.py | 4 +- frappe/database/database.py | 22 +- frappe/database/query.py | 707 +++++++----------- .../desk/doctype/number_card/number_card.py | 2 +- frappe/desk/listview.py | 2 +- frappe/query_builder/__init__.py | 2 +- frappe/query_builder/functions.py | 2 +- frappe/query_builder/utils.py | 5 +- frappe/tests/test_db_query.py | 4 +- frappe/tests/test_query.py | 88 +-- frappe/utils/goal.py | 2 +- 11 files changed, 347 insertions(+), 493 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 2d491ca068..6b9d157003 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -23,7 +23,7 @@ import click from werkzeug.local import Local, release_local from frappe.query_builder import ( - get_qb_engine, + get_query, get_query_builder, patch_query_aggregation, patch_query_execute, @@ -244,7 +244,7 @@ def init(site: str, sites_path: str = ".", new_site: bool = False) -> None: local.session = _dict() local.dev_server = _dev_server local.qb = get_query_builder(local.conf.db_type or "mariadb") - local.qb.engine = get_qb_engine() + local.qb.get_query = get_query setup_module_map() if not _qb_patched.get(local.conf.db_type): diff --git a/frappe/database/database.py b/frappe/database/database.py index dfcc9dfe58..acbd28c9d7 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -620,7 +620,7 @@ class Database: return [map(values.get, fields)] else: - r = frappe.qb.engine.get_query( + r = frappe.qb.get_query( "Singles", filters={"field": ("in", tuple(fields)), "doctype": doctype}, fields=["field", "value"], @@ -653,7 +653,7 @@ class Database: # Get coulmn and value of the single doctype Accounts Settings account_settings = frappe.db.get_singles_dict("Accounts Settings") """ - queried_result = frappe.qb.engine.get_query( + queried_result = frappe.qb.get_query( "Singles", filters={"doctype": doctype}, fields=["field", "value"], @@ -726,7 +726,7 @@ class Database: if cache and fieldname in self.value_cache[doctype]: return self.value_cache[doctype][fieldname] - val = frappe.qb.engine.get_query( + val = frappe.qb.get_query( table="Singles", filters={"doctype": doctype, "field": fieldname}, fields="value", @@ -766,10 +766,10 @@ class Database: distinct=False, limit=None, ): - query = frappe.qb.engine.get_query( + query = frappe.qb.get_query( table=doctype, filters=filters, - orderby=order_by, + order_by=order_by, for_update=for_update, fields=fields, distinct=distinct, @@ -795,7 +795,7 @@ class Database: as_dict=False, ): if names := list(filter(None, names)): - return frappe.qb.engine.get_query( + return frappe.qb.get_query( doctype, fields=field, filters=names, @@ -852,7 +852,7 @@ class Database: frappe.clear_document_cache(dt, dt) else: - query = frappe.qb.engine.build_conditions(table=dt, filters=dn, update=True) + query = frappe.qb.get_query(table=dt, filters=dn, update=True) if isinstance(dn, str): frappe.clear_document_cache(dt, dn) @@ -1017,9 +1017,9 @@ class Database: cache_count = frappe.cache().get_value(f"doctype:count:{dt}") if cache_count is not None: return cache_count - count = frappe.qb.engine.get_query( - table=dt, filters=filters, fields=Count("*"), distinct=distinct - ).run(debug=debug)[0][0] + count = frappe.qb.get_query(table=dt, filters=filters, fields=Count("*"), distinct=distinct).run( + debug=debug + )[0][0] if not filters and cache: frappe.cache().set_value(f"doctype:count:{dt}", count, expires_in_sec=86400) return count @@ -1160,7 +1160,7 @@ class Database: Doctype name can be passed directly, it will be pre-pended with `tab`. """ filters = filters or kwargs.get("conditions") - query = frappe.qb.engine.build_conditions(table=doctype, filters=filters).delete() + query = frappe.qb.get_query(table=doctype, filters=filters).delete() if "debug" not in kwargs: kwargs["debug"] = debug return query.run(**kwargs) diff --git a/frappe/database/query.py b/frappe/database/query.py index a9dab02744..88de7f7088 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -8,6 +8,7 @@ from typing import TYPE_CHECKING, Callable import sqlparse from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder +from pypika.queries import QueryBuilder import frappe from frappe import _ @@ -171,18 +172,16 @@ def table_from_string(table: str) -> "DocType": return frappe.qb.DocType(table_name=table_name) -def get_nested_set_hierarchy_result(hierarchy: str, field: str, table: str): - ref_doctype = table +def get_nested_set_hierarchy_result(doctype: str, name: str, hierarchy: str): + table = frappe.qb.DocType(doctype) try: - lft, rgt = ( - frappe.qb.from_(ref_doctype).select("lft", "rgt").where(Field("name") == field).run()[0] - ) + lft, rgt = frappe.qb.from_(table).select("lft", "rgt").where(table.name == name).run()[0] except IndexError: lft, rgt = None, None if hierarchy in ("descendants of", "not descendants of"): result = ( - frappe.qb.from_(ref_doctype) + frappe.qb.from_(table) .select(Field("name")) .where(Field("lft") > lft) .where(Field("rgt") < rgt) @@ -192,7 +191,7 @@ def get_nested_set_hierarchy_result(hierarchy: str, field: str, table: str): else: # Get ancestor elements of a DocType with a tree structure result = ( - frappe.qb.from_(ref_doctype) + frappe.qb.from_(table) .select(Field("name")) .where(Field("lft") < lft) .where(Field("rgt") > rgt) @@ -232,37 +231,67 @@ OPERATOR_MAP: dict[str, Callable] = { class Engine: tables: dict[str, str] = {} - @cached_property - def OPERATOR_MAP(self): - # default operators - all_operators = OPERATOR_MAP.copy() + def get_query( + self, + table: str, + fields: list | tuple | None = None, + filters: dict[str, str | int] | str | int | list[list | str | int] | None = None, + pluck: str | None = None, + order_by: str | None = None, + group_by: str | None = None, + limit: int | None = None, + offset: int | None = None, + distinct: bool = False, + for_update: bool = False, + update: bool = False, + into: bool = False, + ) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: + # Clean up state before each query + self.is_mariadb = frappe.db.db_type == "mariadb" + self.is_postgres = frappe.db.db_type == "postgres" + self.tables = {} + self.implicit_joins = set() - # TODO: update with site-specific custom operators / removed previous buggy implementation - if frappe.get_hooks("filters_config"): - from frappe.utils.commands import warn + self.doctype = table + self.table = self.get_table(table) - warn( - "The 'filters_config' hook used to add custom operators is not yet implemented" - " in frappe.db.query engine. Use db_query (frappe.get_list) instead." - ) + if update: + self.query = frappe.qb.update(self.table) + elif into: + self.query = frappe.qb.into(self.table) + else: + self.query = frappe.qb.from_(self.table) - return all_operators + self.fields = self.parse_fields(fields) + if not self.fields: + self.fields = [getattr(self.table, pluck or "name")] - def get_condition(self, table: str | Table, **kwargs) -> frappe.qb: - """Get initial table object + for field in self.fields: + if isinstance(field, DynamicTableField): + self.query = field.apply(self.query) + else: + self.query = self.query.select(field) - Args: - table (str): DocType + self.apply_filters(filters) + self.apply_implicit_joins() + self.apply_order_by(order_by) - Returns: - frappe.qb: DocType with initial condition - """ - table_object = self.get_table(table) - if kwargs.get("update"): - return frappe.qb.update(table_object) - if kwargs.get("into"): - return frappe.qb.into(table_object) - return frappe.qb.from_(table_object) + if limit: + self.query = self.query.limit(limit) + + if offset: + self.query = self.query.offset(offset) + + if distinct: + self.query = self.query.distinct() + + if for_update: + self.query = self.query.for_update() + + if group_by: + self.query = self.query.groupby(group_by) + + return self.query def get_table(self, table_name: str | Table) -> Table: if isinstance(table_name, Table): @@ -272,178 +301,93 @@ class Engine: self.tables[table_name] = frappe.qb.DocType(table_name) return self.tables[table_name] - def criterion_query(self, table: str, criterion: Criterion, **kwargs) -> frappe.qb: - """Generate filters from Criterion objects - - Args: - table (str): DocType - criterion (Criterion): Filters - - Returns: - frappe.qb: condition object - """ - condition = self.add_conditions(self.get_condition(table, **kwargs), **kwargs) - return condition.where(criterion) - - def add_conditions(self, conditions: frappe.qb, **kwargs): - """Adding additional conditions - - Args: - conditions (frappe.qb): built conditions - - Returns: - conditions (frappe.qb): frappe.qb object - """ - if kwargs.get("orderby") and kwargs.get("orderby") != "KEEP_DEFAULT_ORDERING": - orderby = kwargs.get("orderby") - if isinstance(orderby, str) and len(orderby.split()) > 1: - for ordby in orderby.split(","): - if ordby := ordby.strip(): - orderby, order = change_orderby(ordby) - conditions = conditions.orderby(orderby, order=order) - else: - conditions = conditions.orderby(orderby, order=kwargs.get("order") or Order.desc) - - if kwargs.get("limit"): - conditions = conditions.limit(kwargs.get("limit")) - conditions = conditions.offset(kwargs.get("offset", 0)) - - if kwargs.get("distinct"): - conditions = conditions.distinct() - - if kwargs.get("for_update"): - conditions = conditions.for_update() - - if kwargs.get("groupby"): - conditions = conditions.groupby(kwargs.get("groupby")) - - return conditions - - def misc_query(self, table: str, filters: list | tuple = None, **kwargs): - """Build conditions using the given Lists or Tuple filters - - Args: - table (str): DocType - filters (Union[List, Tuple], optional): Filters. Defaults to None. - """ - conditions = self.get_condition(table, **kwargs) + def apply_filters( + self, filters: dict[str, str | int | list] | str | int | list[list] | None = None + ): if not filters: - return conditions - if isinstance(filters, list): - for f in filters: - if isinstance(f, (list, tuple)): - _operator = self.OPERATOR_MAP[f[-2].casefold()] - if len(f) == 4: - table_object = self.get_table(f[0]) - _field = table_object[f[1]] - else: - _field = Field(f[0]) - conditions = conditions.where(_operator(_field, f[-1])) - elif isinstance(f, dict): - conditions = self.dict_query(table, f, **kwargs) - else: - _operator = self.OPERATOR_MAP[filters[1].casefold()] - if not isinstance(filters[0], str): - conditions = self.make_function_for_filters(filters[0], filters[2]) - break - conditions = conditions.where(_operator(Field(filters[0]), filters[2])) - break + return - return self.add_conditions(conditions, **kwargs) - - def dict_query(self, table: str, filters: dict[str, str | int] = None, **kwargs) -> frappe.qb: - """Build conditions using the given dictionary filters - - Args: - table (str): DocType - filters (Dict[str, Union[str, int]], optional): Filters. Defaults to None. - - Returns: - frappe.qb: conditions object - """ - conditions = self.get_condition(table, **kwargs) - if isinstance(table, str): - table = frappe.qb.DocType(table) - if not filters: - conditions = self.add_conditions(conditions, **kwargs) - return conditions - - for key, value in filters.items(): - if isinstance(value, bool): - filters.update({key: str(int(value))}) - - filters = { - (self.get_function_object(k) if has_function(k) else k): v for k, v in filters.items() - } - for key in filters: - value = filters.get(key) - _operator = self.OPERATOR_MAP["="] - - if not isinstance(key, str): - conditions = conditions.where(self.make_function_for_filters(key, value)) - continue - # Nested set support - if isinstance(value, (list, tuple)): - if value[0] in self.OPERATOR_MAP["nested_set"]: - hierarchy, _field = value - result = get_nested_set_hierarchy_result(hierarchy, _field, table) - _operator = ( - self.OPERATOR_MAP["not in"] - if hierarchy in ("not ancestors of", "not descendants of") - else self.OPERATOR_MAP["in"] - ) - if result: - result = list(itertools.chain.from_iterable(result)) - conditions = conditions.where(_operator(getattr(table, key), result)) - else: - conditions = conditions.where(_operator(getattr(table, key), ("",))) - # Allow additional conditions - break - - _operator = self.OPERATOR_MAP[value[0].casefold()] - _value = value[1] if value[1] else ("",) - conditions = conditions.where(_operator(getattr(table, key), _value)) - else: - if value is not None: - conditions = conditions.where(_operator(getattr(table, key), value)) - else: - _table = conditions._from[0] - field = getattr(_table, key) - conditions = conditions.where(field.isnull()) - - return self.add_conditions(conditions, **kwargs) - - def build_conditions( - self, table: str, filters: dict[str, str | int] | str | int = None, **kwargs - ) -> frappe.qb: - """Build conditions for sql query - - Args: - filters (Union[Dict[str, Union[str, int]], str, int]): conditions in Dict - table (str): DocType - - Returns: - frappe.qb: frappe.qb conditions object - """ - if isinstance(filters, int) or isinstance(filters, str): + if isinstance(filters, (str, int)): filters = {"name": str(filters)} if isinstance(filters, Criterion): - criterion = self.criterion_query(table, filters, **kwargs) + self.query = self.query.where(filters) + + elif isinstance(filters, dict): + self.apply_dict_filters(filters) elif isinstance(filters, (list, tuple)): - criterion = self.misc_query(table, filters, **kwargs) + self.apply_list_filters(filters) + def apply_list_filters(self, filters: list[list]): + for filter in filters: + if len(filter) == 2: + field, value = filter + self._apply_filter(field, value) + elif len(filter) == 3: + field, operator, value = filter + self._apply_filter(field, value, operator) + elif len(filter) == 4: + doctype, field, operator, value = filter + self._apply_filter(field, value, operator, doctype) + + def apply_dict_filters(self, filters: dict[str, str | int | list]): + for key in filters: + value = filters.get(key) + self._apply_filter(key, value) + + def _apply_filter( + self, field: str, value: str | int | list | None, operator: str = "=", doctype: str | None = None + ): + _field = field + _value = value + _operator = operator + + if has_function(field): + _field = self.get_function_object(field) + elif not doctype or doctype == self.doctype: + _field = self.table[field] + elif doctype: + _field = self.get_table(doctype)[field] + + # keep track of implicit join if child table is referenced + if doctype and doctype != self.doctype: + meta = frappe.get_meta(doctype) + if meta.istable: + self.implicit_joins.add((doctype, "child")) + + if isinstance(_value, (str, int)): + _value = str(_value) + elif isinstance(_value, (list, tuple)): + _operator, _value = _value + elif isinstance(_value, bool): + _value = int(_value) + + if isinstance(_value, str) and has_function(_value): + _value = self.get_function_object(_value) + + # Nested set + if _operator in self.OPERATOR_MAP["nested_set"]: + hierarchy = _operator + docname = _value + result = get_nested_set_hierarchy_result(self.doctype, docname, hierarchy) + operator_fn = ( + self.OPERATOR_MAP["not in"] + if hierarchy in ("not ancestors of", "not descendants of") + else self.OPERATOR_MAP["in"] + ) + if result: + result = list(itertools.chain.from_iterable(result)) + self.query = self.query.where(operator_fn(_field, result)) + else: + self.query = self.query.where(operator_fn(_field, ("",))) + return + + operator_fn = self.OPERATOR_MAP[_operator.casefold()] + if _value is None and isinstance(_field, Field): + self.query = self.query.where(_field.isnull()) else: - criterion = self.dict_query(filters=filters, table=table, **kwargs) - - return criterion - - def make_function_for_filters(self, key, value: int | str): - value = list(value) - if isinstance(value[1], str) and has_function(value[1]): - value[1] = self.get_function_object(value[1]) - return OPERATOR_MAP[value[0].casefold()](key, value[1]) + self.query = self.query.where(operator_fn(_field, _value)) def get_function_object(self, field: str) -> "Function": """Expects field to look like 'SUM(*)' or 'name' or something similar. Returns PyPika Function object""" @@ -495,84 +439,12 @@ class Engine: # Fall back for functions not present in `SqlFunctions`` return Function(func, *_args, alias=alias or None) - def function_objects_from_string(self, fields): - fields = list(map(lambda str: str.strip(), COMMA_PATTERN.split(fields))) - return self.function_objects_from_list(fields=fields) - - def function_objects_from_list(self, fields): - functions = [] - for field in fields: - field = field.casefold() if (isinstance(field, str) and "`" not in field) else field - if not issubclass(type(field), Criterion): - if any([f"{func}(" in field for func in SQL_FUNCTIONS]) or "(" in field: - functions.append(field) - - return [self.get_function_object(function) for function in functions] - - def remove_string_functions(self, fields, function_objects): - """Remove string functions from fields which have already been converted to function objects""" - - def _remove_string_aliasing(function, fields: list | str): - if function.alias: - to_replace = " as " + function.alias.casefold() - if to_replace in fields: - fields = fields.replace(to_replace, "") - elif " as " + f"`{function.alias.casefold()}" in fields: - fields = fields.replace(" as " + f"`{function.alias.casefold()}`", "") - return fields - - for function in function_objects: - if isinstance(fields, str): - fields = _remove_string_aliasing(function, fields) - fields = BRACKETS_PATTERN.sub("", re.sub(function.name, "", fields, flags=re.IGNORECASE)) - # Check if only comma is left in fields after stripping functions. - if "," in fields and (len(fields.strip()) == 1): - fields = "" - else: - updated_fields = [] - for field in fields: - if isinstance(field, str): - field = _remove_string_aliasing(function, field) - substituted_string = ( - BRACKETS_PATTERN.sub("", field).strip().casefold() - if "`" not in field - else BRACKETS_PATTERN.sub("", field).strip() - ) - # This is done to avoid casefold of table name. - if substituted_string.casefold() == function.name.casefold(): - replaced_string = substituted_string.casefold().replace(function.name.casefold(), "") - else: - replaced_string = substituted_string.replace(function.name.casefold(), "") - updated_fields.append(replaced_string) - fields = [field for field in updated_fields if field] - return fields - - def get_fieldnames_from_child_table(self, doctype, fields): - # Hacky and flaky implementation of implicit joins. - # convert child_table.fieldname to `tabChild DocType`.`fieldname` - _fields = [] - for field in fields: - if "." in field and "tab" not in field: - alias = None - if " as " in field: - field, alias = field.split(" as ") - fieldname, linked_fieldname = field.split(".") - linked_doctype = frappe.get_meta(doctype).get_field(fieldname).options - - field = f"`tab{linked_doctype}`.`{linked_fieldname}`" - if alias: - field = f"{field} {alias}" - _fields.append(field) - return _fields - def sanitize_fields(self, fields: str | list | tuple): - is_mariadb = frappe.db.db_type == "mariadb" - def _sanitize_field(field: str): if not isinstance(field, str): return field stripped_field = sqlparse.format(field, strip_comments=True, keyword_case="lower") - if is_mariadb: + if self.is_mariadb: return MARIADB_SPECIFIC_COMMENT.sub("", stripped_field) return stripped_field @@ -583,174 +455,88 @@ class Engine: return fields - def get_list_fields(self, table: str, fields: list) -> list: - updated_fields = [] - if issubclass(type(fields), Criterion) or "*" in fields: - return fields - fields = self.get_fieldnames_from_child_table(doctype=table, fields=fields) - for field in fields: - if not isinstance(field, Criterion) and field: - if " as " in field: - field, reference = field.split(" as ") - if "`" in field: - updated_fields.append(PseudoColumnMapper(f"{field} {reference}")) - else: - updated_fields.append(Field(field.strip()).as_(reference)) - elif "`" in str(field): - updated_fields.append(PseudoColumnMapper(field.strip())) - else: - updated_fields.append(Field(field)) - return updated_fields + def parse_string_field(self, field: str): + if field == "*": + return self.table.star + alias = None + if " as " in field: + field, alias = field.split(" as ") + if "`" in field: + if alias: + return PseudoColumnMapper(f"{field} {alias}") + return PseudoColumnMapper(field) + if alias: + return self.table[field].as_(alias) + return self.table[field] - def get_string_fields(self, fields: str) -> Field: - if fields == "*": - return fields - if "`" in fields: - fields = PseudoColumnMapper(fields) - if " as " in str(fields): - fields, reference = str(fields).split(" as ") - if "`" in str(fields): - fields = PseudoColumnMapper(f"{fields} {reference}") - else: - fields = Field(fields).as_(reference) - return fields - - def set_fields(self, table: str, fields, **kwargs) -> list: - fields = kwargs.get("pluck") if kwargs.get("pluck") else fields or "name" + def parse_fields(self, fields: str | list | tuple | None) -> list: + if not fields: + return [] fields = self.sanitize_fields(fields) - if isinstance(fields, list) and None in fields and Field not in fields: - return None - function_objects = [] - is_list = isinstance(fields, (list, tuple, set)) - if is_list and len(fields) == 1: - fields = fields[0] - is_list = False + if isinstance(fields, (list, tuple, set)) and None in fields and Field not in fields: + return [] - if is_list: - function_objects += self.function_objects_from_list(fields=fields) + if not isinstance(fields, (list, tuple)): + fields = [fields] - is_str = isinstance(fields, str) - if is_str: - fields = fields.casefold() if "`" not in fields else fields - function_objects += self.function_objects_from_string(fields=fields) - - fields = self.remove_string_functions(fields, function_objects) - - if is_str and "," in fields: - fields = [field.replace(" ", "") if "as" not in field else field for field in fields.split(",")] - is_list, is_str = True, False - - if is_str: - fields = self.get_string_fields(fields) - if not is_str and fields: - fields = self.get_list_fields(table, fields) - - # Need to check instance again since fields modified. - if not isinstance(fields, (list, tuple, set)): - fields = [fields] if fields else [] - - fields.extend(function_objects) - return fields - - def join_child_tables( - self, - criterion: Criterion, - join_type: str, - child_table: Table, - parent_table: Table, - ) -> Criterion: - if self.joined_tables.get(join_type) != child_table: - criterion = getattr(criterion, join_type)(child_table).on( - (child_table.parent == parent_table.name) - & (child_table.parenttype == TAB_PATTERN.sub("", parent_table._table_name)) - ) - self.joined_tables[join_type] = child_table - return criterion - - def join(self, criterion, fields, table, join_type): - """Handles all join operations on criterion objects""" - has_join = False - table_pattern = ( - re.compile(r"`\btab\w+") if frappe.db.db_type == "mariadb" else re.compile(r'"\btab\w+') - ) - - def _update_pypika_fields(field): - if not is_pypika_function_object(field): - field = field if isinstance(field, (str, PseudoColumnMapper)) else field.get_sql() - if not table_pattern.search(str(field)): - if isinstance(field, PseudoColumnMapper): - field = field.get_sql() - return getattr(frappe.qb.DocType(table), field) - else: - return field + def parse_field(field: str): + if has_function(field): + return self.get_function_object(field) + elif parsed := DynamicTableField.parse(field, self.doctype): + return parsed else: - field.args = [getattr(frappe.qb.DocType(table), arg.get_sql()) for arg in field.args] - return field + return self.parse_string_field(field) - if not isinstance(fields, Criterion): - for field in fields: - # Only perform this bit if foreign doctype in fields - if ( - not is_pypika_function_object(field) - and (str(field).startswith('"tab') or str(field).startswith("`tab")) - and (f"`tab{table}`" not in str(field) and f'tab{table}"' not in str(field)) - ): - has_join = True - child_table = table_from_string(str(field)) - parent_table = frappe.qb.DocType(table) if not isinstance(table, Table) else table - criterion = self.join_child_tables( - criterion=criterion, - join_type=join_type, - child_table=child_table, - parent_table=parent_table, - ) + _fields = [] + for field in fields: + if isinstance(field, Criterion): + _fields.append(field) + elif isinstance(field, str): + if "," in field: + field = field.casefold() if "`" not in field else field + field_list = COMMA_PATTERN.split(field) + for field in field_list: + if _field := field.strip(): + _fields.append(parse_field(_field)) + else: + _fields.append(parse_field(field)) - if has_join: - fields = [_update_pypika_fields(field) for field in fields] + return _fields - if len(self.tables) > 1: - parent_table = self.tables[table] - child_tables = list(self.tables.values())[1:] - for child_table in child_tables: - criterion = self.join_child_tables( - criterion, - join_type=join_type, - child_table=child_table, - parent_table=parent_table, + def apply_implicit_joins(self): + for d in self.implicit_joins: + doctype, join_type = d + table = self.get_table(doctype) + if join_type == "child": + self.query = self.query.left_join(table).on( + (table.parent == self.table.name) & (table.parenttype == self.doctype) ) - return criterion, fields + def apply_order_by(self, order_by: str | None): + if not order_by or order_by == "KEEP_DEFAULT_ORDERING": + return + for declaration in order_by.split(","): + if _order_by := declaration.strip(): + parts = _order_by.split(" ") + order_field, order_direction = parts[0], parts[1] if len(parts) > 1 else "asc" + order_direction = Order.asc if order_direction.lower() == "asc" else Order.desc + self.query = self.query.orderby(order_field, order=order_direction) - def get_query( - self, - table: str, - fields: list | tuple, - filters: dict[str, str | int] | str | int | list[list | str | int] = None, - **kwargs, - ) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: - # Clean up state before each query - self.tables = {} - self.joined_tables = {} - self.linked_doctype = None - self.fieldname = None + @cached_property + def OPERATOR_MAP(self): + # default operators + all_operators = OPERATOR_MAP.copy() - criterion = self.build_conditions(table, filters, **kwargs) - fields = self.set_fields(table, fields, **kwargs) - join_type = kwargs.get("join").replace(" ", "_") if kwargs.get("join") else "left_join" - criterion, fields = self.join( - criterion=criterion, fields=fields, table=table, join_type=join_type - ) + # TODO: update with site-specific custom operators / removed previous buggy implementation + if frappe.get_hooks("filters_config"): + from frappe.utils.commands import warn - if isinstance(fields, (list, tuple)): - query = criterion.select(*fields) + warn( + "The 'filters_config' hook used to add custom operators is not yet implemented" + " in frappe.db.query engine. Use db_query (frappe.get_list) instead." + ) - elif isinstance(fields, Criterion): - query = criterion.select(fields) - - else: - query = criterion.select(fields) - - return query + return all_operators class Permission: @@ -781,3 +567,80 @@ class Permission: @staticmethod def get_tables_from_query(query: str): return [table for table in WORDS_PATTERN.findall(query) if table.startswith("tab")] + + +class DynamicTableField: + def __init__( + self, + doctype: str, + fieldname: str, + parent_doctype: str, + alias: str | None = None, + ) -> None: + self.doctype = doctype + self.fieldname = fieldname + self.alias = alias + self.parent_doctype = parent_doctype + + def __str__(self) -> str: + table_name = f"`tab{self.doctype}`" + fieldname = f"`{self.fieldname}`" + if frappe.db.db_type == "postgres": + table_name = table_name.replace("`", '"') + fieldname = fieldname.replace("`", '"') + alias = f"AS {self.alias}" if self.alias else "" + return f"{table_name}.{fieldname} {alias}".strip() + + @staticmethod + def parse(field: str, doctype: str): + if "." in field: + alias = None + if " as " in field: + field, alias = field.split(" as ") + if field.startswith("`tab") or field.startswith('"tab'): + _, child_doctype, child_field = re.search(r'([`"])tab(.+?)\1.\1(.+)\1', field).groups() + if child_doctype == doctype: + return + return ChildTableField(child_doctype, child_field, doctype, alias=alias) + else: + linked_fieldname, fieldname = field.split(".") + linked_field = frappe.get_meta(doctype).get_field(linked_fieldname) + linked_doctype = linked_field.options + if linked_field.fieldtype == "Link": + return LinkTableField(linked_doctype, fieldname, doctype, linked_fieldname, alias=alias) + elif linked_field.fieldtype in frappe.model.table_fields: + return ChildTableField(linked_doctype, fieldname, doctype, alias=alias) + + def apply(self, query: QueryBuilder) -> QueryBuilder: + raise NotImplementedError + + +class ChildTableField(DynamicTableField): + def apply(self, query: QueryBuilder) -> QueryBuilder: + table = frappe.qb.DocType(self.doctype) + main_table = frappe.qb.DocType(self.parent_doctype) + if not query.is_joined(table): + query = query.left_join(table).on( + (table.parent == main_table.name) & (table.parenttype == self.parent_doctype) + ) + return query.select(getattr(table, self.fieldname).as_(self.alias or None)) + + +class LinkTableField(DynamicTableField): + def __init__( + self, + doctype: str, + fieldname: str, + parent_doctype: str, + link_fieldname: str, + alias: str | None = None, + ) -> None: + super().__init__(doctype, fieldname, parent_doctype, alias=alias) + self.link_fieldname = link_fieldname + + def apply(self, query: QueryBuilder) -> QueryBuilder: + table = frappe.qb.DocType(self.doctype) + main_table = frappe.qb.DocType(self.parent_doctype) + if not query.is_joined(table): + query = query.left_join(table).on(table.name == getattr(main_table, self.link_fieldname)) + return query.select(getattr(table, self.fieldname).as_(self.alias or None)) diff --git a/frappe/desk/doctype/number_card/number_card.py b/frappe/desk/doctype/number_card/number_card.py index 8e808ff635..30ec99644a 100644 --- a/frappe/desk/doctype/number_card/number_card.py +++ b/frappe/desk/doctype/number_card/number_card.py @@ -200,7 +200,7 @@ def get_cards_for_user(doctype, txt, searchfield, start, page_len, filters): if txt: search_conditions = [numberCard[field].like(f"%{txt}%") for field in searchfields] - condition_query = frappe.qb.engine.build_conditions(doctype, filters) + condition_query = frappe.qb.get_query(doctype, filters) return ( condition_query.select(numberCard.name, numberCard.label, numberCard.document_type) diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index ea6eb6259c..8b514444df 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -36,7 +36,7 @@ def get_group_by_count(doctype: str, current_filters: str, field: str) -> list[d ToDo = DocType("ToDo") User = DocType("User") count = Count("*").as_("count") - filtered_records = frappe.qb.engine.build_conditions(doctype, current_filters).select("name") + filtered_records = frappe.qb.get_query(doctype, filters=current_filters).select("name") return ( frappe.qb.from_(ToDo) diff --git a/frappe/query_builder/__init__.py b/frappe/query_builder/__init__.py index eb1d9df08f..b1f242f78c 100644 --- a/frappe/query_builder/__init__.py +++ b/frappe/query_builder/__init__.py @@ -7,7 +7,7 @@ from frappe.query_builder.terms import ParameterizedFunction, ParameterizedValue from frappe.query_builder.utils import ( Column, DocType, - get_qb_engine, + get_query, get_query_builder, patch_query_aggregation, patch_query_execute, diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index 24e2ee0e5f..b1e4e7eff1 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -103,7 +103,7 @@ class Cast_(Function): def _aggregate(function, dt, fieldname, filters, **kwargs): return ( - frappe.qb.engine.build_conditions(dt, filters) + frappe.qb.get_query(dt, filters=filters) .select(function(PseudoColumn(fieldname))) .run(**kwargs)[0][0] or 0 diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index be0403a291..f80dd4fc33 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -3,6 +3,7 @@ from importlib import import_module from typing import Any, Callable, get_type_hints from pypika import Query +from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder from pypika.queries import Column from pypika.terms import PseudoColumn @@ -55,10 +56,10 @@ def get_query_builder(type_of_db: str) -> Postgres | MariaDB: return picks[db] -def get_qb_engine(): +def get_query(*args, **kwargs) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: from frappe.database.query import Engine - return Engine() + return Engine().get_query(*args, **kwargs) def get_attr(method_string): diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 162d3e9d8a..1a3e8735dc 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -229,9 +229,7 @@ class TestReportview(FrappeTestCase): ) def test_none_filter(self): - query = frappe.qb.engine.get_query( - "DocType", fields="name", filters={"restrict_to_domain": None} - ) + query = frappe.qb.get_query("DocType", fields="name", filters={"restrict_to_domain": None}) sql = str(query).replace("`", "").replace('"', "") condition = "restrict_to_domain IS NULL" self.assertIn(condition, sql) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 3f48882345..957d76d022 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -56,30 +56,28 @@ class TestQuery(FrappeTestCase): @run_only_if(db_type_is.MARIADB) def test_multiple_tables_in_filters(self): self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "DocType", ["*"], [ - ["BOM Update Log", "name", "like", "f%"], + ["DocField", "name", "like", "f%"], ["DocType", "parent", "=", "something"], ], ).get_sql(), - "SELECT * FROM `tabDocType` LEFT JOIN `tabBOM Update Log` ON `tabBOM Update Log`.`parent`=`tabDocType`.`name` AND `tabBOM Update Log`.`parenttype`='DocType' WHERE `tabBOM Update Log`.`name` LIKE 'f%' AND `tabDocType`.`parent`='something'", + "SELECT `tabDocType`.* FROM `tabDocType` LEFT JOIN `tabDocField` ON `tabDocField`.`parent`=`tabDocType`.`name` AND `tabDocField`.`parenttype`='DocType' WHERE `tabDocField`.`name` LIKE 'f%' AND `tabDocType`.`parent`='something'", ) @run_only_if(db_type_is.MARIADB) def test_string_fields(self): self.assertEqual( - frappe.qb.engine.get_query( - "User", fields="name, email", filters={"name": "Administrator"} - ).get_sql(), + frappe.qb.get_query("User", fields="name, email", filters={"name": "Administrator"}).get_sql(), frappe.qb.from_("User") .select(Field("name"), Field("email")) .where(Field("name") == "Administrator") .get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields=["`name`, `email`"], filters={"name": "Administrator"} ).get_sql(), frappe.qb.from_("User") @@ -89,7 +87,7 @@ class TestQuery(FrappeTestCase): ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields=["`tabUser`.`name`", "`tabUser`.`email`"], filters={"name": "Administrator"} ).run(), frappe.qb.from_("User") @@ -99,7 +97,7 @@ class TestQuery(FrappeTestCase): ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields=["`tabUser`.`name` as owner", "`tabUser`.`email`"], filters={"name": "Administrator"}, @@ -111,7 +109,7 @@ class TestQuery(FrappeTestCase): ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields=["`tabUser`.`name`, Count(`name`) as count"], filters={"name": "Administrator"} ).run(), frappe.qb.from_("User") @@ -121,7 +119,7 @@ class TestQuery(FrappeTestCase): ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields=["`tabUser`.`name`, Count(`name`) as `count`"], filters={"name": "Administrator"}, @@ -133,7 +131,7 @@ class TestQuery(FrappeTestCase): ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields="`tabUser`.`name`, Count(`name`) as `count`", filters={"name": "Administrator"} ).run(), frappe.qb.from_("User") @@ -144,38 +142,34 @@ class TestQuery(FrappeTestCase): def test_functions_fields(self): self.assertEqual( - frappe.qb.engine.get_query("User", fields="Count(name)", filters={}).get_sql(), + frappe.qb.get_query("User", fields="Count(name)", filters={}).get_sql(), frappe.qb.from_("User").select(Count(Field("name"))).get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query("User", fields=["Count(name)", "Max(name)"], filters={}).get_sql(), + frappe.qb.get_query("User", fields=["Count(name)", "Max(name)"], filters={}).get_sql(), frappe.qb.from_("User").select(Count(Field("name")), Max(Field("name"))).get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( - "User", fields=["abs(name-email)", "Count(name)"], filters={} - ).get_sql(), + frappe.qb.get_query("User", fields=["abs(name-email)", "Count(name)"], filters={}).get_sql(), frappe.qb.from_("User") .select(Abs(Field("name") - Field("email")), Count(Field("name"))) .get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query("User", fields=[Count("*")], filters={}).get_sql(), + frappe.qb.get_query("User", fields=[Count("*")], filters={}).get_sql(), frappe.qb.from_("User").select(Count("*")).get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( - "User", fields="timestamp(creation, modified)", filters={} - ).get_sql(), + frappe.qb.get_query("User", fields="timestamp(creation, modified)", filters={}).get_sql(), frappe.qb.from_("User").select(Timestamp(Field("creation"), Field("modified"))).get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", fields="Count(name) as count, Max(email) as max_email", filters={} ).get_sql(), frappe.qb.from_("User") @@ -186,85 +180,83 @@ class TestQuery(FrappeTestCase): def test_qb_fields(self): user_doctype = frappe.qb.DocType("User") self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( user_doctype, fields=[user_doctype.name, user_doctype.email], filters={} ).get_sql(), frappe.qb.from_(user_doctype).select(user_doctype.name, user_doctype.email).get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query(user_doctype, fields=user_doctype.email, filters={}).get_sql(), + frappe.qb.get_query(user_doctype, fields=user_doctype.email, filters={}).get_sql(), frappe.qb.from_(user_doctype).select(user_doctype.email).get_sql(), ) def test_aliasing(self): user_doctype = frappe.qb.DocType("User") self.assertEqual( - frappe.qb.engine.get_query( - user_doctype, fields=["name as owner", "email as id"], filters={} - ).get_sql(), + frappe.qb.get_query("User", fields=["name as owner", "email as id"], filters={}).get_sql(), frappe.qb.from_(user_doctype) .select(user_doctype.name.as_("owner"), user_doctype.email.as_("id")) .get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( - user_doctype, fields="name as owner, email as id", filters={} - ).get_sql(), + frappe.qb.get_query(user_doctype, fields="name as owner, email as id", filters={}).get_sql(), frappe.qb.from_(user_doctype) .select(user_doctype.name.as_("owner"), user_doctype.email.as_("id")) .get_sql(), ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( user_doctype, fields=["Count(name) as count", "email as id"], filters={} ).get_sql(), frappe.qb.from_(user_doctype) - .select(user_doctype.email.as_("id"), Count(Field("name")).as_("count")) + .select(Count(Field("name")).as_("count"), user_doctype.email.as_("id")) .get_sql(), ) @run_only_if(db_type_is.MARIADB) def test_filters(self): self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "User", filters={"IfNull(name, " ")": ("<", Now())}, fields=["Max(name)"] ).run(), frappe.qb.from_("User").select(Max(Field("name"))).where(Ifnull("name", "") < Now()).run(), ) def test_implicit_join_query(self): + self.maxDiff = None + self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "Note", filters={"name": "Test Note Title"}, fields=["name", "`tabNote Seen By`.`user` as seen_by"], ).get_sql(), - "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` seen_by FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( + "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` `seen_by` FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( "`", '"' if frappe.db.db_type == "postgres" else "`" ), ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "Note", filters={"name": "Test Note Title"}, fields=["name", "`tabNote Seen By`.`user` as seen_by", "`tabNote Seen By`.`idx` as idx"], ).get_sql(), - "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` seen_by,`tabNote Seen By`.`idx` idx FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( + "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` `seen_by`,`tabNote Seen By`.`idx` `idx` FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( "`", '"' if frappe.db.db_type == "postgres" else "`" ), ) self.assertEqual( - frappe.qb.engine.get_query( + frappe.qb.get_query( "Note", filters={"name": "Test Note Title"}, fields=["name", "seen_by.user as seen_by", "`tabNote Seen By`.`idx` as idx"], ).get_sql(), - "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` seen_by,`tabNote Seen By`.`idx` idx FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( + "SELECT `tabNote`.`name`,`tabNote Seen By`.`user` `seen_by`,`tabNote Seen By`.`idx` `idx` FROM `tabNote` LEFT JOIN `tabNote Seen By` ON `tabNote Seen By`.`parent`=`tabNote`.`name` AND `tabNote Seen By`.`parenttype`='Note' WHERE `tabNote`.`name`='Test Note Title'".replace( "`", '"' if frappe.db.db_type == "postgres" else "`" ), ) @@ -272,40 +264,40 @@ class TestQuery(FrappeTestCase): @run_only_if(db_type_is.MARIADB) def test_comment_stripping(self): self.assertNotIn( - "email", frappe.qb.engine.get_query("User", fields=["name", "#email"], filters={}).get_sql() + "email", frappe.qb.get_query("User", fields=["name", "#email"], filters={}).get_sql() ) def test_nestedset(self): frappe.db.sql("delete from `tabDocType` where `name` = 'Test Tree DocType'") frappe.db.sql_ddl("drop table if exists `tabTest Tree DocType`") create_tree_docs() - descendants_result = frappe.qb.engine.get_query( + descendants_result = frappe.qb.get_query( "Test Tree DocType", fields=["name"], filters={"name": ("descendants of", "Parent 1")}, - orderby="modified", + order_by="modified", ).run(as_list=1) # Format decendants result descendants_result = list(itertools.chain.from_iterable(descendants_result)) self.assertListEqual(descendants_result, get_descendants_of("Test Tree DocType", "Parent 1")) - ancestors_result = frappe.qb.engine.get_query( + ancestors_result = frappe.qb.get_query( "Test Tree DocType", fields=["name"], filters={"name": ("ancestors of", "Child 2")}, - orderby="modified", + order_by="modified", ).run(as_list=1) # Format ancestors result ancestors_result = list(itertools.chain.from_iterable(ancestors_result)) self.assertListEqual(ancestors_result, get_ancestors_of("Test Tree DocType", "Child 2")) - not_descendants_result = frappe.qb.engine.get_query( + not_descendants_result = frappe.qb.get_query( "Test Tree DocType", fields=["name"], filters={"name": ("not descendants of", "Parent 1")}, - orderby="modified", + order_by="modified", ).run(as_dict=1) self.assertListEqual( @@ -317,11 +309,11 @@ class TestQuery(FrappeTestCase): ), ) - not_ancestors_result = frappe.qb.engine.get_query( + not_ancestors_result = frappe.qb.get_query( "Test Tree DocType", fields=["name"], filters={"name": ("not ancestors of", "Child 2")}, - orderby="modified", + order_by="modified", ).run(as_dict=1) self.assertListEqual( diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index 13c4633031..0dcadc5ec6 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -24,7 +24,7 @@ def get_monthly_results( date_format = "%m-%Y" if frappe.db.db_type != "postgres" else "MM-YYYY" return dict( - frappe.qb.engine.build_conditions(table=goal_doctype, filters=filters) + frappe.qb.get_query(table=goal_doctype, filters=filters) .select( DateFormat(Table[date_col], date_format).as_("month_year"), Function(aggregation, goal_field), From f79185d79ceeb3dbe74098996f5af01c00833dc2 Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Thu, 3 Nov 2022 15:08:26 +0000 Subject: [PATCH 02/58] feat: use Connected App for OAuth based Email Account --- .../doctype/email_account/email_account.js | 71 ++++++++++------- .../doctype/email_account/email_account.json | 38 ++++----- .../doctype/email_account/email_account.py | 48 ++++++------ frappe/email/oauth.py | 77 +------------------ .../doctype/connected_app/connected_app.py | 29 ++++++- .../doctype/token_cache/token_cache.py | 9 ++- 6 files changed, 126 insertions(+), 146 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 98160e5f46..92b7deece3 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -68,15 +68,19 @@ frappe.email_defaults_pop = { function oauth_access(frm) { return frappe.call({ - method: "frappe.email.oauth.oauth_access", + method: "frappe.client.get", args: { - email_account: frm.doc.name, - service: frm.doc.service || "", + doctype: "Connected App", + name: frm.doc.connected_app, }, - callback: function (r) { - if (!r.exc) { - window.open(r.message.url, "_self"); - } + callback: app => { + return frappe.call({ + method: "initiate_web_application_flow", + doc: app.message, + callback: function (r) { + window.open(r.message, "_self"); + }, + }); }, }); } @@ -105,7 +109,6 @@ frappe.ui.form.on("Email Account", { }); } frm.events.show_gmail_message_for_less_secure_apps(frm); - frm.events.toggle_auth_method(frm); }, use_imap: function (frm) { @@ -153,7 +156,6 @@ frappe.ui.form.on("Email Account", { frm.add_child("imap_folder", { folder_name: "INBOX" }); frm.refresh_field("imap_folder"); } - frm.toggle_display(["auth_method"], frm.doc.service === "GMail"); set_default_max_attachment_size(frm, "attachment_limit"); }, @@ -167,21 +169,25 @@ frappe.ui.form.on("Email Account", { delete frappe.route_flags.delete_user_from_locals; delete locals["User"][frappe.route_flags.linked_user]; } + + if (frm.doc.connected_app && !frm.doc.connected_user) { + frm.set_value("connected_user", frappe.session.user); + } }, after_save(frm) { - if (frm.doc.auth_method === "OAuth" && !frm.doc.refresh_token) { - oauth_access(frm); - } - }, - - toggle_auth_method: function (frm) { - if (frm.doc.service !== "GMail") { - frm.toggle_display(["auth_method"], false); - frm.doc.auth_method = "Basic"; - } else { - frm.toggle_display(["auth_method"], true); - } + frappe.call({ + method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + args: { + connected_app: frm.doc.connected_app, + connected_user: frm.doc.connected_user, + }, + callback: r => { + if (!r.message) { + oauth_access(frm); + } + }, + }); }, show_gmail_message_for_less_secure_apps: function (frm) { @@ -197,13 +203,22 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "OAuth" && !frm.doc.refresh_token) { - let msg = __( - 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' - ); - frm.dashboard.clear_headline(); - frm.dashboard.set_headline_alert(msg, "yellow"); - } + frappe.call({ + method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + args: { + connected_app: frm.doc.connected_app, + connected_user: frm.doc.connected_user, + }, + callback: r => { + if (frm.doc.auth_method === "OAuth" && !r.message) { + let msg = __( + 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' + ); + frm.dashboard.clear_headline(); + frm.dashboard.set_headline_alert(msg, "yellow"); + } + }, + }); }, authorize_api_access: function (frm) { diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index da88ac680c..38345ea618 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -20,8 +20,8 @@ "awaiting_password", "ascii_encode_password", "column_break_10", - "refresh_token", - "access_token", + "connected_app", + "connected_user", "login_id_is_different", "login_id", "mailbox_settings", @@ -577,25 +577,11 @@ "label": "IMAP Details" }, { - "depends_on": "eval: doc.service === \"GMail\" && doc.auth_method === \"OAuth\" && !doc.__islocal && !doc.__unsaved", + "depends_on": "eval: doc.auth_method === \"OAuth\" && !doc.__islocal && !doc.__unsaved", "fieldname": "authorize_api_access", "fieldtype": "Button", "label": "Authorize API Access" }, - { - "fieldname": "refresh_token", - "fieldtype": "Small Text", - "hidden": 1, - "label": "Refresh Token", - "read_only": 1 - }, - { - "fieldname": "access_token", - "fieldtype": "Small Text", - "hidden": 1, - "label": "Access Token", - "read_only": 1 - }, { "default": "Basic", "fieldname": "auth_method", @@ -610,12 +596,28 @@ "fieldname": "use_starttls", "fieldtype": "Check", "label": "Use STARTTLS" + }, + { + "depends_on": "eval: doc.auth_method === \"OAuth\"", + "fieldname": "connected_app", + "fieldtype": "Link", + "label": "Connected App", + "mandatory_depends_on": "eval: doc.auth_method === \"OAuth\"", + "options": "Connected App" + }, + { + "depends_on": "eval: doc.auth_method === \"OAuth\"", + "fieldname": "connected_user", + "fieldtype": "Link", + "label": "Connected User", + "mandatory_depends_on": "eval: doc.auth_method === \"OAuth\"", + "options": "User" } ], "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-08-23 00:31:05.305462", + "modified": "2022-11-03 20:26:52.876668", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 1db45604e1..30fad87565 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -11,6 +11,7 @@ from poplib import error_proto import frappe from frappe import _, are_emails_muted, safe_encode +from frappe.integrations.doctype.connected_app.connected_app import check_active_token from frappe.desk.form import assign_to from frappe.email.doctype.email_domain.email_domain import EMAIL_DOMAIN_FIELDS from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError @@ -85,21 +86,13 @@ class EmailAccount(Document): use_oauth = self.auth_method == "OAuth" self.use_starttls = cint(self.use_imap and self.use_starttls and not self.use_ssl) - if getattr(self, "service", "") != "GMail" and use_oauth: - self.auth_method = "Basic" - use_oauth = False - if use_oauth: # no need for awaiting password for oauth self.awaiting_password = 0 self.password = None - elif self.refresh_token: - # clear access & refresh token - self.refresh_token = self.access_token = None - if not frappe.local.flags.in_install and not self.awaiting_password: - if self.refresh_token or self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if self.auth_method == "OAuth" or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -188,6 +181,7 @@ class EmailAccount(Document): if frappe.cache().get_value("workers:no-internet") == True: return None + oauth_token = self.get_oauth_token() args = frappe._dict( { "email_account_name": self.email_account_name, @@ -202,8 +196,8 @@ class EmailAccount(Document): "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, "use_oauth": self.auth_method == "OAuth", - "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, - "access_token": decrypt(self.access_token) if self.access_token else None, + "refresh_token": oauth_token.get_password("refresh_token") if oauth_token else None, + "access_token": oauth_token.get_password("refresh_token") if oauth_token else None, } ) @@ -392,8 +386,6 @@ class EmailAccount(Document): }, "name": {"conf_names": ("email_sender_name",), "default": "Frappe"}, "auth_method": {"conf_names": ("auth_method"), "default": "Basic"}, - "access_token": {"conf_names": ("mail_access_token")}, - "refresh_token": {"conf_names": ("mail_refresh_token")}, "from_site_config": {"default": True}, } @@ -401,15 +393,13 @@ class EmailAccount(Document): for doc_field_name, d in field_to_conf_name_map.items(): conf_names, default = d.get("conf_names") or [], d.get("default") value = [frappe.conf.get(k) for k in conf_names if frappe.conf.get(k)] - - if doc_field_name in ("refresh_token", "access_token"): - account_details[doc_field_name] = value and encrypt(value[0]) - else: - account_details[doc_field_name] = (value and value[0]) or default + account_details[doc_field_name] = (value and value[0]) or default return account_details def sendmail_config(self): + oauth_token = self.get_oauth_token() + return { "email_account": self.name, "server": self.smtp_server, @@ -420,8 +410,8 @@ class EmailAccount(Document): "use_tls": cint(self.use_tls), "service": getattr(self, "service", ""), "use_oauth": self.auth_method == "OAuth", - "refresh_token": decrypt(self.refresh_token) if self.refresh_token else None, - "access_token": decrypt(self.access_token) if self.access_token else None, + "refresh_token": oauth_token.get_password("refresh_token") if oauth_token else None, + "access_token": oauth_token.get_password("refresh_token") if oauth_token else None, } def get_smtp_server(self): @@ -681,6 +671,13 @@ class EmailAccount(Document): except Exception: self.log_error("Unable to add to Sent folder") + def get_oauth_token(self): + token = None + if self.auth_method == "OAuth": + connected_app = frappe.get_doc("Connected App", self.connected_app) + token = connected_app.get_active_token(self.connected_user) + + return token @frappe.whitelist() def get_append_to( @@ -786,15 +783,20 @@ def pull(now=False): doctype = frappe.qb.DocType("Email Account") email_accounts = ( frappe.qb.from_(doctype) - .select(doctype.name) + .select(doctype.name, doctype.auth_method) .where(doctype.enable_incoming == 1) .where( - (doctype.awaiting_password == 0) - | ((doctype.auth_method == "OAuth") & (doctype.refresh_token.isnotnull())) + (doctype.awaiting_password == 0) | (doctype.auth_method == "OAuth") ) .run(as_dict=1) ) for email_account in email_accounts: + if email_account.auth_method == "OAuth" and not check_active_token( + connected_app=doctype.connected_app, + connected_user=doctype.connected_user, + ): + continue + if now: pull_from_email_account(email_account.name) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index f5b60a9f3d..af86bc47d2 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -53,7 +53,7 @@ class Oauth: return f"user={self.email}\1auth=Bearer {self._access_token}\1\1" def connect(self, _retry: int = 0) -> None: - """Connection method with retry on exception for Oauth""" + """Connection method with retry on exception for connection errors""" try: if isinstance(self._conn, POP3): res = self._connect_pop() @@ -69,18 +69,14 @@ class Oauth: self._connect_smtp() except Exception as e: - # maybe the access token expired - refreshing - access_token = self._refresh_access_token() - - if not access_token or _retry > 0: + if _retry > 0: frappe.log_error( - "OAuth Error - Authentication Failed", str(e), "Email Account", self.email_account + "SMTP Connection Error - Authentication Failed", str(e), "Email Account", self.email_account ) # raising a bare exception here as we have a lot of exception handling present # where the connect method is called from - hence just logging and raising. raise - self._access_token = access_token self.connect(_retry + 1) def _connect_pop(self) -> bytes: @@ -98,70 +94,3 @@ class Oauth: def _connect_smtp(self) -> None: self._conn.auth(self._mechanism, lambda x: self._auth_string, initial_response_ok=False) - - def _refresh_access_token(self) -> str: - """Refreshes access token via calling `refresh_access_token` method of oauth service object""" - service_obj = self._get_service_object() - access_token = service_obj.refresh_access_token(self._refresh_token).get("access_token") - - if access_token: - # set the new access token in db - frappe.db.set_value( - "Email Account", - self.email_account, - "access_token", - encrypt(access_token), - update_modified=False, - ) - - return access_token - - def _get_service_object(self): - """Get Oauth service object""" - - return { - "GMail": GoogleOAuth("mail", validate=False), - }[self.service] - - -@frappe.whitelist(methods=["POST"]) -def oauth_access(email_account: str, service: str): - """Used as a default endpoint/caller for all oauth services. - Returns authorization url for redirection""" - - if not service: - frappe.throw(frappe._("No Service is selected. Please select one and try again!")) - - if service == "GMail": - return authorize_google_access(email_account) - - raise NotImplementedError(f"Service {service} currently doesn't have oauth implementation.") - - -def authorize_google_access(email_account: str, code: str = None): - """Facilitates google oauth for email. - This is invoked 2 times - first time when user clicks `Authorize API Access` for getting the authorization url - and second time for setting the refresh and access token in db when google redirects back with oauth code.""" - - doctype = "Email Account" - oauth_obj = GoogleOAuth("mail") - - if not code: - return oauth_obj.get_authentication_url( - { - "redirect": f"/app/Form/{quote(doctype)}/{quote(email_account)}", - "success_query_param": "successful_authorization=1", - "email_account": email_account, - }, - ) - - res = oauth_obj.authorize(code) - frappe.db.set_value( - doctype, - email_account, - { - "refresh_token": encrypt(res.get("refresh_token")), - "access_token": encrypt(res.get("access_token")), - }, - update_modified=False, - ) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index 308d1ca84a..46894132e8 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -57,7 +57,7 @@ class ConnectedApp(Document): def initiate_web_application_flow(self, user=None, success_uri=None): """Return an authorization URL for the user. Save state in Token Cache.""" user = user or frappe.session.user - oauth = self.get_oauth2_session(init=True) + oauth = self.get_oauth2_session(user, init=True) query_params = self.get_query_params() authorization_url, state = oauth.authorization_url(self.authorization_uri, **query_params) token_cache = self.get_token_cache(user) @@ -102,6 +102,21 @@ class ConnectedApp(Document): def get_query_params(self): return {param.key: param.value for param in self.query_parameters} + def get_active_token(self, user=None): + user = user or frappe.session.user + token_cache = self.get_token_cache(user) + if token_cache and not token_cache.is_expired(): + return token_cache + elif token_cache and token_cache.get_expires_in(): + oauth_session = self.get_oauth2_session(user) + token = oauth_session.refresh_token( + body=f"redirect_uri={self.redirect_uri}", + token_url=self.token_uri, + refresh_token=token_cache.get_password("refresh_token"), + ) + token_cache.update_data(token) + return token_cache + @frappe.whitelist(allow_guest=True) def callback(code=None, state=None): @@ -142,3 +157,15 @@ def callback(code=None, state=None): frappe.local.response["type"] = "redirect" frappe.local.response["location"] = token_cache.get("success_uri") or connected_app.get_url() + + +@frappe.whitelist() +def check_active_token(connected_app, connected_user=None): + is_token_active = False + app = frappe.get_doc("Connected App", connected_app) + token = app.get_active_token(connected_user) + + if token: + is_token_active = True + + return is_token_active diff --git a/frappe/integrations/doctype/token_cache/token_cache.py b/frappe/integrations/doctype/token_cache/token_cache.py index 25f07a16ba..b01974e5ab 100644 --- a/frappe/integrations/doctype/token_cache/token_cache.py +++ b/frappe/integrations/doctype/token_cache/token_cache.py @@ -3,6 +3,8 @@ from datetime import datetime, timedelta +import pytz + import frappe from frappe import _ from frappe.model.document import Document @@ -50,8 +52,11 @@ class TokenCache(Document): return self def get_expires_in(self): - expiry_time = frappe.utils.get_datetime(self.modified) + timedelta(self.expires_in) - return (datetime.now() - expiry_time).total_seconds() + now_utc = datetime.utcnow().replace(tzinfo=pytz.utc) + expiry_time = frappe.utils.get_datetime(self.modified) + timedelta(seconds=self.expires_in) + expiry_local = expiry_time.replace(tzinfo=pytz.timezone(frappe.utils.get_time_zone())) + expiry_utc = expiry_local.astimezone(pytz.utc) + return (expiry_utc - now_utc).total_seconds() def is_expired(self): return self.get_expires_in() < 0 From a4e5df674b2c938b187cd2f07172bfc71ccb6e2a Mon Sep 17 00:00:00 2001 From: Revant Nandgaonkar Date: Sun, 6 Nov 2022 06:11:24 +0000 Subject: [PATCH 03/58] fix(email): oauth_access call and info message --- .../doctype/email_account/email_account.js | 86 +++++++++---------- 1 file changed, 42 insertions(+), 44 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 92b7deece3..ba6ebd4de7 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -67,22 +67,16 @@ frappe.email_defaults_pop = { }; function oauth_access(frm) { - return frappe.call({ - method: "frappe.client.get", - args: { - doctype: "Connected App", - name: frm.doc.connected_app, - }, - callback: app => { - return frappe.call({ - method: "initiate_web_application_flow", - doc: app.message, - callback: function (r) { - window.open(r.message, "_self"); - }, - }); - }, - }); + frappe.model.with_doc("Connected App", frm.doc.connected_app, () => { + const connected_app = frappe.get_doc("Connected App", frm.doc.connected_app); + return frappe.call({ + doc: connected_app, + method: "initiate_web_application_flow", + callback: function(r) { + window.open(r.message, "_self"); + } + }); + }) } function set_default_max_attachment_size(frm, field) { @@ -176,18 +170,20 @@ frappe.ui.form.on("Email Account", { }, after_save(frm) { - frappe.call({ - method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", - args: { - connected_app: frm.doc.connected_app, - connected_user: frm.doc.connected_user, - }, - callback: r => { - if (!r.message) { - oauth_access(frm); - } - }, - }); + if (frm.doc.auth_method === "OAuth") { + frappe.call({ + method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + args: { + connected_app: frm.doc.connected_app, + connected_user: frm.doc.connected_user, + }, + callback: r => { + if (!r.message) { + oauth_access(frm); + } + }, + }); + } }, show_gmail_message_for_less_secure_apps: function (frm) { @@ -203,22 +199,24 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - frappe.call({ - method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", - args: { - connected_app: frm.doc.connected_app, - connected_user: frm.doc.connected_user, - }, - callback: r => { - if (frm.doc.auth_method === "OAuth" && !r.message) { - let msg = __( - 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' - ); - frm.dashboard.clear_headline(); - frm.dashboard.set_headline_alert(msg, "yellow"); - } - }, - }); + if (frm.doc.auth_method === "OAuth") { + frappe.call({ + method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + args: { + connected_app: frm.doc.connected_app, + connected_user: frm.doc.connected_user, + }, + callback: r => { + if (!r.message) { + let msg = __( + 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' + ); + frm.dashboard.clear_headline(); + frm.dashboard.set_headline_alert(msg, "yellow"); + } + }, + }); + } }, authorize_api_access: function (frm) { From f0a17d7adb68a57c16322eb18f47f44469d63054 Mon Sep 17 00:00:00 2001 From: phot0n Date: Wed, 28 Dec 2022 14:48:16 +0530 Subject: [PATCH 04/58] fix: make connected app work with email account * removed (now) unnecessary things from oauth class * simplified get_active_token in connected_app * removed gmail banner from email account docs --- .../doctype/email_account/email_account.js | 62 +++---------------- .../doctype/email_account/email_account.json | 3 +- .../doctype/email_account/email_account.py | 26 +++----- frappe/email/oauth.py | 41 ++++-------- frappe/email/receive.py | 4 -- frappe/email/smtp.py | 8 +-- .../doctype/connected_app/connected_app.py | 9 ++- 7 files changed, 33 insertions(+), 120 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index ba6ebd4de7..67e2ffc863 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -72,20 +72,20 @@ function oauth_access(frm) { return frappe.call({ doc: connected_app, method: "initiate_web_application_flow", - callback: function(r) { + callback: function (r) { window.open(r.message, "_self"); - } + }, }); - }) + }); } -function set_default_max_attachment_size(frm, field) { - if (frm.doc.__islocal && !frm.doc[field]) { +function set_default_max_attachment_size(frm) { + if (frm.doc.__islocal && !frm.doc["attachment_limit"]) { frappe.call({ method: "frappe.core.api.file.get_max_file_size", callback: function (r) { if (!r.exc) { - frm.set_value(field, Number(r.message) / (1024 * 1024)); + frm.set_value("attachment_limit", Number(r.message) / (1024 * 1024)); } }, }); @@ -102,7 +102,6 @@ frappe.ui.form.on("Email Account", { frm.set_value(key, value); }); } - frm.events.show_gmail_message_for_less_secure_apps(frm); }, use_imap: function (frm) { @@ -130,12 +129,6 @@ frappe.ui.form.on("Email Account", { }, onload: function (frm) { - if (frappe.utils.get_query_params().successful_authorization === "1") { - frappe.show_alert(__("Successfully Authorized")); - // FIXME: find better alternative - window.history.replaceState(null, "", window.location.pathname); - } - frm.set_df_property("append_to", "only_select", true); frm.set_query( "append_to", @@ -150,23 +143,17 @@ frappe.ui.form.on("Email Account", { frm.add_child("imap_folder", { folder_name: "INBOX" }); frm.refresh_field("imap_folder"); } - set_default_max_attachment_size(frm, "attachment_limit"); + set_default_max_attachment_size(frm); }, refresh: function (frm) { frm.events.enable_incoming(frm); frm.events.notify_if_unreplied(frm); - frm.events.show_gmail_message_for_less_secure_apps(frm); - frm.events.show_oauth_authorization_message(frm); if (frappe.route_flags.delete_user_from_locals && frappe.route_flags.linked_user) { delete frappe.route_flags.delete_user_from_locals; delete locals["User"][frappe.route_flags.linked_user]; } - - if (frm.doc.connected_app && !frm.doc.connected_user) { - frm.set_value("connected_user", frappe.session.user); - } }, after_save(frm) { @@ -177,7 +164,7 @@ frappe.ui.form.on("Email Account", { connected_app: frm.doc.connected_app, connected_user: frm.doc.connected_user, }, - callback: r => { + callback: (r) => { if (!r.message) { oauth_access(frm); } @@ -186,39 +173,6 @@ frappe.ui.form.on("Email Account", { } }, - show_gmail_message_for_less_secure_apps: function (frm) { - frm.dashboard.clear_headline(); - let msg = __( - "GMail will only work if you enable 2-step authentication and use app-specific password OR use OAuth." - ); - let cta = __("Read the step by step guide here."); - msg += ` ${cta}`; - if (frm.doc.service === "GMail") { - frm.dashboard.set_headline_alert(msg); - } - }, - - show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "OAuth") { - frappe.call({ - method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", - args: { - connected_app: frm.doc.connected_app, - connected_user: frm.doc.connected_user, - }, - callback: r => { - if (!r.message) { - let msg = __( - 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' - ); - frm.dashboard.clear_headline(); - frm.dashboard.set_headline_alert(msg, "yellow"); - } - }, - }); - } - }, - authorize_api_access: function (frm) { oauth_access(frm); }, diff --git a/frappe/email/doctype/email_account/email_account.json b/frappe/email/doctype/email_account/email_account.json index 38345ea618..f9e4f95ff0 100644 --- a/frappe/email/doctype/email_account/email_account.json +++ b/frappe/email/doctype/email_account/email_account.json @@ -203,7 +203,6 @@ "label": "Use SSL" }, { - "default": "1", "depends_on": "eval:!doc.domain && doc.enable_incoming", "description": "Ignore attachments over this size", "fetch_from": "domain.attachment_limit", @@ -617,7 +616,7 @@ "icon": "fa fa-inbox", "index_web_pages_for_search": 1, "links": [], - "modified": "2022-11-03 20:26:52.876668", + "modified": "2022-12-28 14:56:18.754804", "modified_by": "Administrator", "module": "Email", "name": "Email Account", diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 30fad87565..3ad2f67c0b 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -11,18 +11,17 @@ from poplib import error_proto import frappe from frappe import _, are_emails_muted, safe_encode -from frappe.integrations.doctype.connected_app.connected_app import check_active_token from frappe.desk.form import assign_to from frappe.email.doctype.email_domain.email_domain import EMAIL_DOMAIN_FIELDS from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError from frappe.email.smtp import SMTPServer from frappe.email.utils import get_port +from frappe.integrations.doctype.connected_app.connected_app import check_active_token from frappe.model.document import Document from frappe.utils import cint, comma_or, cstr, parse_addr, validate_email_address from frappe.utils.background_jobs import enqueue, get_jobs from frappe.utils.error import raise_error_on_no_output from frappe.utils.jinja import render_template -from frappe.utils.password import decrypt, encrypt from frappe.utils.user import get_system_managers @@ -92,7 +91,7 @@ class EmailAccount(Document): self.password = None if not frappe.local.flags.in_install and not self.awaiting_password: - if self.auth_method == "OAuth" or self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if use_oauth or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 @@ -190,14 +189,12 @@ class EmailAccount(Document): "use_ssl": self.use_ssl, "use_starttls": self.use_starttls, "username": getattr(self, "login_id", None) or self.email_id, - "service": getattr(self, "service", ""), "use_imap": self.use_imap, "email_sync_rule": email_sync_rule, "incoming_port": get_port(self), "initial_sync_count": self.initial_sync_count or 100, "use_oauth": self.auth_method == "OAuth", - "refresh_token": oauth_token.get_password("refresh_token") if oauth_token else None, - "access_token": oauth_token.get_password("refresh_token") if oauth_token else None, + "access_token": oauth_token.get_password("access_token") if oauth_token else None, } ) @@ -408,10 +405,8 @@ class EmailAccount(Document): "password": self._password, "use_ssl": cint(self.use_ssl_for_outgoing), "use_tls": cint(self.use_tls), - "service": getattr(self, "service", ""), "use_oauth": self.auth_method == "OAuth", - "refresh_token": oauth_token.get_password("refresh_token") if oauth_token else None, - "access_token": oauth_token.get_password("refresh_token") if oauth_token else None, + "access_token": oauth_token.get_password("access_token") if oauth_token else None, } def get_smtp_server(self): @@ -679,6 +674,7 @@ class EmailAccount(Document): return token + @frappe.whitelist() def get_append_to( doctype=None, txt=None, searchfield=None, start=None, page_len=None, filters=None @@ -783,20 +779,12 @@ def pull(now=False): doctype = frappe.qb.DocType("Email Account") email_accounts = ( frappe.qb.from_(doctype) - .select(doctype.name, doctype.auth_method) + .select(doctype.name) .where(doctype.enable_incoming == 1) - .where( - (doctype.awaiting_password == 0) | (doctype.auth_method == "OAuth") - ) + .where((doctype.awaiting_password == 0) | (doctype.auth_method == "OAuth")) .run(as_dict=1) ) for email_account in email_accounts: - if email_account.auth_method == "OAuth" and not check_active_token( - connected_app=doctype.connected_app, - connected_user=doctype.connected_user, - ): - continue - if now: pull_from_email_account(email_account.name) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index af86bc47d2..c69ab2211e 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -2,11 +2,8 @@ import base64 from imaplib import IMAP4 from poplib import POP3 from smtplib import SMTP -from urllib.parse import quote import frappe -from frappe.integrations.google_oauth import GoogleOAuth -from frappe.utils.password import encrypt class OAuthenticationError(Exception): @@ -20,30 +17,21 @@ class Oauth: email_account: str, email: str, access_token: str, - refresh_token: str, - service: str, mechanism: str = "XOAUTH2", ) -> None: self.email_account = email_account self.email = email - self.service = service self._mechanism = mechanism self._conn = conn self._access_token = access_token - self._refresh_token = refresh_token self._validate() def _validate(self) -> None: - if self.service != "GMail": - raise NotImplementedError( - f"Service {self.service} currently doesn't have oauth implementation." - ) - - if not self._refresh_token: + if not self._access_token: frappe.throw( - frappe._("Please Authorize OAuth."), + frappe._("Please Authorize OAuth for Email Account {}").format(self.email_account), OAuthenticationError, frappe._("OAuth Error"), ) @@ -52,14 +40,11 @@ class Oauth: def _auth_string(self) -> str: return f"user={self.email}\1auth=Bearer {self._access_token}\1\1" - def connect(self, _retry: int = 0) -> None: + def connect(self) -> None: """Connection method with retry on exception for connection errors""" try: if isinstance(self._conn, POP3): - res = self._connect_pop() - - if not res.startswith(b"+OK"): - raise + self._connect_pop() elif isinstance(self._conn, IMAP4): self._connect_imap() @@ -69,15 +54,12 @@ class Oauth: self._connect_smtp() except Exception as e: - if _retry > 0: - frappe.log_error( - "SMTP Connection Error - Authentication Failed", str(e), "Email Account", self.email_account - ) - # raising a bare exception here as we have a lot of exception handling present - # where the connect method is called from - hence just logging and raising. - raise - - self.connect(_retry + 1) + frappe.log_error( + "Email Connection Error - Authentication Failed", str(e), "Email Account", self.email_account + ) + # raising a bare exception here as we have a lot of exception handling present + # where the connect method is called from - hence just logging and raising. + raise def _connect_pop(self) -> bytes: # poplib doesn't have AUTH command implementation @@ -87,7 +69,8 @@ class Oauth: ) ) - return res + if not res.startswith(b"+OK"): + raise def _connect_imap(self) -> None: self._conn.authenticate(self._mechanism, lambda x: self._auth_string) diff --git a/frappe/email/receive.py b/frappe/email/receive.py index 7028dc1f11..c635bdd98a 100644 --- a/frappe/email/receive.py +++ b/frappe/email/receive.py @@ -109,8 +109,6 @@ class EmailServer: self.settings.email_account, self.settings.username, self.settings.access_token, - self.settings.refresh_token, - self.settings.service, ).connect() else: @@ -142,8 +140,6 @@ class EmailServer: self.settings.email_account, self.settings.username, self.settings.access_token, - self.settings.refresh_token, - self.settings.service, ).connect() else: diff --git a/frappe/email/smtp.py b/frappe/email/smtp.py index 10eb2f7681..028b21b0ae 100644 --- a/frappe/email/smtp.py +++ b/frappe/email/smtp.py @@ -54,9 +54,7 @@ class SMTPServer: use_tls=None, use_ssl=None, use_oauth=0, - refresh_token=None, access_token=None, - service=None, ): self.login = login self.email_account = email_account @@ -66,9 +64,7 @@ class SMTPServer: self.use_tls = use_tls self.use_ssl = use_ssl self.use_oauth = use_oauth - self.refresh_token = refresh_token self.access_token = access_token - self.service = service self._session = None if not self.server: @@ -112,9 +108,7 @@ class SMTPServer: self.secure_session(_session) if self.use_oauth: - Oauth( - _session, self.email_account, self.login, self.access_token, self.refresh_token, self.service - ).connect() + Oauth(_session, self.email_account, self.login, self.access_token).connect() elif self.password: res = _session.login(str(self.login or ""), str(self.password or "")) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index 46894132e8..e7da138fbc 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -105,9 +105,7 @@ class ConnectedApp(Document): def get_active_token(self, user=None): user = user or frappe.session.user token_cache = self.get_token_cache(user) - if token_cache and not token_cache.is_expired(): - return token_cache - elif token_cache and token_cache.get_expires_in(): + if token_cache and token_cache.is_expired(): oauth_session = self.get_oauth2_session(user) token = oauth_session.refresh_token( body=f"redirect_uri={self.redirect_uri}", @@ -115,7 +113,8 @@ class ConnectedApp(Document): refresh_token=token_cache.get_password("refresh_token"), ) token_cache.update_data(token) - return token_cache + + return token_cache @frappe.whitelist(allow_guest=True) @@ -151,7 +150,7 @@ def callback(code=None, state=None): code=code, client_secret=connected_app.get_password("client_secret"), include_client_id=True, - **query_params + **query_params, ) token_cache.update_data(token) From ca8842861a0bc34cda7103e2443283a8ae013766 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 29 Dec 2022 17:03:16 +0530 Subject: [PATCH 05/58] refactor(minor): simplify check_active_token --- .../integrations/doctype/connected_app/connected_app.py | 8 +------- frappe/translations/ru.csv | 1 + frappe/website/doctype/blog_post/templates/blog_post.html | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index e7da138fbc..cd219263e9 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -160,11 +160,5 @@ def callback(code=None, state=None): @frappe.whitelist() def check_active_token(connected_app, connected_user=None): - is_token_active = False app = frappe.get_doc("Connected App", connected_app) - token = app.get_active_token(connected_user) - - if token: - is_token_active = True - - return is_token_active + return bool(app.get_active_token(connected_user)) diff --git a/frappe/translations/ru.csv b/frappe/translations/ru.csv index f83bf411ff..1c99690ff3 100644 --- a/frappe/translations/ru.csv +++ b/frappe/translations/ru.csv @@ -6,6 +6,7 @@ Account,Аккаунт, Accounts Manager,Диспетчер учетных записей, Accounts User,Пользователь Учетных записей, Action,Действие, +min read, Действие, Actions,Действия, Active,Активен, Add,Добавить, diff --git a/frappe/website/doctype/blog_post/templates/blog_post.html b/frappe/website/doctype/blog_post/templates/blog_post.html index 67236fb26d..708b896357 100644 --- a/frappe/website/doctype/blog_post/templates/blog_post.html +++ b/frappe/website/doctype/blog_post/templates/blog_post.html @@ -22,7 +22,7 @@ {%- if read_time -%}  · - {{ read_time }} min read + {{ read_time }} {{ _("min read") }} {%- endif -%} From b78f86a30790d95cc1b90aa3743395db220c9df2 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 29 Dec 2022 17:14:28 +0530 Subject: [PATCH 06/58] fix: redirect back to email account after successful oauth --- frappe/email/doctype/email_account/email_account.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 67e2ffc863..31da072692 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -72,6 +72,9 @@ function oauth_access(frm) { return frappe.call({ doc: connected_app, method: "initiate_web_application_flow", + args: { + success_uri: window.location.pathname, + }, callback: function (r) { window.open(r.message, "_self"); }, From 231d90cb40eda39543a55ea6a5fd588bdc5fa514 Mon Sep 17 00:00:00 2001 From: phot0n Date: Thu, 29 Dec 2022 17:26:08 +0530 Subject: [PATCH 07/58] chore: handle get requests via frappe.whitelist decorator --- frappe/email/doctype/email_account/email_account.py | 1 - frappe/email/oauth.py | 2 +- frappe/integrations/doctype/connected_app/connected_app.py | 4 +--- frappe/translations/ru.csv | 1 - frappe/website/doctype/blog_post/templates/blog_post.html | 2 +- 5 files changed, 3 insertions(+), 7 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 3ad2f67c0b..a0fc8f162e 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -16,7 +16,6 @@ from frappe.email.doctype.email_domain.email_domain import EMAIL_DOMAIN_FIELDS from frappe.email.receive import EmailServer, InboundMail, SentEmailInInboxError from frappe.email.smtp import SMTPServer from frappe.email.utils import get_port -from frappe.integrations.doctype.connected_app.connected_app import check_active_token from frappe.model.document import Document from frappe.utils import cint, comma_or, cstr, parse_addr, validate_email_address from frappe.utils.background_jobs import enqueue, get_jobs diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index c69ab2211e..ad951d770e 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -61,7 +61,7 @@ class Oauth: # where the connect method is called from - hence just logging and raising. raise - def _connect_pop(self) -> bytes: + def _connect_pop(self) -> None: # poplib doesn't have AUTH command implementation res = self._conn._shortcmd( "AUTH {} {}".format( diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index cd219263e9..e8193c89f2 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -117,7 +117,7 @@ class ConnectedApp(Document): return token_cache -@frappe.whitelist(allow_guest=True) +@frappe.whitelist(methods=["GET"], allow_guest=True) def callback(code=None, state=None): """Handle client's code. @@ -125,8 +125,6 @@ def callback(code=None, state=None): transmit a code that can be used by the local server to obtain an access token. """ - if frappe.request.method != "GET": - frappe.throw(_("Invalid request method: {}").format(frappe.request.method)) if frappe.session.user == "Guest": frappe.local.response["type"] = "redirect" diff --git a/frappe/translations/ru.csv b/frappe/translations/ru.csv index 1c99690ff3..f83bf411ff 100644 --- a/frappe/translations/ru.csv +++ b/frappe/translations/ru.csv @@ -6,7 +6,6 @@ Account,Аккаунт, Accounts Manager,Диспетчер учетных записей, Accounts User,Пользователь Учетных записей, Action,Действие, -min read, Действие, Actions,Действия, Active,Активен, Add,Добавить, diff --git a/frappe/website/doctype/blog_post/templates/blog_post.html b/frappe/website/doctype/blog_post/templates/blog_post.html index 708b896357..67236fb26d 100644 --- a/frappe/website/doctype/blog_post/templates/blog_post.html +++ b/frappe/website/doctype/blog_post/templates/blog_post.html @@ -22,7 +22,7 @@ {%- if read_time -%}  · - {{ read_time }} {{ _("min read") }} + {{ read_time }} min read {%- endif -%} From 847206222f9033ae3e1212a94b76c1df94319d48 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 31 Dec 2022 22:17:20 +0530 Subject: [PATCH 08/58] fix: delete option --- frappe/database/database.py | 2 +- frappe/database/query.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index acbd28c9d7..9bb4bfc8e6 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -1160,7 +1160,7 @@ class Database: Doctype name can be passed directly, it will be pre-pended with `tab`. """ filters = filters or kwargs.get("conditions") - query = frappe.qb.get_query(table=doctype, filters=filters).delete() + query = frappe.qb.get_query(table=doctype, filters=filters, delete=True) if "debug" not in kwargs: kwargs["debug"] = debug return query.run(**kwargs) diff --git a/frappe/database/query.py b/frappe/database/query.py index 88de7f7088..7119819a40 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -245,6 +245,7 @@ class Engine: for_update: bool = False, update: bool = False, into: bool = False, + delete: bool = False, ) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: # Clean up state before each query self.is_mariadb = frappe.db.db_type == "mariadb" @@ -259,6 +260,8 @@ class Engine: self.query = frappe.qb.update(self.table) elif into: self.query = frappe.qb.into(self.table) + elif delete: + self.query = frappe.qb.from_(self.table).delete() else: self.query = frappe.qb.from_(self.table) From e272adb0b13a4e0642af1e9af6e758299eee7c7d Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 31 Dec 2022 22:17:39 +0530 Subject: [PATCH 09/58] fix: use table.field instead Field('field') --- frappe/database/query.py | 44 ++++++++-------------------------------- 1 file changed, 8 insertions(+), 36 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 7119819a40..b9e60043a0 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -130,26 +130,6 @@ def func_timespan(key: Field, value: str) -> frappe.qb: return func_between(key, get_timespan_date_range(value)) -def change_orderby(order: str): - """Convert orderby to standart Order object - - Args: - order (str): Field, order - - Returns: - tuple: field, order - """ - order = order.split() - - try: - if order[1].lower() == "asc": - return order[0], Order.asc - except IndexError: - pass - - return order[0], Order.desc - - def literal_eval_(literal): try: return literal_eval(literal) @@ -164,14 +144,6 @@ def has_function(field): return True -def table_from_string(table: str) -> "DocType": - if frappe.db.db_type == "postgres": - table_name = table.split('"', maxsplit=1)[1].split(".")[0][3:].replace('"', "") - else: - table_name = table.split("`", maxsplit=1)[1].split(".")[0][3:].replace("`", "") - return frappe.qb.DocType(table_name=table_name) - - def get_nested_set_hierarchy_result(doctype: str, name: str, hierarchy: str): table = frappe.qb.DocType(doctype) try: @@ -182,20 +154,20 @@ def get_nested_set_hierarchy_result(doctype: str, name: str, hierarchy: str): if hierarchy in ("descendants of", "not descendants of"): result = ( frappe.qb.from_(table) - .select(Field("name")) - .where(Field("lft") > lft) - .where(Field("rgt") < rgt) - .orderby(Field("lft"), order=Order.asc) + .select(table.name) + .where(table.lft > lft) + .where(table.rgt < rgt) + .orderby(table.lft, order=Order.asc) .run() ) else: # Get ancestor elements of a DocType with a tree structure result = ( frappe.qb.from_(table) - .select(Field("name")) - .where(Field("lft") < lft) - .where(Field("rgt") > rgt) - .orderby(Field("lft"), order=Order.desc) + .select(table.name) + .where(table.lft < lft) + .where(table.rgt > rgt) + .orderby(table.lft, order=Order.desc) .run() ) return result From 99160cbe1703d047c36766e07a6e6fa82064b476 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 31 Dec 2022 22:17:54 +0530 Subject: [PATCH 10/58] fix: refactor usage --- frappe/desk/doctype/number_card/number_card.py | 2 +- frappe/desk/listview.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/desk/doctype/number_card/number_card.py b/frappe/desk/doctype/number_card/number_card.py index 30ec99644a..d940448cb1 100644 --- a/frappe/desk/doctype/number_card/number_card.py +++ b/frappe/desk/doctype/number_card/number_card.py @@ -200,7 +200,7 @@ def get_cards_for_user(doctype, txt, searchfield, start, page_len, filters): if txt: search_conditions = [numberCard[field].like(f"%{txt}%") for field in searchfields] - condition_query = frappe.qb.get_query(doctype, filters) + condition_query = frappe.qb.get_query(doctype, filters=filters) return ( condition_query.select(numberCard.name, numberCard.label, numberCard.document_type) diff --git a/frappe/desk/listview.py b/frappe/desk/listview.py index 8b514444df..05d45ad9ac 100644 --- a/frappe/desk/listview.py +++ b/frappe/desk/listview.py @@ -36,7 +36,7 @@ def get_group_by_count(doctype: str, current_filters: str, field: str) -> list[d ToDo = DocType("ToDo") User = DocType("User") count = Count("*").as_("count") - filtered_records = frappe.qb.get_query(doctype, filters=current_filters).select("name") + filtered_records = frappe.qb.get_query(doctype, filters=current_filters, fields=["name"]) return ( frappe.qb.from_(ToDo) From 7ca39a81bfbd9a79d85ae6db1e3cf4a99716ce71 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 31 Dec 2022 22:18:07 +0530 Subject: [PATCH 11/58] fix: explicitly specifiy order --- frappe/tests/test_query.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 957d76d022..486bf9fe49 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -275,7 +275,7 @@ class TestQuery(FrappeTestCase): "Test Tree DocType", fields=["name"], filters={"name": ("descendants of", "Parent 1")}, - order_by="modified", + order_by="modified desc", ).run(as_list=1) # Format decendants result @@ -286,7 +286,7 @@ class TestQuery(FrappeTestCase): "Test Tree DocType", fields=["name"], filters={"name": ("ancestors of", "Child 2")}, - order_by="modified", + order_by="modified desc", ).run(as_list=1) # Format ancestors result @@ -297,7 +297,7 @@ class TestQuery(FrappeTestCase): "Test Tree DocType", fields=["name"], filters={"name": ("not descendants of", "Parent 1")}, - order_by="modified", + order_by="modified desc", ).run(as_dict=1) self.assertListEqual( @@ -313,7 +313,7 @@ class TestQuery(FrappeTestCase): "Test Tree DocType", fields=["name"], filters={"name": ("not ancestors of", "Child 2")}, - order_by="modified", + order_by="modified desc", ).run(as_dict=1) self.assertListEqual( From b7c0ba1beaa2cadc5f6bca52ec4e0943c47681d0 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Sat, 31 Dec 2022 22:55:00 +0530 Subject: [PATCH 12/58] fix: allow dynamic fields in filters e.g., `filters={'link.field': 'value'}` `filters={'child.field': 'value'}` --- frappe/database/query.py | 53 +++++++++++++++++++++++++++++++------- frappe/tests/test_query.py | 43 +++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index b9e60043a0..3593de7c74 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -243,7 +243,7 @@ class Engine: for field in self.fields: if isinstance(field, DynamicTableField): - self.query = field.apply(self.query) + self.query = field.apply_select(self.query) else: self.query = self.query.select(field) @@ -318,18 +318,25 @@ class Engine: _value = value _operator = operator - if has_function(field): + if dynamic_field := DynamicTableField.parse(field, self.doctype): + # apply implicit join if link field's field is referenced + self.query = dynamic_field.apply_join(self.query) + _field = dynamic_field.field + elif has_function(field): _field = self.get_function_object(field) elif not doctype or doctype == self.doctype: _field = self.table[field] elif doctype: _field = self.get_table(doctype)[field] - # keep track of implicit join if child table is referenced + # apply implicit join if child table is referenced if doctype and doctype != self.doctype: meta = frappe.get_meta(doctype) - if meta.istable: - self.implicit_joins.add((doctype, "child")) + table = self.get_table(doctype) + if meta.istable and not self.query.is_joined(table): + self.query = self.query.left_join(table).on( + (table.parent == self.table.name) & (table.parenttype == self.doctype) + ) if isinstance(_value, (str, int)): _value = str(_value) @@ -586,19 +593,38 @@ class DynamicTableField: elif linked_field.fieldtype in frappe.model.table_fields: return ChildTableField(linked_doctype, fieldname, doctype, alias=alias) - def apply(self, query: QueryBuilder) -> QueryBuilder: + def apply_select(self, query: QueryBuilder) -> QueryBuilder: raise NotImplementedError class ChildTableField(DynamicTableField): - def apply(self, query: QueryBuilder) -> QueryBuilder: + def __init__( + self, + doctype: str, + fieldname: str, + parent_doctype: str, + alias: str | None = None, + ) -> None: + self.doctype = doctype + self.fieldname = fieldname + self.alias = alias + self.parent_doctype = parent_doctype + self.table = frappe.qb.DocType(self.doctype) + self.field = self.table[self.fieldname] + + def apply_select(self, query: QueryBuilder) -> QueryBuilder: + table = frappe.qb.DocType(self.doctype) + query = self.apply_join(query) + return query.select(getattr(table, self.fieldname).as_(self.alias or None)) + + def apply_join(self, query: QueryBuilder) -> QueryBuilder: table = frappe.qb.DocType(self.doctype) main_table = frappe.qb.DocType(self.parent_doctype) if not query.is_joined(table): query = query.left_join(table).on( (table.parent == main_table.name) & (table.parenttype == self.parent_doctype) ) - return query.select(getattr(table, self.fieldname).as_(self.alias or None)) + return query class LinkTableField(DynamicTableField): @@ -612,10 +638,17 @@ class LinkTableField(DynamicTableField): ) -> None: super().__init__(doctype, fieldname, parent_doctype, alias=alias) self.link_fieldname = link_fieldname + self.table = frappe.qb.DocType(self.doctype) + self.field = self.table[self.fieldname] - def apply(self, query: QueryBuilder) -> QueryBuilder: + def apply_select(self, query: QueryBuilder) -> QueryBuilder: + table = frappe.qb.DocType(self.doctype) + query = self.apply_join(query) + return query.select(getattr(table, self.fieldname).as_(self.alias or None)) + + def apply_join(self, query: QueryBuilder) -> QueryBuilder: table = frappe.qb.DocType(self.doctype) main_table = frappe.qb.DocType(self.parent_doctype) if not query.is_joined(table): query = query.left_join(table).on(table.name == getattr(main_table, self.link_fieldname)) - return query.select(getattr(table, self.fieldname).as_(self.alias or None)) + return query diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 486bf9fe49..12cb6446d2 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -225,6 +225,39 @@ class TestQuery(FrappeTestCase): frappe.qb.from_("User").select(Max(Field("name"))).where(Ifnull("name", "") < Now()).run(), ) + self.assertEqual( + frappe.qb.get_query( + "DocType", + fields=["name"], + filters={"module.app_name": "frappe"}, + ).get_sql(), + "SELECT `tabDocType`.`name` FROM `tabDocType` LEFT JOIN `tabModule Def` ON `tabModule Def`.`name`=`tabDocType`.`module` WHERE `tabModule Def`.`app_name`='frappe'".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + + self.assertEqual( + frappe.qb.get_query( + "DocType", + fields=["name"], + filters={"module.app_name": ("like", "frap%")}, + ).get_sql(), + "SELECT `tabDocType`.`name` FROM `tabDocType` LEFT JOIN `tabModule Def` ON `tabModule Def`.`name`=`tabDocType`.`module` WHERE `tabModule Def`.`app_name` LIKE 'frap%'".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + + self.assertEqual( + frappe.qb.get_query( + "DocType", + fields=["name"], + filters={"permissions.role": "System Manager"}, + ).get_sql(), + "SELECT `tabDocType`.`name` FROM `tabDocType` LEFT JOIN `tabDocPerm` ON `tabDocPerm`.`parent`=`tabDocType`.`name` AND `tabDocPerm`.`parenttype`='DocType' WHERE `tabDocPerm`.`role`='System Manager'".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + def test_implicit_join_query(self): self.maxDiff = None @@ -261,6 +294,16 @@ class TestQuery(FrappeTestCase): ), ) + self.assertEqual( + frappe.qb.get_query( + "DocType", + fields=["name", "module.app_name as app_name"], + ).get_sql(), + "SELECT `tabDocType`.`name`,`tabModule Def`.`app_name` `app_name` FROM `tabDocType` LEFT JOIN `tabModule Def` ON `tabModule Def`.`name`=`tabDocType`.`module`".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + @run_only_if(db_type_is.MARIADB) def test_comment_stripping(self): self.assertNotIn( From 9d6f82725eb9fcd4b4e51ee9c5d7230b5a9973d6 Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 1 Jan 2023 21:22:36 +0530 Subject: [PATCH 13/58] chore: remove tracking changes for token cache happens very frequently for email and leads to unnecessary bloat in version doctype plus token caches are only read and delete only --- frappe/integrations/doctype/token_cache/token_cache.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/integrations/doctype/token_cache/token_cache.json b/frappe/integrations/doctype/token_cache/token_cache.json index c016405031..0e6601fd98 100644 --- a/frappe/integrations/doctype/token_cache/token_cache.json +++ b/frappe/integrations/doctype/token_cache/token_cache.json @@ -86,10 +86,11 @@ } ], "links": [], - "modified": "2020-11-13 13:35:53.714352", + "modified": "2023-01-01 21:01:24.405729", "modified_by": "Administrator", "module": "Integrations", "name": "Token Cache", + "naming_rule": "Expression", "owner": "Administrator", "permissions": [ { @@ -106,5 +107,5 @@ ], "sort_field": "modified", "sort_order": "DESC", - "track_changes": 1 + "states": [] } \ No newline at end of file From 52daef0dfd30b8b0a0559cefdd7acfa521f197eb Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 1 Jan 2023 22:43:33 +0530 Subject: [PATCH 14/58] fix: get_expires_in logic for token cache --- frappe/integrations/doctype/token_cache/token_cache.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/integrations/doctype/token_cache/token_cache.py b/frappe/integrations/doctype/token_cache/token_cache.py index b01974e5ab..034b1d9107 100644 --- a/frappe/integrations/doctype/token_cache/token_cache.py +++ b/frappe/integrations/doctype/token_cache/token_cache.py @@ -52,11 +52,10 @@ class TokenCache(Document): return self def get_expires_in(self): + modified = frappe.utils.get_datetime(self.modified) + expiry_utc = modified.astimezone(pytz.utc) + timedelta(seconds=self.expires_in) now_utc = datetime.utcnow().replace(tzinfo=pytz.utc) - expiry_time = frappe.utils.get_datetime(self.modified) + timedelta(seconds=self.expires_in) - expiry_local = expiry_time.replace(tzinfo=pytz.timezone(frappe.utils.get_time_zone())) - expiry_utc = expiry_local.astimezone(pytz.utc) - return (expiry_utc - now_utc).total_seconds() + return cint((expiry_utc - now_utc).total_seconds()) def is_expired(self): return self.get_expires_in() < 0 From f50be4bbf5ae9396dabf3e5e31ed1e40e9fcf8d9 Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 1 Jan 2023 23:00:03 +0530 Subject: [PATCH 15/58] chore: log any exception while refreshing access token in connected apps --- .../doctype/connected_app/connected_app.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index e8193c89f2..ff2eb2dc96 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -107,11 +107,17 @@ class ConnectedApp(Document): token_cache = self.get_token_cache(user) if token_cache and token_cache.is_expired(): oauth_session = self.get_oauth2_session(user) - token = oauth_session.refresh_token( - body=f"redirect_uri={self.redirect_uri}", - token_url=self.token_uri, - refresh_token=token_cache.get_password("refresh_token"), - ) + + try: + token = oauth_session.refresh_token( + body=f"redirect_uri={self.redirect_uri}", + token_url=self.token_uri, + refresh_token=token_cache.get_password("refresh_token"), + ) + except Exception: + self.log_error("Token Refresh Error") + return None + token_cache.update_data(token) return token_cache From 6bed904bf7f0145dd12abff1f45327e4e3aa6fee Mon Sep 17 00:00:00 2001 From: phot0n Date: Tue, 3 Jan 2023 18:05:49 +0530 Subject: [PATCH 16/58] fix: only validate oauth if tokens are set * also brough back oauth authorization message --- .../doctype/email_account/email_account.js | 23 +++++++++++++++++++ .../doctype/email_account/email_account.py | 3 ++- .../doctype/connected_app/connected_app.py | 1 - 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index 31da072692..f139c23ce6 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -74,6 +74,7 @@ function oauth_access(frm) { method: "initiate_web_application_flow", args: { success_uri: window.location.pathname, + user: frm.doc.connected_user, }, callback: function (r) { window.open(r.message, "_self"); @@ -147,6 +148,7 @@ frappe.ui.form.on("Email Account", { frm.refresh_field("imap_folder"); } set_default_max_attachment_size(frm); + frm.events.show_oauth_authorization_message(frm); }, refresh: function (frm) { @@ -180,6 +182,27 @@ frappe.ui.form.on("Email Account", { oauth_access(frm); }, + show_oauth_authorization_message(frm) { + if (frm.doc.auth_method === "OAuth") { + frappe.call({ + method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + args: { + connected_app: frm.doc.connected_app, + connected_user: frm.doc.connected_user, + }, + callback: (r) => { + if (!r.message) { + let msg = __( + 'OAuth has been enabled but not authorised. Please use "Authorise API Access" button to do the same.' + ); + frm.dashboard.clear_headline(); + frm.dashboard.set_headline_alert(msg, "yellow"); + } + }, + }); + } + }, + domain: frappe.utils.debounce((frm) => { if (frm.doc.domain) { frappe.call({ diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index a0fc8f162e..66f7e1c688 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -82,6 +82,7 @@ class EmailAccount(Document): return use_oauth = self.auth_method == "OAuth" + validate_oauth = use_oauth and not (self.is_new() and not self.get_oauth_token()) self.use_starttls = cint(self.use_imap and self.use_starttls and not self.use_ssl) if use_oauth: @@ -90,7 +91,7 @@ class EmailAccount(Document): self.password = None if not frappe.local.flags.in_install and not self.awaiting_password: - if use_oauth or self.password or self.smtp_server in ("127.0.0.1", "localhost"): + if validate_oauth or self.password or self.smtp_server in ("127.0.0.1", "localhost"): if self.enable_incoming: self.get_incoming_server() self.no_failed = 0 diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index ff2eb2dc96..f78ccd59ce 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -112,7 +112,6 @@ class ConnectedApp(Document): token = oauth_session.refresh_token( body=f"redirect_uri={self.redirect_uri}", token_url=self.token_uri, - refresh_token=token_cache.get_password("refresh_token"), ) except Exception: self.log_error("Token Refresh Error") From 481d72c0aee32a3c4242f8f5265bc4695287ca94 Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 6 Jan 2023 13:07:13 +0530 Subject: [PATCH 17/58] fix: only pull from email accoutns which have token * removed unnecessary after_save client hook from email account * renamed check_active_token -> has_token (no need to refresh the access token) * removed oauthentication error from email oauth - now just a simple validation error --- .../doctype/email_account/email_account.js | 19 +------------------ .../doctype/email_account/email_account.py | 13 +++++++++++-- frappe/email/oauth.py | 13 +++++-------- .../doctype/connected_app/connected_app.py | 5 +++-- .../doctype/token_cache/token_cache.py | 4 ++-- 5 files changed, 22 insertions(+), 32 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index f139c23ce6..bc3d168639 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -161,23 +161,6 @@ frappe.ui.form.on("Email Account", { } }, - after_save(frm) { - if (frm.doc.auth_method === "OAuth") { - frappe.call({ - method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", - args: { - connected_app: frm.doc.connected_app, - connected_user: frm.doc.connected_user, - }, - callback: (r) => { - if (!r.message) { - oauth_access(frm); - } - }, - }); - } - }, - authorize_api_access: function (frm) { oauth_access(frm); }, @@ -185,7 +168,7 @@ frappe.ui.form.on("Email Account", { show_oauth_authorization_message(frm) { if (frm.doc.auth_method === "OAuth") { frappe.call({ - method: "frappe.integrations.doctype.connected_app.connected_app.check_active_token", + method: "frappe.integrations.doctype.connected_app.connected_app.has_token", args: { connected_app: frm.doc.connected_app, connected_user: frm.doc.connected_user, diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 66f7e1c688..1e08ce5615 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -776,15 +776,24 @@ def pull(now=False): else: return + from frappe.integrations.doctype.connected_app.connected_app import has_token + doctype = frappe.qb.DocType("Email Account") email_accounts = ( frappe.qb.from_(doctype) - .select(doctype.name) + .select(doctype.name, doctype.auth_method, doctype.connected_app, doctype.connected_user) .where(doctype.enable_incoming == 1) - .where((doctype.awaiting_password == 0) | (doctype.auth_method == "OAuth")) + .where(doctype.awaiting_password == 0) .run(as_dict=1) ) + for email_account in email_accounts: + if email_account.auth_method == "OAuth" and not has_token( + email_account.connected_app, email_account.connected_user + ): + # don't try to pull from accounts which dont have access token (for Oauth) + continue + if now: pull_from_email_account(email_account.name) diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index ad951d770e..6b56047069 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -6,10 +6,6 @@ from smtplib import SMTP import frappe -class OAuthenticationError(Exception): - pass - - class Oauth: def __init__( self, @@ -32,8 +28,7 @@ class Oauth: if not self._access_token: frappe.throw( frappe._("Please Authorize OAuth for Email Account {}").format(self.email_account), - OAuthenticationError, - frappe._("OAuth Error"), + title=frappe._("OAuth Error"), ) @property @@ -53,9 +48,11 @@ class Oauth: # SMTP self._connect_smtp() - except Exception as e: + except Exception: frappe.log_error( - "Email Connection Error - Authentication Failed", str(e), "Email Account", self.email_account + "Email Connection Error - Authentication Failed", + reference_doctype="Email Account", + reference_name=self.email_account, ) # raising a bare exception here as we have a lot of exception handling present # where the connect method is called from - hence just logging and raising. diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index f78ccd59ce..ca9677d4da 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -162,6 +162,7 @@ def callback(code=None, state=None): @frappe.whitelist() -def check_active_token(connected_app, connected_user=None): +def has_token(connected_app, connected_user=None): app = frappe.get_doc("Connected App", connected_app) - return bool(app.get_active_token(connected_user)) + token_cache = app.get_token_cache(connected_user or frappe.session.user) + return bool(token_cache.get_json()["access_token"]) diff --git a/frappe/integrations/doctype/token_cache/token_cache.py b/frappe/integrations/doctype/token_cache/token_cache.py index 034b1d9107..2facc006c6 100644 --- a/frappe/integrations/doctype/token_cache/token_cache.py +++ b/frappe/integrations/doctype/token_cache/token_cache.py @@ -62,8 +62,8 @@ class TokenCache(Document): def get_json(self): return { - "access_token": self.get_password("access_token", ""), - "refresh_token": self.get_password("refresh_token", ""), + "access_token": self.get_password("access_token", False), + "refresh_token": self.get_password("refresh_token", False), "expires_in": self.get_expires_in(), "token_type": self.token_type, } From e4ac91a035faf88560911057b0e8cf926a619ad9 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 15:20:30 +0530 Subject: [PATCH 18/58] fix: ignore string with parenthesis if it is not an sql function --- frappe/database/query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 3593de7c74..ac23097226 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -140,7 +140,7 @@ def literal_eval_(literal): def has_function(field): _field = field.casefold() if (isinstance(field, str) and "`" not in field) else field if not issubclass(type(_field), Criterion): - if any([f"{func}(" in _field for func in SQL_FUNCTIONS]) or "(" in _field: + if any([f"{func}(" in _field for func in SQL_FUNCTIONS]): return True From 35c2654f005a60495978f6588e2eb4be64f87872 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 15:34:50 +0530 Subject: [PATCH 19/58] chore: indentation fix --- frappe/database/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index c9c37fda30..c104c9cf9c 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -887,7 +887,7 @@ class Database: field, val, modified=modified, modified_by=modified_by, update_modified=update_modified ) - query = frappe.qb.get_query(table=dt, filters=dn, update=True) + query = frappe.qb.get_query(table=dt, filters=dn, update=True) if isinstance(dn, str): frappe.clear_document_cache(dt, dn) From f982439eb9a86561cdb0b9d449ab83a12c56e0a0 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 16:43:44 +0530 Subject: [PATCH 20/58] fix: pass fields explicitly - to prevent addition of default `name` field - also, add fields only if it is a select query --- frappe/database/query.py | 18 +++++++++--------- frappe/query_builder/functions.py | 6 +++--- frappe/utils/goal.py | 11 +++++++---- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index ac23097226..b9dfa33217 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -236,16 +236,16 @@ class Engine: self.query = frappe.qb.from_(self.table).delete() else: self.query = frappe.qb.from_(self.table) + # add fields + self.fields = self.parse_fields(fields) + if not self.fields: + self.fields = [getattr(self.table, pluck or "name")] - self.fields = self.parse_fields(fields) - if not self.fields: - self.fields = [getattr(self.table, pluck or "name")] - - for field in self.fields: - if isinstance(field, DynamicTableField): - self.query = field.apply_select(self.query) - else: - self.query = self.query.select(field) + for field in self.fields: + if isinstance(field, DynamicTableField): + self.query = field.apply_select(self.query) + else: + self.query = self.query.select(field) self.apply_filters(filters) self.apply_implicit_joins() diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index e1ab182553..512df8835c 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -119,9 +119,9 @@ class Cast_(Function): def _aggregate(function, dt, fieldname, filters, **kwargs): return ( - frappe.qb.get_query(dt, filters=filters) - .select(function(PseudoColumn(fieldname))) - .run(**kwargs)[0][0] + frappe.qb.get_query(dt, filters=filters, fields=[function(PseudoColumn(fieldname))]).run( + **kwargs + )[0][0] or 0 ) diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index 0dcadc5ec6..f60aec4d2b 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -24,10 +24,13 @@ def get_monthly_results( date_format = "%m-%Y" if frappe.db.db_type != "postgres" else "MM-YYYY" return dict( - frappe.qb.get_query(table=goal_doctype, filters=filters) - .select( - DateFormat(Table[date_col], date_format).as_("month_year"), - Function(aggregation, goal_field), + frappe.qb.get_query( + table=goal_doctype, + fields=[ + DateFormat(Table[date_col], date_format).as_("month_year"), + Function(aggregation, goal_field), + ], + filters=filters, ) .groupby("month_year") .run() From 6192a9285a561b0e7d219ca04a36251aaec864a7 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 17:51:55 +0530 Subject: [PATCH 21/58] fix: use Field objects as is in apply_filter --- frappe/database/query.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index b9dfa33217..dc6511bb95 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -318,7 +318,9 @@ class Engine: _value = value _operator = operator - if dynamic_field := DynamicTableField.parse(field, self.doctype): + if isinstance(_field, Field): + pass + elif dynamic_field := DynamicTableField.parse(field, self.doctype): # apply implicit join if link field's field is referenced self.query = dynamic_field.apply_join(self.query) _field = dynamic_field.field From 9e9de7053cf7c852daa3d631f2f25239d90c2cf9 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 18:19:31 +0530 Subject: [PATCH 22/58] fix: set default order_by direction to desc --- frappe/database/query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index dc6511bb95..eb092c9d0d 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -502,7 +502,7 @@ class Engine: for declaration in order_by.split(","): if _order_by := declaration.strip(): parts = _order_by.split(" ") - order_field, order_direction = parts[0], parts[1] if len(parts) > 1 else "asc" + order_field, order_direction = parts[0], parts[1] if len(parts) > 1 else "desc" order_direction = Order.asc if order_direction.lower() == "asc" else Order.desc self.query = self.query.orderby(order_field, order=order_direction) From 60febc9799e0c45590ed966967c6ec03377cefed Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 19:43:48 +0530 Subject: [PATCH 23/58] fix: list filter filters as list must always be list of list --- frappe/tests/test_db.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index 3962cc746d..ed01af655c 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -545,7 +545,7 @@ class TestDB(FrappeTestCase): self.assertEqual((frappe.db.count("Note")), 2) # simple filters - self.assertEqual((frappe.db.count("Note", ["title", "=", "note1"])), 1) + self.assertEqual((frappe.db.count("Note", [["title", "=", "note1"]])), 1) frappe.get_doc(doctype="Note", title="note3", content="something other").insert() From 08fc5b5c90368d66b5374d7478227b8b8dab3290 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 9 Jan 2023 19:54:26 +0530 Subject: [PATCH 24/58] fix: allow list of dict in filters --- frappe/database/query.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index eb092c9d0d..532e03f2c6 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -292,19 +292,22 @@ class Engine: self.apply_dict_filters(filters) elif isinstance(filters, (list, tuple)): - self.apply_list_filters(filters) + for filter in filters: + if isinstance(filter, (str, int, Criterion, dict)): + self.apply_filters(filter) + elif isinstance(filter, (list, tuple)): + self.apply_list_filters(filter) - def apply_list_filters(self, filters: list[list]): - for filter in filters: - if len(filter) == 2: - field, value = filter - self._apply_filter(field, value) - elif len(filter) == 3: - field, operator, value = filter - self._apply_filter(field, value, operator) - elif len(filter) == 4: - doctype, field, operator, value = filter - self._apply_filter(field, value, operator, doctype) + def apply_list_filters(self, filter: list): + if len(filter) == 2: + field, value = filter + self._apply_filter(field, value) + elif len(filter) == 3: + field, operator, value = filter + self._apply_filter(field, value, operator) + elif len(filter) == 4: + doctype, field, operator, value = filter + self._apply_filter(field, value, operator, doctype) def apply_dict_filters(self, filters: dict[str, str | int | list]): for key in filters: From fe13108eecc246c9e0aa91987e3b235ba1098ac2 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 10 Jan 2023 16:15:08 +0530 Subject: [PATCH 25/58] fix: refactor - move operator map in separate file - remove unnecessary code - organize functions --- frappe/database/operator_map.py | 138 +++++++++++++++ frappe/database/query.py | 305 +++++++------------------------- frappe/query_builder/utils.py | 6 +- 3 files changed, 208 insertions(+), 241 deletions(-) create mode 100644 frappe/database/operator_map.py diff --git a/frappe/database/operator_map.py b/frappe/database/operator_map.py new file mode 100644 index 0000000000..2c8b53dae3 --- /dev/null +++ b/frappe/database/operator_map.py @@ -0,0 +1,138 @@ +# Copyright (c) 2023, Frappe Technologies Pvt. Ltd. and Contributors +# MIT License. See license.txt + +import operator +from typing import Callable + +import frappe +from frappe.database.utils import NestedSetHierarchy +from frappe.model.db_query import get_timespan_date_range +from frappe.query_builder import Field + + +def like(key: Field, value: str) -> frappe.qb: + """Wrapper method for `LIKE` + + Args: + key (str): field + value (str): criterion + + Returns: + frappe.qb: `frappe.qb object with `LIKE` + """ + return key.like(value) + + +def func_in(key: Field, value: list | tuple) -> frappe.qb: + """Wrapper method for `IN` + + Args: + key (str): field + value (Union[int, str]): criterion + + Returns: + frappe.qb: `frappe.qb object with `IN` + """ + if isinstance(value, str): + value = value.split(",") + return key.isin(value) + + +def not_like(key: Field, value: str) -> frappe.qb: + """Wrapper method for `NOT LIKE` + + Args: + key (str): field + value (str): criterion + + Returns: + frappe.qb: `frappe.qb object with `NOT LIKE` + """ + return key.not_like(value) + + +def func_not_in(key: Field, value: list | tuple | str): + """Wrapper method for `NOT IN` + + Args: + key (str): field + value (Union[int, str]): criterion + + Returns: + frappe.qb: `frappe.qb object with `NOT IN` + """ + if isinstance(value, str): + value = value.split(",") + return key.notin(value) + + +def func_regex(key: Field, value: str) -> frappe.qb: + """Wrapper method for `REGEX` + + Args: + key (str): field + value (str): criterion + + Returns: + frappe.qb: `frappe.qb object with `REGEX` + """ + return key.regex(value) + + +def func_between(key: Field, value: list | tuple) -> frappe.qb: + """Wrapper method for `BETWEEN` + + Args: + key (str): field + value (Union[int, str]): criterion + + Returns: + frappe.qb: `frappe.qb object with `BETWEEN` + """ + return key[slice(*value)] + + +def func_is(key, value): + "Wrapper for IS" + return key.isnotnull() if value.lower() == "set" else key.isnull() + + +def func_timespan(key: Field, value: str) -> frappe.qb: + """Wrapper method for `TIMESPAN` + + Args: + key (str): field + value (str): criterion + + Returns: + frappe.qb: `frappe.qb object with `TIMESPAN` + """ + + return func_between(key, get_timespan_date_range(value)) + + +# default operators +OPERATOR_MAP: dict[str, Callable] = { + "+": operator.add, + "=": operator.eq, + "-": operator.sub, + "!=": operator.ne, + "<": operator.lt, + ">": operator.gt, + "<=": operator.le, + "=<": operator.le, + ">=": operator.ge, + "=>": operator.ge, + "/": operator.truediv, + "*": operator.mul, + "in": func_in, + "not in": func_not_in, + "like": like, + "not like": not_like, + "regex": func_regex, + "between": func_between, + "is": func_is, + "timespan": func_timespan, + "nested_set": NestedSetHierarchy, + # TODO: Add support for custom operators (WIP) - via filters_config hooks +} diff --git a/frappe/database/query.py b/frappe/database/query.py index 532e03f2c6..4852f80739 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -1,20 +1,16 @@ import itertools -import operator import re from ast import literal_eval -from functools import cached_property from types import BuiltinFunctionType -from typing import TYPE_CHECKING, Callable +from typing import TYPE_CHECKING import sqlparse -from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder from pypika.queries import QueryBuilder import frappe from frappe import _ -from frappe.database.utils import NestedSetHierarchy, is_pypika_function_object -from frappe.model.db_query import get_timespan_date_range -from frappe.query_builder import Criterion, Field, Order, Table, functions +from frappe.database.operator_map import OPERATOR_MAP +from frappe.query_builder import Criterion, Field, Order, functions from frappe.query_builder.functions import Function, SqlFunctions from frappe.query_builder.utils import PseudoColumnMapper from frappe.utils.data import MARIADB_SPECIFIC_COMMENT @@ -29,186 +25,12 @@ SQL_FUNCTIONS = [sql_function.value for sql_function in SqlFunctions] COMMA_PATTERN = re.compile(r",\s*(?![^()]*\))") -def like(key: Field, value: str) -> frappe.qb: - """Wrapper method for `LIKE` - - Args: - key (str): field - value (str): criterion - - Returns: - frappe.qb: `frappe.qb object with `LIKE` - """ - return key.like(value) - - -def func_in(key: Field, value: list | tuple) -> frappe.qb: - """Wrapper method for `IN` - - Args: - key (str): field - value (Union[int, str]): criterion - - Returns: - frappe.qb: `frappe.qb object with `IN` - """ - if isinstance(value, str): - value = value.split(",") - return key.isin(value) - - -def not_like(key: Field, value: str) -> frappe.qb: - """Wrapper method for `NOT LIKE` - - Args: - key (str): field - value (str): criterion - - Returns: - frappe.qb: `frappe.qb object with `NOT LIKE` - """ - return key.not_like(value) - - -def func_not_in(key: Field, value: list | tuple | str): - """Wrapper method for `NOT IN` - - Args: - key (str): field - value (Union[int, str]): criterion - - Returns: - frappe.qb: `frappe.qb object with `NOT IN` - """ - if isinstance(value, str): - value = value.split(",") - return key.notin(value) - - -def func_regex(key: Field, value: str) -> frappe.qb: - """Wrapper method for `REGEX` - - Args: - key (str): field - value (str): criterion - - Returns: - frappe.qb: `frappe.qb object with `REGEX` - """ - return key.regex(value) - - -def func_between(key: Field, value: list | tuple) -> frappe.qb: - """Wrapper method for `BETWEEN` - - Args: - key (str): field - value (Union[int, str]): criterion - - Returns: - frappe.qb: `frappe.qb object with `BETWEEN` - """ - return key[slice(*value)] - - -def func_is(key, value): - "Wrapper for IS" - return key.isnotnull() if value.lower() == "set" else key.isnull() - - -def func_timespan(key: Field, value: str) -> frappe.qb: - """Wrapper method for `TIMESPAN` - - Args: - key (str): field - value (str): criterion - - Returns: - frappe.qb: `frappe.qb object with `TIMESPAN` - """ - - return func_between(key, get_timespan_date_range(value)) - - -def literal_eval_(literal): - try: - return literal_eval(literal) - except (ValueError, SyntaxError): - return literal - - -def has_function(field): - _field = field.casefold() if (isinstance(field, str) and "`" not in field) else field - if not issubclass(type(_field), Criterion): - if any([f"{func}(" in _field for func in SQL_FUNCTIONS]): - return True - - -def get_nested_set_hierarchy_result(doctype: str, name: str, hierarchy: str): - table = frappe.qb.DocType(doctype) - try: - lft, rgt = frappe.qb.from_(table).select("lft", "rgt").where(table.name == name).run()[0] - except IndexError: - lft, rgt = None, None - - if hierarchy in ("descendants of", "not descendants of"): - result = ( - frappe.qb.from_(table) - .select(table.name) - .where(table.lft > lft) - .where(table.rgt < rgt) - .orderby(table.lft, order=Order.asc) - .run() - ) - else: - # Get ancestor elements of a DocType with a tree structure - result = ( - frappe.qb.from_(table) - .select(table.name) - .where(table.lft < lft) - .where(table.rgt > rgt) - .orderby(table.lft, order=Order.desc) - .run() - ) - return result - - -# default operators -OPERATOR_MAP: dict[str, Callable] = { - "+": operator.add, - "=": operator.eq, - "-": operator.sub, - "!=": operator.ne, - "<": operator.lt, - ">": operator.gt, - "<=": operator.le, - "=<": operator.le, - ">=": operator.ge, - "=>": operator.ge, - "/": operator.truediv, - "*": operator.mul, - "in": func_in, - "not in": func_not_in, - "like": like, - "not like": not_like, - "regex": func_regex, - "between": func_between, - "is": func_is, - "timespan": func_timespan, - "nested_set": NestedSetHierarchy, - # TODO: Add support for custom operators (WIP) - via filters_config hooks -} - - class Engine: - tables: dict[str, str] = {} - def get_query( self, table: str, fields: list | tuple | None = None, filters: dict[str, str | int] | str | int | list[list | str | int] | None = None, - pluck: str | None = None, order_by: str | None = None, group_by: str | None = None, limit: int | None = None, @@ -218,15 +40,11 @@ class Engine: update: bool = False, into: bool = False, delete: bool = False, - ) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: - # Clean up state before each query + ) -> QueryBuilder: self.is_mariadb = frappe.db.db_type == "mariadb" self.is_postgres = frappe.db.db_type == "postgres" - self.tables = {} - self.implicit_joins = set() - self.doctype = table - self.table = self.get_table(table) + self.table = frappe.qb.DocType(table) if update: self.query = frappe.qb.update(self.table) @@ -236,19 +54,9 @@ class Engine: self.query = frappe.qb.from_(self.table).delete() else: self.query = frappe.qb.from_(self.table) - # add fields - self.fields = self.parse_fields(fields) - if not self.fields: - self.fields = [getattr(self.table, pluck or "name")] - - for field in self.fields: - if isinstance(field, DynamicTableField): - self.query = field.apply_select(self.query) - else: - self.query = self.query.select(field) + self.apply_fields(fields) self.apply_filters(filters) - self.apply_implicit_joins() self.apply_order_by(order_by) if limit: @@ -268,16 +76,21 @@ class Engine: return self.query - def get_table(self, table_name: str | Table) -> Table: - if isinstance(table_name, Table): - return table_name - table_name = table_name.strip('"').strip("'") - if table_name not in self.tables: - self.tables[table_name] = frappe.qb.DocType(table_name) - return self.tables[table_name] + def apply_fields(self, fields): + # add fields + self.fields = self.parse_fields(fields) + if not self.fields: + self.fields = [getattr(self.table, "name")] + + for field in self.fields: + if isinstance(field, DynamicTableField): + self.query = field.apply_select(self.query) + else: + self.query = self.query.select(field) def apply_filters( - self, filters: dict[str, str | int | list] | str | int | list[list] | None = None + self, + filters: dict[str, str | int] | str | int | list[list | str | int] | None = None, ): if not filters: return @@ -332,12 +145,12 @@ class Engine: elif not doctype or doctype == self.doctype: _field = self.table[field] elif doctype: - _field = self.get_table(doctype)[field] + _field = frappe.qb.DocType(doctype)[field] # apply implicit join if child table is referenced if doctype and doctype != self.doctype: meta = frappe.get_meta(doctype) - table = self.get_table(doctype) + table = frappe.qb.DocType(doctype) if meta.istable and not self.query.is_joined(table): self.query = self.query.left_join(table).on( (table.parent == self.table.name) & (table.parenttype == self.doctype) @@ -354,14 +167,14 @@ class Engine: _value = self.get_function_object(_value) # Nested set - if _operator in self.OPERATOR_MAP["nested_set"]: + if _operator in OPERATOR_MAP["nested_set"]: hierarchy = _operator docname = _value result = get_nested_set_hierarchy_result(self.doctype, docname, hierarchy) operator_fn = ( - self.OPERATOR_MAP["not in"] + OPERATOR_MAP["not in"] if hierarchy in ("not ancestors of", "not descendants of") - else self.OPERATOR_MAP["in"] + else OPERATOR_MAP["in"] ) if result: result = list(itertools.chain.from_iterable(result)) @@ -370,7 +183,7 @@ class Engine: self.query = self.query.where(operator_fn(_field, ("",))) return - operator_fn = self.OPERATOR_MAP[_operator.casefold()] + operator_fn = OPERATOR_MAP[_operator.casefold()] if _value is None and isinstance(_field, Field): self.query = self.query.where(_field.isnull()) else: @@ -490,15 +303,6 @@ class Engine: return _fields - def apply_implicit_joins(self): - for d in self.implicit_joins: - doctype, join_type = d - table = self.get_table(doctype) - if join_type == "child": - self.query = self.query.left_join(table).on( - (table.parent == self.table.name) & (table.parenttype == self.doctype) - ) - def apply_order_by(self, order_by: str | None): if not order_by or order_by == "KEEP_DEFAULT_ORDERING": return @@ -509,22 +313,6 @@ class Engine: order_direction = Order.asc if order_direction.lower() == "asc" else Order.desc self.query = self.query.orderby(order_field, order=order_direction) - @cached_property - def OPERATOR_MAP(self): - # default operators - all_operators = OPERATOR_MAP.copy() - - # TODO: update with site-specific custom operators / removed previous buggy implementation - if frappe.get_hooks("filters_config"): - from frappe.utils.commands import warn - - warn( - "The 'filters_config' hook used to add custom operators is not yet implemented" - " in frappe.db.query engine. Use db_query (frappe.get_list) instead." - ) - - return all_operators - class Permission: @classmethod @@ -657,3 +445,46 @@ class LinkTableField(DynamicTableField): if not query.is_joined(table): query = query.left_join(table).on(table.name == getattr(main_table, self.link_fieldname)) return query + + +def literal_eval_(literal): + try: + return literal_eval(literal) + except (ValueError, SyntaxError): + return literal + + +def has_function(field): + _field = field.casefold() if (isinstance(field, str) and "`" not in field) else field + if not issubclass(type(_field), Criterion): + if any([f"{func}(" in _field for func in SQL_FUNCTIONS]): + return True + + +def get_nested_set_hierarchy_result(doctype: str, name: str, hierarchy: str): + table = frappe.qb.DocType(doctype) + try: + lft, rgt = frappe.qb.from_(table).select("lft", "rgt").where(table.name == name).run()[0] + except IndexError: + lft, rgt = None, None + + if hierarchy in ("descendants of", "not descendants of"): + result = ( + frappe.qb.from_(table) + .select(table.name) + .where(table.lft > lft) + .where(table.rgt < rgt) + .orderby(table.lft, order=Order.asc) + .run() + ) + else: + # Get ancestor elements of a DocType with a tree structure + result = ( + frappe.qb.from_(table) + .select(table.name) + .where(table.lft < lft) + .where(table.rgt > rgt) + .orderby(table.lft, order=Order.desc) + .run() + ) + return result diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 004a226036..bfc2c49b8e 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -2,9 +2,7 @@ from enum import Enum from importlib import import_module from typing import Any, Callable, get_type_hints -from pypika import Query -from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder -from pypika.queries import Column +from pypika.queries import Column, QueryBuilder from pypika.terms import PseudoColumn import frappe @@ -56,7 +54,7 @@ def get_query_builder(type_of_db: str) -> Postgres | MariaDB: return picks[db] -def get_query(*args, **kwargs) -> MySQLQueryBuilder | PostgreSQLQueryBuilder: +def get_query(*args, **kwargs) -> QueryBuilder: from frappe.database.query import Engine return Engine().get_query(*args, **kwargs) From 95d8a0f919c68f5f828256b4957045bb786e05de Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 10 Jan 2023 16:48:38 +0530 Subject: [PATCH 26/58] fix: allow Table instance --- frappe/database/query.py | 14 ++++++++++---- frappe/database/utils.py | 8 ++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 4852f80739..cdca28601d 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -5,11 +5,12 @@ from types import BuiltinFunctionType from typing import TYPE_CHECKING import sqlparse -from pypika.queries import QueryBuilder +from pypika.queries import QueryBuilder, Table import frappe from frappe import _ from frappe.database.operator_map import OPERATOR_MAP +from frappe.database.utils import get_doctype_name from frappe.query_builder import Criterion, Field, Order, functions from frappe.query_builder.functions import Function, SqlFunctions from frappe.query_builder.utils import PseudoColumnMapper @@ -28,7 +29,7 @@ COMMA_PATTERN = re.compile(r",\s*(?![^()]*\))") class Engine: def get_query( self, - table: str, + table: str | Table, fields: list | tuple | None = None, filters: dict[str, str | int] | str | int | list[list | str | int] | None = None, order_by: str | None = None, @@ -43,8 +44,13 @@ class Engine: ) -> QueryBuilder: self.is_mariadb = frappe.db.db_type == "mariadb" self.is_postgres = frappe.db.db_type == "postgres" - self.doctype = table - self.table = frappe.qb.DocType(table) + + if isinstance(table, Table): + self.table = table + self.doctype = get_doctype_name(table.get_sql()) + else: + self.doctype = table + self.table = frappe.qb.DocType(table) if update: self.query = frappe.qb.update(self.table) diff --git a/frappe/database/utils.py b/frappe/database/utils.py index 4ea039e5a7..f89ba3c737 100644 --- a/frappe/database/utils.py +++ b/frappe/database/utils.py @@ -34,6 +34,14 @@ def is_pypika_function_object(field: str) -> bool: return getattr(field, "__module__", None) == "pypika.functions" or isinstance(field, Function) +def get_doctype_name(table_name: str) -> str: + if "tab" in table_name: + table_name = table_name.replace("tab", "") + table_name = table_name.replace("`", "") + table_name = table_name.replace('"', "") + return table_name + + class LazyString: def _setup(self) -> None: raise NotImplementedError From a0f6a5ff46068dd9b9665bb1bd2e16564b84a2ce Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 10 Jan 2023 18:21:50 +0530 Subject: [PATCH 27/58] fix: move pluck to run --- frappe/database/database.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index c104c9cf9c..2c0718a48a 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -835,10 +835,9 @@ class Database: fields=field, filters=names, order_by=order_by, - pluck=pluck, distinct=distinct, limit=limit, - ).run(debug=debug, run=run, as_dict=as_dict) + ).run(debug=debug, run=run, as_dict=as_dict, pluck=pluck) return {} def set_value( From 76deeb531cd6a8d022426658dd3e488dff0ce127 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 10 Jan 2023 18:22:05 +0530 Subject: [PATCH 28/58] fix: support list of str or int in filters --- frappe/database/query.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index cdca28601d..726f930b19 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -111,11 +111,14 @@ class Engine: self.apply_dict_filters(filters) elif isinstance(filters, (list, tuple)): - for filter in filters: - if isinstance(filter, (str, int, Criterion, dict)): - self.apply_filters(filter) - elif isinstance(filter, (list, tuple)): - self.apply_list_filters(filter) + if all(isinstance(d, (str, int)) for d in filters): + self.apply_dict_filters({"name": ("in", filters)}) + else: + for filter in filters: + if isinstance(filter, (str, int, Criterion, dict)): + self.apply_filters(filter) + elif isinstance(filter, (list, tuple)): + self.apply_list_filters(filter) def apply_list_filters(self, filter: list): if len(filter) == 2: From 5340efd1567759c6fc532094beb453989b5c81d2 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Fri, 13 Jan 2023 16:21:51 +0530 Subject: [PATCH 29/58] fix: don't cast integer value in filter --- frappe/database/query.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 726f930b19..7152b77dab 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -165,9 +165,7 @@ class Engine: (table.parent == self.table.name) & (table.parenttype == self.doctype) ) - if isinstance(_value, (str, int)): - _value = str(_value) - elif isinstance(_value, (list, tuple)): + if isinstance(_value, (list, tuple)): _operator, _value = _value elif isinstance(_value, bool): _value = int(_value) From a93380ac9c55135443f4c0ad5d4ac24efa6e75f2 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Fri, 13 Jan 2023 16:22:25 +0530 Subject: [PATCH 30/58] fix: handle empty list for "in" and "not in" --- frappe/database/query.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/database/query.py b/frappe/database/query.py index 7152b77dab..ea5f2b8760 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -173,6 +173,9 @@ class Engine: if isinstance(_value, str) and has_function(_value): _value = self.get_function_object(_value) + if isinstance(_value, (list, tuple)) and not _value: + _value = ("",) + # Nested set if _operator in OPERATOR_MAP["nested_set"]: hierarchy = _operator From c2ceceea6e2934754f910d1e25c307ae36b0e179 Mon Sep 17 00:00:00 2001 From: phot0n Date: Fri, 13 Jan 2023 17:17:17 +0530 Subject: [PATCH 31/58] fix: use OAUTHLIB_RELAX_TOKEN_SCOPE for ignoring scope change without this we get an error regarding the mismatch of scopes from microsoft --- frappe/integrations/doctype/connected_app/connected_app.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index ca9677d4da..4137e6b85b 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -14,6 +14,8 @@ if any((os.getenv("CI"), frappe.conf.developer_mode, frappe.conf.allow_tests)): # Disable mandatory TLS in developer mode and tests os.environ["OAUTHLIB_INSECURE_TRANSPORT"] = "1" +os.environ["OAUTHLIB_RELAX_TOKEN_SCOPE"] = "1" + class ConnectedApp(Document): """Connect to a remote oAuth Server. Retrieve and store user's access token From 88d4d5e10ddff41788523e8f4705d7410b171898 Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 15 Jan 2023 20:53:18 +0530 Subject: [PATCH 32/58] chore: minor cleanup - removed unnecessary branches and comments --- frappe/email/doctype/email_account/email_account.py | 11 +++-------- frappe/email/oauth.py | 3 +-- .../doctype/connected_app/connected_app.py | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 1e08ce5615..d20dd995ba 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -667,12 +667,9 @@ class EmailAccount(Document): self.log_error("Unable to add to Sent folder") def get_oauth_token(self): - token = None if self.auth_method == "OAuth": connected_app = frappe.get_doc("Connected App", self.connected_app) - token = connected_app.get_active_token(self.connected_user) - - return token + return connected_app.get_active_token(self.connected_user) @frappe.whitelist() @@ -769,14 +766,12 @@ def notify_unreplied(): def pull(now=False): """Will be called via scheduler, pull emails from all enabled Email accounts.""" + from frappe.integrations.doctype.connected_app.connected_app import has_token if frappe.cache().get_value("workers:no-internet") == True: if test_internet(): frappe.cache().set_value("workers:no-internet", False) - else: - return - - from frappe.integrations.doctype.connected_app.connected_app import has_token + return doctype = frappe.qb.DocType("Email Account") email_accounts = ( diff --git a/frappe/email/oauth.py b/frappe/email/oauth.py index 6b56047069..87feb8ca11 100644 --- a/frappe/email/oauth.py +++ b/frappe/email/oauth.py @@ -36,7 +36,6 @@ class Oauth: return f"user={self.email}\1auth=Bearer {self._access_token}\1\1" def connect(self) -> None: - """Connection method with retry on exception for connection errors""" try: if isinstance(self._conn, POP3): self._connect_pop() @@ -59,7 +58,7 @@ class Oauth: raise def _connect_pop(self) -> None: - # poplib doesn't have AUTH command implementation + # NOTE: poplib doesn't have AUTH command implementation res = self._conn._shortcmd( "AUTH {} {}".format( self._mechanism, base64.b64encode(bytes(self._auth_string, "utf-8")).decode("utf-8") diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index 4137e6b85b..96f23afc1c 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -167,4 +167,4 @@ def callback(code=None, state=None): def has_token(connected_app, connected_user=None): app = frappe.get_doc("Connected App", connected_app) token_cache = app.get_token_cache(connected_user or frappe.session.user) - return bool(token_cache.get_json()["access_token"]) + return bool(token_cache.get_password("access_token", False)) From b5c81cc0152bc7a365f0f2f82d41ab5803d9b1bf Mon Sep 17 00:00:00 2001 From: phot0n Date: Sun, 15 Jan 2023 21:01:03 +0530 Subject: [PATCH 33/58] chore(patch): disable all email accounts with oauth mechanism --- .../email/doctype/email_account/email_account.py | 2 +- frappe/patches.txt | 1 + .../v14_0/disable_email_accounts_with_oauth.py | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 frappe/patches/v14_0/disable_email_accounts_with_oauth.py diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index d20dd995ba..f754869938 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -911,7 +911,7 @@ def remove_user_email_inbox(email_account): @frappe.whitelist() def set_email_password(email_account, password): account = frappe.get_doc("Email Account", email_account) - if account.awaiting_password and not account.auth_method == "OAuth": + if account.awaiting_password and account.auth_method != "OAuth": account.awaiting_password = 0 account.password = password try: diff --git a/frappe/patches.txt b/frappe/patches.txt index cf1e509e78..b345e7f106 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -198,6 +198,7 @@ frappe.patches.v14_0.delete_payment_gateways frappe.patches.v15_0.remove_event_streaming frappe.patches.v15_0.remove_prepared_report_settings_from_system_settings frappe.patches.v15_0.copy_disable_prepared_report_to_prepared_report +frappe.patches.v14_0.disable_email_accounts_with_oauth [post_model_sync] frappe.patches.v14_0.drop_data_import_legacy diff --git a/frappe/patches/v14_0/disable_email_accounts_with_oauth.py b/frappe/patches/v14_0/disable_email_accounts_with_oauth.py new file mode 100644 index 0000000000..27c322c60a --- /dev/null +++ b/frappe/patches/v14_0/disable_email_accounts_with_oauth.py @@ -0,0 +1,15 @@ +import click + +import frappe + + +def execute(): + # Setting awaiting password to 1 for email accounts where Oauth is enabled. + # This is done so that people can resetup their email accounts with connected app mechanism. + doctype = frappe.qb.DocType("Email Account") + frappe.qb.update(doctype).set(doctype.awaiting_password, 1).where(doctype.auth_mehtod == "OAuth") + + click.secho( + "Email Accounts with auth method as OAuth have been disabled." + "Please re-setup your OAuth based email accounts with the connected app mechanism to re-enable them." + ) From d8b7bc18d7453cf37148b716ed013f25394dc157 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Thu, 18 Aug 2022 11:40:29 +0530 Subject: [PATCH 34/58] refactor!: deprecate sorting based on `apps.txt` in `get_installed_apps` --- frappe/__init__.py | 9 +++++++-- frappe/utils/change_log.py | 2 +- frappe/utils/jinja.py | 3 +-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index b7fd117868..23d40f08a9 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1400,7 +1400,12 @@ def get_all_apps(with_internal_apps=True, sites_path=None): @request_cache def get_installed_apps(sort=False, frappe_last=False): - """Get list of installed apps in current site.""" + """ + Get list of installed apps in current site. + + :param sort: [DEPRECATED] Sort installed apps based on the sequence in sites/apps.txt + """ + if getattr(flags, "in_install_db", True): return [] @@ -1445,7 +1450,7 @@ def _load_app_hooks(app_name: str | None = None): import types hooks = {} - apps = [app_name] if app_name else get_installed_apps(sort=True) + apps = [app_name] if app_name else get_installed_apps() for app in apps: try: diff --git a/frappe/utils/change_log.py b/frappe/utils/change_log.py index 12069cce09..6850a9bbf9 100644 --- a/frappe/utils/change_log.py +++ b/frappe/utils/change_log.py @@ -108,7 +108,7 @@ def get_versions(): } }""" versions = {} - for app in frappe.get_installed_apps(sort=True): + for app in frappe.get_installed_apps(): app_hooks = frappe.get_hooks(app_name=app) versions[app] = { "title": app_hooks.get("app_title")[0], diff --git a/frappe/utils/jinja.py b/frappe/utils/jinja.py index 33bb929bc4..0e5c03eff0 100644 --- a/frappe/utils/jinja.py +++ b/frappe/utils/jinja.py @@ -112,8 +112,7 @@ def get_jloader(): apps = frappe.get_hooks("template_apps") if not apps: - apps = frappe.local.flags.web_pages_apps or frappe.get_installed_apps(sort=True) - apps.reverse() + apps = list(reversed(frappe.local.flags.web_pages_apps or frappe.get_installed_apps())) if "frappe" not in apps: apps.append("frappe") From f5cbcec10342b9f15cd689a589cd6843b2172e11 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Thu, 18 Aug 2022 12:08:33 +0530 Subject: [PATCH 35/58] fix: defer `local.all_apps` loading --- frappe/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 23d40f08a9..e93b16a79c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1412,12 +1412,12 @@ def get_installed_apps(sort=False, frappe_last=False): if not db: connect() - if not local.all_apps: - local.all_apps = cache().get_value("all_apps", get_all_apps) - installed = json.loads(db.get_global("installed_apps") or "[]") if sort: + if not local.all_apps: + local.all_apps = cache().get_value("all_apps", get_all_apps) + installed = [app for app in local.all_apps if app in installed] if frappe_last: From 5e2bbf834f8eff7db340f7a760771bf628a61442 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 13 Jan 2023 13:50:01 +0530 Subject: [PATCH 36/58] refactor: filter out apps not installed on bench --- frappe/__init__.py | 13 +++++++++++-- frappe/tests/test_commands.py | 1 + frappe/utils/change_log.py | 2 +- frappe/utils/jinja.py | 4 +++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index e93b16a79c..b4b44925ef 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1399,12 +1399,15 @@ def get_all_apps(with_internal_apps=True, sites_path=None): @request_cache -def get_installed_apps(sort=False, frappe_last=False): +def get_installed_apps(sort=False, frappe_last=False, *, _ensure_on_bench=False): """ Get list of installed apps in current site. :param sort: [DEPRECATED] Sort installed apps based on the sequence in sites/apps.txt + :param frappe_last: [DEPRECATED] Keep frappe last. Do not use this, reverse the app list instead. + :param ensure_on_bench: Only return apps that are present on bench. """ + from frappe.utils.deprecations import deprecation_warning if getattr(flags, "in_install_db", True): return [] @@ -1418,9 +1421,15 @@ def get_installed_apps(sort=False, frappe_last=False): if not local.all_apps: local.all_apps = cache().get_value("all_apps", get_all_apps) + deprecation_warning("`sort` argument is deprecated and will be removed in v15.") installed = [app for app in local.all_apps if app in installed] + if _ensure_on_bench: + all_apps = cache().get_value("all_apps", get_all_apps) + installed = [app for app in installed if app in all_apps] + if frappe_last: + deprecation_warning("`frappe_last` argument is deprecated and will be removed in v15.") if "frappe" in installed: installed.remove("frappe") installed.append("frappe") @@ -1450,7 +1459,7 @@ def _load_app_hooks(app_name: str | None = None): import types hooks = {} - apps = [app_name] if app_name else get_installed_apps() + apps = [app_name] if app_name else get_installed_apps(_ensure_on_bench=True) for app in apps: try: diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 7af66f6c62..bbf51de884 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -300,6 +300,7 @@ class TestCommands(BaseTestCommands): frappe.local.cache = {} self.assertEqual(frappe.recorder.status(), False) + @unittest.skip("Poorly written, relied on app name being absent in apps.txt") def test_remove_from_installed_apps(self): app = "test_remove_app" add_to_installed_apps(app) diff --git a/frappe/utils/change_log.py b/frappe/utils/change_log.py index 6850a9bbf9..55534614e6 100644 --- a/frappe/utils/change_log.py +++ b/frappe/utils/change_log.py @@ -108,7 +108,7 @@ def get_versions(): } }""" versions = {} - for app in frappe.get_installed_apps(): + for app in frappe.get_installed_apps(_ensure_on_bench=True): app_hooks = frappe.get_hooks(app_name=app) versions[app] = { "title": app_hooks.get("app_title")[0], diff --git a/frappe/utils/jinja.py b/frappe/utils/jinja.py index 0e5c03eff0..d92df18c70 100644 --- a/frappe/utils/jinja.py +++ b/frappe/utils/jinja.py @@ -112,7 +112,9 @@ def get_jloader(): apps = frappe.get_hooks("template_apps") if not apps: - apps = list(reversed(frappe.local.flags.web_pages_apps or frappe.get_installed_apps())) + apps = list( + reversed(frappe.local.flags.web_pages_apps or frappe.get_installed_apps(_ensure_on_bench=True)) + ) if "frappe" not in apps: apps.append("frappe") From 52e3d8d58b7fd62619bcd689b3599d501702cf57 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 16 Jan 2023 14:11:37 +0530 Subject: [PATCH 37/58] fix: handle empty string passed to filters --- frappe/database/query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index ea5f2b8760..072f1eed4f 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -98,7 +98,7 @@ class Engine: self, filters: dict[str, str | int] | str | int | list[list | str | int] | None = None, ): - if not filters: + if filters is None: return if isinstance(filters, (str, int)): From 5bc5ff100bffc0210ddddab8c8513b6e8aa02dd6 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 16 Jan 2023 14:12:04 +0530 Subject: [PATCH 38/58] test: tests for various filter options --- frappe/tests/test_query.py | 41 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 12cb6446d2..42d3670f2d 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -258,6 +258,47 @@ class TestQuery(FrappeTestCase): ), ) + self.assertEqual( + frappe.qb.get_query( + "DocType", + fields=["module"], + filters="", + ).get_sql(), + "SELECT `module` FROM `tabDocType` WHERE `name`=''".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + + self.assertEqual( + frappe.qb.get_query( + "DocType", + filters=["ToDo", "Note"], + ).get_sql(), + "SELECT `name` FROM `tabDocType` WHERE `name` IN ('ToDo','Note')".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + + self.assertEqual( + frappe.qb.get_query( + "DocType", + filters={"name": ("in", [])}, + ).get_sql(), + "SELECT `name` FROM `tabDocType` WHERE `name` IN ('')".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + + self.assertEqual( + frappe.qb.get_query( + "DocType", + filters=[1, 2, 3], + ).get_sql(), + "SELECT `name` FROM `tabDocType` WHERE `name` IN (1,2,3)".replace( + "`", '"' if frappe.db.db_type == "postgres" else "`" + ), + ) + def test_implicit_join_query(self): self.maxDiff = None From 543458b473491db2e37683044ca8d01abe41290b Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 16 Jan 2023 15:38:15 +0530 Subject: [PATCH 39/58] fix: handle empty list as filters --- frappe/database/query.py | 2 +- frappe/tests/test_query.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 072f1eed4f..10423f9ca4 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -111,7 +111,7 @@ class Engine: self.apply_dict_filters(filters) elif isinstance(filters, (list, tuple)): - if all(isinstance(d, (str, int)) for d in filters): + if all(isinstance(d, (str, int)) for d in filters) and len(filters) > 0: self.apply_dict_filters({"name": ("in", filters)}) else: for filter in filters: diff --git a/frappe/tests/test_query.py b/frappe/tests/test_query.py index 42d3670f2d..82218e5952 100644 --- a/frappe/tests/test_query.py +++ b/frappe/tests/test_query.py @@ -299,6 +299,14 @@ class TestQuery(FrappeTestCase): ), ) + self.assertEqual( + frappe.qb.get_query( + "DocType", + filters=[], + ).get_sql(), + "SELECT `name` FROM `tabDocType`".replace("`", '"' if frappe.db.db_type == "postgres" else "`"), + ) + def test_implicit_join_query(self): self.maxDiff = None From 1e6086fd757c3ff1a7d8b59b48a6b79e283ef02e Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Mon, 16 Jan 2023 16:22:38 +0530 Subject: [PATCH 40/58] fix(minor): only show authorization message if connected app is set (#19605) --- frappe/email/doctype/email_account/email_account.js | 2 +- frappe/integrations/doctype/connected_app/connected_app.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/email/doctype/email_account/email_account.js b/frappe/email/doctype/email_account/email_account.js index bc3d168639..90f71bf88f 100644 --- a/frappe/email/doctype/email_account/email_account.js +++ b/frappe/email/doctype/email_account/email_account.js @@ -166,7 +166,7 @@ frappe.ui.form.on("Email Account", { }, show_oauth_authorization_message(frm) { - if (frm.doc.auth_method === "OAuth") { + if (frm.doc.auth_method === "OAuth" && frm.doc.connected_app) { frappe.call({ method: "frappe.integrations.doctype.connected_app.connected_app.has_token", args: { diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index 96f23afc1c..536b63fe7b 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -167,4 +167,4 @@ def callback(code=None, state=None): def has_token(connected_app, connected_user=None): app = frappe.get_doc("Connected App", connected_app) token_cache = app.get_token_cache(connected_user or frappe.session.user) - return bool(token_cache.get_password("access_token", False)) + return bool(token_cache and token_cache.get_password("access_token", False)) From 0f5d17663d995986e74bb3145b4c76d73b70664b Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Mon, 16 Jan 2023 16:44:04 +0530 Subject: [PATCH 41/58] fix: Remove setup wizard user image (#19601) --- frappe/desk/page/setup_wizard/setup_wizard.js | 40 ++++++------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/frappe/desk/page/setup_wizard/setup_wizard.js b/frappe/desk/page/setup_wizard/setup_wizard.js index 1cfceb29b0..969aedb882 100644 --- a/frappe/desk/page/setup_wizard/setup_wizard.js +++ b/frappe/desk/page/setup_wizard/setup_wizard.js @@ -244,7 +244,7 @@ frappe.setup.SetupWizard = class SetupWizard extends frappe.ui.Slides { } get_setup_slides_filtered_by_domain() { - var filtered_slides = []; + let filtered_slides = []; frappe.setup.slides.forEach(function (slide) { if (frappe.setup.domains) { let active_domains = frappe.setup.domains; @@ -329,7 +329,7 @@ frappe.setup.SetupWizardSlide = class SetupWizardSlide extends frappe.ui.Slide { } set_init_values() { - var me = this; + let me = this; // set values from frappe.setup.values if (frappe.wizard.values && this.fields) { this.fields.forEach(function (f) { @@ -348,7 +348,7 @@ frappe.setup.slides_settings = [ { // Welcome (language) slide name: "welcome", - title: __("Hello!"), + title: __("Welcome"), fields: [ { @@ -418,16 +418,9 @@ frappe.setup.slides_settings = [ { // Profile slide name: "user", - title: __("The First User: You"), + title: __("Let's setup your account"), icon: "fa fa-user", fields: [ - { - fieldtype: "Attach Image", - fieldname: "attach_user_image", - label: __("Attach Your Picture"), - is_private: 0, - align: "center", - }, { fieldname: "full_name", label: __("Full Name"), @@ -456,15 +449,6 @@ frappe.setup.slides_settings = [ [frappe.boot.user.first_name, frappe.boot.user.last_name].join(" ").trim() ); } - - var user_image = frappe.get_cookie("user_image"); - var $attach_user_image = slide.form.fields_dict.attach_user_image.$wrapper; - - if (user_image) { - $attach_user_image.find(".missing-image").toggle(false); - $attach_user_image.find("img").attr("src", decodeURIComponent(user_image)); - $attach_user_image.find(".img-container").toggle(true); - } delete slide.form.fields_dict.email; } else { slide.form.fields_dict.email.df.reqd = 1; @@ -484,7 +468,7 @@ frappe.setup.slides_settings = [ let email = frappe.setup.data.email; slide.form.fields_dict.email.set_input(email); if (frappe.get_gravatar(email, 200)) { - var $attach_user_image = slide.form.fields_dict.attach_user_image.$wrapper; + let $attach_user_image = slide.form.fields_dict.attach_user_image.$wrapper; $attach_user_image.find(".missing-image").toggle(false); $attach_user_image.find("img").attr("src", frappe.get_gravatar(email, 200)); $attach_user_image.find(".img-container").toggle(true); @@ -569,7 +553,7 @@ frappe.setup.utils = { .on("change", function () { clearTimeout(slide.language_call_timeout); slide.language_call_timeout = setTimeout(() => { - var lang = $(this).val() || "English"; + let lang = $(this).val() || "English"; frappe._messages = {}; frappe.call({ method: "frappe.desk.page.setup_wizard.setup_wizard.load_messages", @@ -595,9 +579,9 @@ frappe.setup.utils = { Bind a slide's country, timezone and currency fields */ slide.get_input("country").on("change", function () { - var country = slide.get_input("country").val(); - var $timezone = slide.get_input("timezone"); - var data = frappe.setup.data.regional_data; + let country = slide.get_input("country").val(); + let $timezone = slide.get_input("timezone"); + let data = frappe.setup.data.regional_data; $timezone.empty(); @@ -618,12 +602,12 @@ frappe.setup.utils = { }); slide.get_input("currency").on("change", function () { - var currency = slide.get_input("currency").val(); + let currency = slide.get_input("currency").val(); if (!currency) return; frappe.model.with_doc("Currency", currency, function () { frappe.provide("locals.:Currency." + currency); - var currency_doc = frappe.model.get_doc("Currency", currency); - var number_format = currency_doc.number_format; + let currency_doc = frappe.model.get_doc("Currency", currency); + let number_format = currency_doc.number_format; if (number_format === "#.###") { number_format = "#.###,##"; } else if (number_format === "#,###") { From 8adfdcbc1d7c9400a157b85e14015cfe7a11e72e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 16 Jan 2023 17:31:39 +0530 Subject: [PATCH 42/58] tests: clear DB transactions before all db calls Because of repeatable read isolation, changes from externally executed command dont reflect until transaction is ended. --- frappe/tests/test_commands.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index bbf51de884..66c78d825c 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -405,23 +405,26 @@ class TestCommands(BaseTestCommands): def test_set_password(self): from frappe.utils.password import check_password + self.assertEqual(check_password("Administrator", "am"), "Administrator") self.execute("bench --site {site} set-password Administrator test1") self.assertEqual(self.returncode, 0) self.assertEqual(check_password("Administrator", "test1"), "Administrator") # to release the lock taken by check_password - frappe.db.commit() + frappe.db.rollback() self.execute("bench --site {site} set-admin-password test2") self.assertEqual(self.returncode, 0) + frappe.db.rollback() self.assertEqual(check_password("Administrator", "test2"), "Administrator") - frappe.db.commit() + frappe.db.rollback() # Reset it back to original password original_password = frappe.conf.admin_password or "admin" self.execute("bench --site {site} set-admin-password %s" % original_password) self.assertEqual(self.returncode, 0) + frappe.db.rollback() self.assertEqual(check_password("Administrator", original_password), "Administrator") - frappe.db.commit() + frappe.db.rollback() @skipIf( not ( From c7edd7e57c712020151c355a105e9d41360be27c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 16 Jan 2023 18:00:10 +0530 Subject: [PATCH 43/58] chore: remove unnecessary assertion --- frappe/tests/test_commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 66c78d825c..83901fd9c4 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -405,9 +405,9 @@ class TestCommands(BaseTestCommands): def test_set_password(self): from frappe.utils.password import check_password - self.assertEqual(check_password("Administrator", "am"), "Administrator") self.execute("bench --site {site} set-password Administrator test1") self.assertEqual(self.returncode, 0) + frappe.db.rollback() self.assertEqual(check_password("Administrator", "test1"), "Administrator") # to release the lock taken by check_password frappe.db.rollback() From 433115f62d6c7a62b57ae74faebf9d3c92c4ec52 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 16 Jan 2023 18:43:52 +0530 Subject: [PATCH 44/58] test: rollback test transaction after executing cmd (#19606) In command tests if connection is active then due to repeatable read isolation you will continue to read old data which might be modified by the command you're trying to test. It makes sense to end transaction after each command execution here. [skip ci] --- frappe/tests/test_commands.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index 83901fd9c4..f8f3921440 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -143,6 +143,9 @@ class BaseTestCommands(FrappeTestCase): @classmethod def execute(self, command, kwargs=None): + # tests might have written to DB which wont be visible to commands until we end current transaction + frappe.db.commit() + site = {"site": frappe.local.site} cmd_input = None if kwargs: @@ -165,6 +168,9 @@ class BaseTestCommands(FrappeTestCase): self.stderr = clean(self._proc.stderr) self.returncode = clean(self._proc.returncode) + # Commands might have written to DB which wont be visible until we end current transaction + frappe.db.rollback() + @classmethod def setup_test_site(cls): cmd_config = { @@ -407,24 +413,17 @@ class TestCommands(BaseTestCommands): self.execute("bench --site {site} set-password Administrator test1") self.assertEqual(self.returncode, 0) - frappe.db.rollback() self.assertEqual(check_password("Administrator", "test1"), "Administrator") - # to release the lock taken by check_password - frappe.db.rollback() self.execute("bench --site {site} set-admin-password test2") self.assertEqual(self.returncode, 0) - frappe.db.rollback() self.assertEqual(check_password("Administrator", "test2"), "Administrator") - frappe.db.rollback() # Reset it back to original password original_password = frappe.conf.admin_password or "admin" self.execute("bench --site {site} set-admin-password %s" % original_password) self.assertEqual(self.returncode, 0) - frappe.db.rollback() self.assertEqual(check_password("Administrator", original_password), "Administrator") - frappe.db.rollback() @skipIf( not ( From 75ae0fa2483688a2417f5ec4df3ccf2b96220312 Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Tue, 17 Jan 2023 10:50:07 +0530 Subject: [PATCH 45/58] chore: remove unnecessary query condition from get_other_system_managers (#19611) --- frappe/core/doctype/user/user.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 7ba35f5181..a7e5cf7669 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -305,12 +305,10 @@ class User(Document): .from_(user_role_doctype) .select(user_doctype.name) .where(user_role_doctype.role == "System Manager") - .where(user_doctype.docstatus < 2) .where(user_doctype.enabled == 1) .where(user_role_doctype.parent == user_doctype.name) .where(user_role_doctype.parent.notin(["Administrator", self.name])) .limit(1) - .distinct() ).run() def get_fullname(self): From 53f81549078e3bdc3fcb5d29109eabc4457fd42c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 17 Jan 2023 10:55:54 +0530 Subject: [PATCH 46/58] chore(flake8): ignore B028 (#19612) [skip ci] --- .flake8 | 1 + 1 file changed, 1 insertion(+) diff --git a/.flake8 b/.flake8 index 2de7a154c9..e783fbbeb3 100644 --- a/.flake8 +++ b/.flake8 @@ -69,6 +69,7 @@ ignore = F841, E713, E712, + B028, max-line-length = 200 exclude=,test_*.py From 647ac83abf8d63c377c0c554f4eefeb4a58fd435 Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Tue, 17 Jan 2023 11:44:04 +0530 Subject: [PATCH 47/58] chore(patch): send notification to system managers for resetup of oauth enabled email accounts (#19610) --- .../disable_email_accounts_with_oauth.py | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/frappe/patches/v14_0/disable_email_accounts_with_oauth.py b/frappe/patches/v14_0/disable_email_accounts_with_oauth.py index 27c322c60a..d620bf4e3b 100644 --- a/frappe/patches/v14_0/disable_email_accounts_with_oauth.py +++ b/frappe/patches/v14_0/disable_email_accounts_with_oauth.py @@ -1,15 +1,36 @@ -import click - import frappe +from frappe.desk.doctype.notification_log.notification_log import make_notification_logs def execute(): + if not frappe.get_value("Email Account", {"auth_method": "OAuth"}): + return + # Setting awaiting password to 1 for email accounts where Oauth is enabled. # This is done so that people can resetup their email accounts with connected app mechanism. - doctype = frappe.qb.DocType("Email Account") - frappe.qb.update(doctype).set(doctype.awaiting_password, 1).where(doctype.auth_mehtod == "OAuth") + frappe.db.set_value("Email Account", {"auth_method": "OAuth"}, "awaiting_password", 1) - click.secho( - "Email Accounts with auth method as OAuth have been disabled." - "Please re-setup your OAuth based email accounts with the connected app mechanism to re-enable them." - ) + message = "Email Accounts with auth method as OAuth have been disabled.\ + Please re-setup your OAuth based email accounts with the connected app mechanism to re-enable them." + + if sysmanagers := get_system_managers(): + make_notification_logs( + { + "type": "Alert", + "subject": frappe._(message), + }, + sysmanagers, + ) + + +def get_system_managers(): + user_doctype = frappe.qb.DocType("User").as_("user") + user_role_doctype = frappe.qb.DocType("Has Role").as_("user_role") + return ( + frappe.qb.from_(user_doctype) + .from_(user_role_doctype) + .select(user_doctype.email) + .where(user_role_doctype.role == "System Manager") + .where(user_doctype.enabled == 1) + .where(user_role_doctype.parent == user_doctype.name) + ).run(pluck=True) From 4bb5b10c7f32c37d913a625cca62f9c8982d7c1f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 17 Jan 2023 12:06:39 +0530 Subject: [PATCH 48/58] fix(DX): better error msg for non-whitelisted methods (#19616) [skip ci] --- frappe/__init__.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index b4b44925ef..fae3b31c4a 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -770,7 +770,12 @@ def is_whitelisted(method): is_guest = session["user"] == "Guest" if method not in whitelisted or is_guest and method not in guest_methods: - throw(_("Not permitted"), PermissionError) + summary = _("You are not permitted to access this resource.") + detail = _("Function {0} is not whitelisted.").format( + bold(f"{method.__module__}.{method.__name__}") + ) + msg = f"
{summary}{detail}
" + throw(msg, PermissionError, title="Method Not Allowed") if is_guest and method not in xss_safe_methods: # strictly sanitize form_dict From ee17b221103907bcdfd194d48ba8fbbfea40c1c7 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 17 Jan 2023 14:04:31 +0530 Subject: [PATCH 49/58] fix: only replace "tab" at the beginning --- frappe/database/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/utils.py b/frappe/database/utils.py index f89ba3c737..304fd72be6 100644 --- a/frappe/database/utils.py +++ b/frappe/database/utils.py @@ -35,8 +35,8 @@ def is_pypika_function_object(field: str) -> bool: def get_doctype_name(table_name: str) -> str: - if "tab" in table_name: - table_name = table_name.replace("tab", "") + if table_name.startswith(("tab", "`tab", '"tab')): + table_name = table_name.replace("tab", "", 1) table_name = table_name.replace("`", "") table_name = table_name.replace('"', "") return table_name From 6b1e98e0d66555ae790764b850034b7321fae062 Mon Sep 17 00:00:00 2001 From: Ritwik Puri Date: Tue, 17 Jan 2023 14:19:33 +0530 Subject: [PATCH 50/58] chore: reorder disable_email_accounts_with_oauth to post model sync (#19620) --- frappe/patches.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/patches.txt b/frappe/patches.txt index b345e7f106..7f7ab9bfe2 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -198,7 +198,6 @@ frappe.patches.v14_0.delete_payment_gateways frappe.patches.v15_0.remove_event_streaming frappe.patches.v15_0.remove_prepared_report_settings_from_system_settings frappe.patches.v15_0.copy_disable_prepared_report_to_prepared_report -frappe.patches.v14_0.disable_email_accounts_with_oauth [post_model_sync] frappe.patches.v14_0.drop_data_import_legacy @@ -221,3 +220,4 @@ frappe.patches.v14_0.add_manage_subscriptions_in_navbar_settings frappe.patches.v14_0.update_attachment_comment frappe.patches.v15_0.set_contact_full_name execute:frappe.delete_doc("Page", "activity", force=1) +frappe.patches.v14_0.disable_email_accounts_with_oauth From 551f58bde9efdad6895957e0d73e1b385b723964 Mon Sep 17 00:00:00 2001 From: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Date: Tue, 17 Jan 2023 16:32:44 +0530 Subject: [PATCH 51/58] fix: Convert extension to lowercase before comparison --- frappe/core/doctype/file/file.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/file/file.js b/frappe/core/doctype/file/file.js index 4fd092a00b..159cf1ce39 100644 --- a/frappe/core/doctype/file/file.js +++ b/frappe/core/doctype/file/file.js @@ -24,6 +24,8 @@ frappe.ui.form.on("File", { preview_file: function (frm) { let $preview = ""; + let file_name = frm.doc.file_name.split("?")[0]; + let file_extension = file_name.split(".").pop()?.toLowerCase(); if (frappe.utils.is_image_file(frm.doc.file_url)) { $preview = $(`
@@ -40,7 +42,7 @@ frappe.ui.form.on("File", { ${__("Your browser does not support the video element.")}
`); - } else if (frm.doc.file_name.split("?")[0].endsWith(".pdf")) { + } else if (file_extension === "pdf") { $preview = $(`
`); - } else if (frm.doc.file_name.split("?")[0].endsWith(".mp3")) { + } else if (file_extension === "mp3") { $preview = $(`