diff --git a/frappe/__init__.py b/frappe/__init__.py index 844a9238e3..d20e856177 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1378,7 +1378,7 @@ def get_list(doctype, *args, **kwargs): frappe.get_list("ToDo", fields="*", filters = {"description": ("like", "test%")}) """ import frappe.model.db_query - return frappe.model.db_query.DatabaseQuery(doctype).execute(None, *args, **kwargs) + return frappe.model.db_query.DatabaseQuery(doctype).execute(*args, **kwargs) def get_all(doctype, *args, **kwargs): """List database query via `frappe.model.db_query`. Will **not** check for permissions. diff --git a/frappe/client.py b/frappe/client.py index 2217b53673..156c31e554 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -8,6 +8,8 @@ import frappe.model import frappe.utils import json, os from frappe.utils import get_safe_filters +from frappe.desk.reportview import validate_args +from frappe.model.db_query import check_parent_permission from six import iteritems, string_types, integer_types @@ -31,8 +33,18 @@ def get_list(doctype, fields=None, filters=None, order_by=None, if frappe.is_table(doctype): check_parent_permission(parent, doctype) - return frappe.get_list(doctype, fields=fields, filters=filters, order_by=order_by, - limit_start=limit_start, limit_page_length=limit_page_length, ignore_permissions=False) + args = frappe._dict( + doctype=doctype, + fields=fields, + filters=filters, + order_by=order_by, + limit_start=limit_start, + limit_page_length=limit_page_length, + ) + + validate_args(args) + + return frappe.get_list(**args) @frappe.whitelist() def get_count(doctype, filters=None, debug=False, cache=False): @@ -91,12 +103,12 @@ def get_value(doctype, fieldname, filters=None, as_dict=True, debug=False, paren if frappe.get_meta(doctype).issingle: value = frappe.db.get_values_from_single(fields, filters, doctype, as_dict=as_dict, debug=debug) else: - value = frappe.get_list(doctype, filters=filters, fields=fields, debug=debug, limit=1) + value = get_list(doctype, filters=filters, fields=fields, limit_page_length=1) if as_dict: value = value[0] if value else {} else: - value = value[0].fieldname + value = value[0][fieldname] return value @@ -378,18 +390,6 @@ def attach_file(filename=None, filedata=None, doctype=None, docname=None, folder def get_hooks(hook, app_name=None): return frappe.get_hooks(hook, app_name) -def check_parent_permission(parent, child_doctype): - if parent: - # User may pass fake parent and get the information from the child table - if child_doctype and not frappe.db.exists('DocField', - {'parent': parent, 'options': child_doctype}): - raise frappe.PermissionError - - if frappe.permissions.has_permission(parent): - return - # Either parent not passed or the user doesn't have permission on parent doctype of child table! - raise frappe.PermissionError - @frappe.whitelist() def is_document_amended(doctype, docname): if frappe.permissions.has_permission(doctype): @@ -400,4 +400,4 @@ def is_document_amended(doctype, docname): except frappe.db.InternalError: pass - return False \ No newline at end of file + return False diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 3003385601..296dfb94f1 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -8,6 +8,7 @@ import frappe, json from six.moves import range import frappe.permissions from frappe.model.db_query import DatabaseQuery +from frappe.model import default_fields, optional_fields from frappe import _ from six import string_types, StringIO from frappe.core.doctype.access_log.access_log import make_access_log @@ -18,10 +19,20 @@ from frappe.utils import cstr, format_duration @frappe.read_only() def get(): args = get_form_params() + return compress(execute(**args), args=args) - data = compress(execute(**args), args = args) +@frappe.whitelist() +@frappe.read_only() +def get_list(): + # uncompressed (refactored from frappe.model.db_query.get_list) + return execute(**get_form_params()) - return data +@frappe.whitelist() +@frappe.read_only() +def get_count(): + args = get_form_params() + args.fields = ['{distinct}count(name) as total_count'.format(distinct = 'distinct ' if args.distinct=='true' else '')] + return execute(**args)[0].get('total_count') def execute(doctype, *args, **kwargs): return DatabaseQuery(doctype).execute(*args, **kwargs) @@ -29,9 +40,136 @@ def execute(doctype, *args, **kwargs): def get_form_params(): """Stringify GET request parameters.""" data = frappe._dict(frappe.local.form_dict) + clean_params(data) + validate_args(data) + return data - is_report = data.get('view') == 'Report' +def validate_args(data): + parse_json(data) + setup_group_by(data) + validate_fields(data) + if data.filters: + validate_filters(data, data.filters) + if data.or_filters: + validate_filters(data, data.or_filters) + + data.strict = None + + return data + +def validate_fields(data): + wildcard = update_wildcard_field_param(data) + + for field in data.fields or []: + fieldname = extract_fieldname(field) + if is_standard(fieldname): + continue + + meta, df = get_meta_and_docfield(fieldname, data) + + if not df: + if wildcard: + continue + else: + raise_invalid_field(fieldname) + + # remove the field from the query if the report hide flag is set and current view is Report + if df.report_hide and data.view == 'Report': + data.fields.remove(field) + continue + + if df.fieldname in [_df.fieldname for _df in meta.get_high_permlevel_fields()]: + if df.get('permlevel') not in meta.get_permlevel_access(parenttype=data.doctype): + data.fields.remove(field) + +def validate_filters(data, filters): + if isinstance(filters, list): + # filters as list + for condition in filters: + if len(condition)==3: + # [fieldname, condition, value] + fieldname = condition[0] + if is_standard(fieldname): + continue + meta, df = get_meta_and_docfield(fieldname, data) + if not df: + raise_invalid_field(condition[0]) + else: + # [doctype, fieldname, condition, value] + fieldname = condition[1] + if is_standard(fieldname): + continue + meta = frappe.get_meta(condition[0]) + if not meta.get_field(fieldname): + raise_invalid_field(fieldname) + + else: + for fieldname in filters: + if is_standard(fieldname): + continue + meta, df = get_meta_and_docfield(fieldname, data) + if not df: + raise_invalid_field(fieldname) + +def setup_group_by(data): + '''Add columns for aggregated values e.g. count(name)''' + if data.group_by: + if data.aggregate_function.lower() not in ('count', 'sum', 'avg'): + frappe.throw(_('Invalid aggregate function')) + if '`' in data.aggregate_on: + raise_invalid_field(data.aggregate_on) + data.fields.append('{aggregate_function}(`tab{doctype}`.`{aggregate_on}`) AS _aggregate_column'.format(**data)) + if data.aggregate_on: + data.fields.append(data.aggregate_on) + + data.pop('aggregate_on') + data.pop('aggregate_function') + +def raise_invalid_field(fieldname): + frappe.throw(_('Field not permitted in query') + ': {0}'.format(fieldname), frappe.DataError) + +def is_standard(fieldname): + if '.' in fieldname: + parenttype, fieldname = get_parenttype_and_fieldname(fieldname, None) + return fieldname in default_fields or fieldname in optional_fields + +def extract_fieldname(field): + for text in (',', '/*', '#'): + if text in field: + raise_invalid_field(field) + + fieldname = field + for sep in (' as ', ' AS '): + if sep in fieldname: + fieldname = fieldname.split(sep)[0] + + # certain functions allowed, extract the fieldname from the function + if (fieldname.startswith('count(') + or fieldname.startswith('sum(') + or fieldname.startswith('avg(')): + if not fieldname.strip().endswith(')'): + raise_invalid_field(field) + fieldname = fieldname.split('(', 1)[1][:-1] + + return fieldname + +def get_meta_and_docfield(fieldname, data): + parenttype, fieldname = get_parenttype_and_fieldname(fieldname, data) + meta = frappe.get_meta(parenttype) + df = meta.get_field(fieldname) + return meta, df + +def update_wildcard_field_param(data): + if ((isinstance(data.fields, string_types) and data.fields == "*") + or (isinstance(data.fields, (list, tuple)) and len(data.fields) == 1 and data.fields[0] == "*")): + data.fields = frappe.db.get_table_columns(data.doctype) + return True + + return False + + +def clean_params(data): data.pop('cmd', None) data.pop('data', None) data.pop('ignore_permissions', None) @@ -41,8 +179,12 @@ def get_form_params(): if "csrf_token" in data: del data["csrf_token"] + +def parse_json(data): if isinstance(data.get("filters"), string_types): data["filters"] = json.loads(data["filters"]) + if isinstance(data.get("or_filters"), string_types): + data["or_filters"] = json.loads(data["or_filters"]) if isinstance(data.get("fields"), string_types): data["fields"] = json.loads(data["fields"]) if isinstance(data.get("docstatus"), string_types): @@ -52,47 +194,8 @@ def get_form_params(): else: data["save_user_settings"] = True - fields = data["fields"] - if ((isinstance(fields, string_types) and fields == "*") - or (isinstance(fields, (list, tuple)) and len(fields) == 1 and fields[0] == "*")): - parenttype = data.doctype - data["fields"] = frappe.db.get_table_columns(parenttype) - fields = data["fields"] - - for field in fields: - key = field.split(" as ")[0] - - if key.startswith('count('): continue - if key.startswith('sum('): continue - if key.startswith('avg('): continue - - parenttype, fieldname = get_parent_dt_and_field(key, data) - - if fieldname == "*": - # * inside list is not allowed with other fields - fields.remove(field) - - meta = frappe.get_meta(parenttype) - df = meta.get_field(fieldname) - - report_hide = df.report_hide if df else None - - # remove the field from the query if the report hide flag is set and current view is Report - if report_hide and is_report: - fields.remove(field) - - if df and fieldname in [df.fieldname for df in meta.get_high_permlevel_fields()]: - if df.get('permlevel') not in meta.get_permlevel_access(parenttype=data.doctype) and field in fields: - fields.remove(field) - - # queries must always be server side - data.query = None - data.strict = None - - return data - -def get_parent_dt_and_field(field, data): +def get_parenttype_and_fieldname(field, data): if "." in field: parenttype, fieldname = field.split(".")[0][4:-1], field.split(".")[1].strip("`") else: @@ -101,7 +204,6 @@ def get_parent_dt_and_field(field, data): return parenttype, fieldname - def compress(data, args = {}): """separate keys and values""" from frappe.desk.query_report import add_total_row diff --git a/frappe/frappeclient.py b/frappe/frappeclient.py index 919c334e51..054a8c9369 100644 --- a/frappe/frappeclient.py +++ b/frappe/frappeclient.py @@ -86,7 +86,7 @@ class FrappeClient(object): 'cmd': 'logout', }, verify=self.verify, headers=self.headers) - def get_list(self, doctype, fields='"*"', filters=None, limit_start=0, limit_page_length=0): + def get_list(self, doctype, fields='["name"]', filters=None, limit_start=0, limit_page_length=0): """Returns list of records of a particular type""" if not isinstance(fields, string_types): fields = json.dumps(fields) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 8eac75eb65..b29e143759 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -14,7 +14,6 @@ import frappe.permissions from datetime import datetime import frappe, json, copy, re from frappe.model import optional_fields -from frappe.client import check_parent_permission from frappe.model.utils.user_settings import get_user_settings, update_user_settings from frappe.utils import flt, cint, get_time, make_filter_tuple, get_filter, add_to_date, cstr, get_timespan_date_range from frappe.model.meta import get_table_columns @@ -32,7 +31,7 @@ class DatabaseQuery(object): self.flags = frappe._dict() self.reference_doctype = None - def execute(self, query=None, fields=None, filters=None, or_filters=None, + def execute(self, fields=None, filters=None, or_filters=None, docstatus=None, group_by=None, order_by=None, limit_start=False, limit_page_length=None, as_list=False, with_childnames=False, debug=False, ignore_permissions=False, user=None, with_comment_count=False, @@ -104,12 +103,9 @@ class DatabaseQuery(object): # no table & ignore_ddl, return if not self.columns: return [] - if query: - result = self.run_custom_query(query) - else: - result = self.build_and_run() - if return_query: - return result + result = self.build_and_run() + if return_query: + return result if with_comment_count and not as_list and self.doctype: self.add_comment_count(result) @@ -707,12 +703,6 @@ class DatabaseQuery(object): return " and ".join(conditions) if conditions else "" - - def run_custom_query(self, query): - if '%(key)s' in query: - query = query.replace('%(key)s', '`name`') - return frappe.db.sql(query, as_dict = (not self.as_list)) - def set_order_by(self, args): meta = frappe.get_meta(self.doctype) @@ -754,7 +744,7 @@ class DatabaseQuery(object): return _lower = parameters.lower() - if 'select' in _lower and ' from ' in _lower: + if 'select' in _lower and 'from' in _lower: frappe.throw(_('Cannot use sub-query in order by')) if re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*").match(_lower): @@ -795,6 +785,18 @@ class DatabaseQuery(object): update_user_settings(self.doctype, user_settings) +def check_parent_permission(parent, child_doctype): + if parent: + # User may pass fake parent and get the information from the child table + if child_doctype and not frappe.db.exists('DocField', + {'parent': parent, 'options': child_doctype}): + raise frappe.PermissionError + + if frappe.permissions.has_permission(parent): + return + # Either parent not passed or the user doesn't have permission on parent doctype of child table! + raise frappe.PermissionError + def get_order_by(doctype, meta): order_by = "" @@ -819,30 +821,6 @@ def get_order_by(doctype, meta): return order_by - -@frappe.whitelist() -def get_list(doctype, *args, **kwargs): - '''wrapper for DatabaseQuery''' - kwargs.pop('cmd', None) - kwargs.pop('ignore_permissions', None) - kwargs.pop('data', None) - kwargs.pop('strict', None) - kwargs.pop('user', None) - - # If doctype is child table - if frappe.is_table(doctype): - # Example frappe.db.get_list('Purchase Receipt Item', {'parent': 'Purchase Receipt'}) - # Here purchase receipt is the parent doctype of the child doctype Purchase Receipt Item - - if not kwargs.get('parent'): - frappe.flags.error_message = _('Parent is required to get child table data') - raise frappe.PermissionError(doctype) - - check_parent_permission(kwargs.get('parent'), doctype) - del kwargs['parent'] - - return DatabaseQuery(doctype).execute(None, *args, **kwargs) - def is_parent_only_filter(doctype, filters): #check if filters contains only parent doctype only_parent_doctype = True diff --git a/frappe/public/js/frappe/db.js b/frappe/public/js/frappe/db.js index cf716c67e5..6073c7d3f0 100644 --- a/frappe/public/js/frappe/db.js +++ b/frappe/public/js/frappe/db.js @@ -15,7 +15,7 @@ frappe.db = { } return new Promise ((resolve) => { frappe.call({ - method: 'frappe.model.db_query.get_list', + method: 'frappe.desk.reportview.get_list', args: args, type: 'GET', callback: function(r) { @@ -92,25 +92,25 @@ frappe.db = { }, count: function(doctype, args={}) { let filters = args.filters || {}; - const with_child_table_filter = Array.isArray(filters) && filters.some(filter => { + + // has a filter with childtable? + const distinct = Array.isArray(filters) && filters.some(filter => { return filter[0] !== doctype; }); - const fields = [ - // cannot break this line as it adds extra \n's and \t's which breaks the query - `count(${with_child_table_filter ? 'distinct': ''} ${frappe.model.get_full_column_name('name', doctype)}) AS total_count` - ]; + const fields = []; return frappe.call({ type: 'GET', - method: 'frappe.desk.reportview.get', + method: 'frappe.desk.reportview.get_count', args: { doctype, filters, fields, + distinct, } }).then(r => { - return r.message.values[0][0]; + return r.message.values; }); }, get_link_options(doctype, txt = '', filters={}) { diff --git a/frappe/public/js/frappe/ui/group_by/group_by.js b/frappe/public/js/frappe/ui/group_by/group_by.js index fa699b1a91..53e4914f0d 100644 --- a/frappe/public/js/frappe/ui/group_by/group_by.js +++ b/frappe/public/js/frappe/ui/group_by/group_by.js @@ -286,15 +286,6 @@ frappe.ui.GroupBy = class { set_args(args) { if (this.aggregate_function && this.group_by) { - let aggregate_column, aggregate_on_field; - - if (this.aggregate_function === 'count') { - aggregate_column = 'count(`tab' + this.doctype + '`.`name`)'; - } else { - aggregate_column = `${this.aggregate_function}(${this.aggregate_on})`; - aggregate_on_field = this.aggregate_on; - } - this.report_view.group_by = this.group_by; this.report_view.sort_by = '_aggregate_column'; this.report_view.sort_order = 'desc'; @@ -316,17 +307,14 @@ frappe.ui.GroupBy = class { '_aggregate_column', this.aggregate_on_doctype || this.doctype, ]); - args.fields.push(aggregate_column + ' as _aggregate_column'); - - if (aggregate_on_field) { - args.fields.push(aggregate_on_field); - } // setup columns in datatable this.report_view.setup_columns(); Object.assign(args, { with_comment_count: false, + aggregate_on: this.aggregate_on || 'name', + aggregate_function: this.aggregate_function || 'count', group_by: this.report_view.group_by || null, order_by: '_aggregate_column desc', }); diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 836bb4bbf5..085c257550 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -17,6 +17,9 @@ from frappe.utils.testutils import add_custom_field, clear_custom_fields test_dependencies = ['User', 'Blog Post', 'Blog Category', 'Blogger'] class TestReportview(unittest.TestCase): + def setUp(self): + frappe.set_user("Administrator") + def test_basic(self): self.assertTrue({"name":"DocType"} in DatabaseQuery("DocType").execute(limit_page_length=None))