From c7338f5a83796933405161b585d4acee0f3cfc44 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 16:45:02 +0530 Subject: [PATCH 01/44] chore: deprecate form_dict.cmd, globals() --- frappe/app.py | 6 +++++- frappe/handler.py | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/frappe/app.py b/frappe/app.py index 28fa9c2426..3d4c2d591f 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -24,8 +24,9 @@ import frappe.utils.response from frappe import _ from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest from frappe.middlewares import StaticDataMiddleware -from frappe.utils import CallbackManager, cint, get_site_name, sanitize_html +from frappe.utils import CallbackManager, cint, get_site_name from frappe.utils.data import escape_html +from frappe.utils.deprecations import deprecation_warning from frappe.utils.error import log_error_snapshot from frappe.website.serve import get_response @@ -99,6 +100,9 @@ def application(request: Request): response = Response() elif frappe.form_dict.cmd: + deprecation_warning( + f"{frappe.form_dict.cmd}: Sending `cmd` for RPC calls is deprecated, call REST API instead `/api/method/cmd`" + ) response = frappe.handler.handle() elif request.path.startswith("/api/"): diff --git a/frappe/handler.py b/frappe/handler.py index c6e7d79878..60c3c53f07 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -15,6 +15,7 @@ from frappe.core.doctype.server_script.server_script_utils import get_server_scr from frappe.monitor import add_data_to_monitor from frappe.utils import cint from frappe.utils.csvutils import build_csv_response +from frappe.utils.deprecations import deprecation_warning from frappe.utils.image import optimize_image from frappe.utils.response import build_response @@ -273,6 +274,9 @@ def get_attr(cmd): if "." in cmd: method = frappe.get_attr(cmd) else: + deprecation_warning( + f"Calling shorthand for {cmd} is deprecated, please specify full path in RPC call." + ) method = globals()[cmd] frappe.log("method:" + cmd) return method From 5af6624cce987909a56ab6b54d9715443f60a8c9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 17:38:20 +0530 Subject: [PATCH 02/44] refactor: Use werkzeug router for API routing --- frappe/api.py | 306 ----------------------------------------- frappe/api/__init__.py | 157 +++++++++++++++++++++ frappe/api/v1.py | 133 ++++++++++++++++++ frappe/app.py | 2 +- 4 files changed, 291 insertions(+), 307 deletions(-) delete mode 100644 frappe/api.py create mode 100644 frappe/api/__init__.py create mode 100644 frappe/api/v1.py diff --git a/frappe/api.py b/frappe/api.py deleted file mode 100644 index 084bee060b..0000000000 --- a/frappe/api.py +++ /dev/null @@ -1,306 +0,0 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors -# License: MIT. See LICENSE -import base64 -import binascii -import json -from typing import Literal -from urllib.parse import urlencode, urlparse - -import frappe -import frappe.client -import frappe.handler -from frappe import _ -from frappe.utils.data import sbool -from frappe.utils.response import build_response - - -def handle(): - """ - Handler for `/api` methods - - ### Examples: - - `/api/method/{methodname}` will call a whitelisted method - - `/api/resource/{doctype}` will query a table - examples: - - `?fields=["name", "owner"]` - - `?filters=[["Task", "name", "like", "%005"]]` - - `?limit_start=0` - - `?limit_page_length=20` - - `/api/resource/{doctype}/{name}` will point to a resource - `GET` will return doclist - `POST` will insert - `PUT` will update - `DELETE` will delete - - `/api/resource/{doctype}/{name}?run_method={method}` will run a whitelisted controller method - """ - - parts = frappe.request.path[1:].split("/", 3) - call = doctype = name = None - - if len(parts) > 1: - call = parts[1] - - if len(parts) > 2: - doctype = parts[2] - - if len(parts) > 3: - name = parts[3] - - return _RESTAPIHandler(call, doctype, name).get_response() - - -class _RESTAPIHandler: - def __init__(self, call: Literal["method", "resource"], doctype: str | None, name: str | None): - self.call = call - self.doctype = doctype - self.name = name - - def get_response(self): - """Prepare and get response based on URL and form body. - - Note: most methods of this class directly operate on the response local. - """ - match self.call: - case "method": - return self.handle_method() - case "resource": - self.handle_resource() - case _: - raise frappe.DoesNotExistError - - return build_response("json") - - def handle_method(self): - frappe.local.form_dict.cmd = self.doctype - return frappe.handler.handle() - - def handle_resource(self): - if self.doctype and self.name: - self.handle_document_resource() - elif self.doctype: - self.handle_doctype_resource() - else: - raise frappe.DoesNotExistError - - def handle_document_resource(self): - if "run_method" in frappe.local.form_dict: - self.execute_doc_method() - return - - match frappe.local.request.method: - case "GET": - self.get_doc() - case "PUT": - self.update_doc() - case "DELETE": - self.delete_doc() - case _: - raise frappe.DoesNotExistError - - def handle_doctype_resource(self): - match frappe.local.request.method: - case "GET": - self.get_doc_list() - case "POST": - self.create_doc() - case _: - raise frappe.DoesNotExistError - - def execute_doc_method(self): - method = frappe.local.form_dict.pop("run_method") - doc = frappe.get_doc(self.doctype, self.name) - doc.is_whitelisted(method) - - if frappe.local.request.method == "GET": - if not doc.has_permission("read"): - frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) - - elif frappe.local.request.method == "POST": - if not doc.has_permission("write"): - frappe.throw(_("Not permitted"), frappe.PermissionError) - - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) - frappe.db.commit() - - def get_doc(self): - doc = frappe.get_doc(self.doctype, self.name) - if not doc.has_permission("read"): - raise frappe.PermissionError - doc.apply_fieldlevel_read_permissions() - frappe.local.response.update({"data": doc}) - - def update_doc(self): - data = get_request_form_data() - - doc = frappe.get_doc(self.doctype, self.name, for_update=True) - - if "flags" in data: - del data["flags"] - - # Not checking permissions here because it's checked in doc.save - doc.update(data) - - frappe.local.response.update({"data": doc.save().as_dict()}) - - # check for child table doctype - if doc.get("parenttype"): - frappe.get_doc(doc.parenttype, doc.parent).save() - frappe.db.commit() - - def delete_doc(self): - # Not checking permissions here because it's checked in delete_doc - frappe.delete_doc(self.doctype, self.name, ignore_missing=False) - frappe.local.response.http_status_code = 202 - frappe.local.response.message = "ok" - frappe.db.commit() - - def get_doc_list(self): - if frappe.local.form_dict.get("fields"): - frappe.local.form_dict["fields"] = json.loads(frappe.local.form_dict["fields"]) - - # set limit of records for frappe.get_list - frappe.local.form_dict.setdefault( - "limit_page_length", - frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, - ) - - # convert strings to native types - only as_dict and debug accept bool - for param in ["as_dict", "debug"]: - param_val = frappe.local.form_dict.get(param) - if param_val is not None: - frappe.local.form_dict[param] = sbool(param_val) - - # evaluate frappe.get_list - data = frappe.call(frappe.client.get_list, self.doctype, **frappe.local.form_dict) - - # set frappe.get_list result to response - frappe.local.response.update({"data": data}) - - def create_doc(self): - data = get_request_form_data() - data.update({"doctype": self.doctype}) - - # insert document from request data - doc = frappe.get_doc(data).insert() - - # set response data - frappe.local.response.update({"data": doc.as_dict()}) - - # commit for POST requests - frappe.db.commit() - - -def get_request_form_data(): - if frappe.local.form_dict.data is None: - data = frappe.safe_decode(frappe.local.request.get_data()) - else: - data = frappe.local.form_dict.data - - try: - return frappe.parse_json(data) - except ValueError: - return frappe.local.form_dict - - -def validate_auth(): - """ - Authenticate and sets user for the request. - """ - authorization_header = frappe.get_request_header("Authorization", "").split(" ") - - if len(authorization_header) == 2: - validate_oauth(authorization_header) - validate_auth_via_api_keys(authorization_header) - - validate_auth_via_hooks() - - -def validate_oauth(authorization_header): - """ - Authenticate request using OAuth and set session user - - Args: - authorization_header (list of str): The 'Authorization' header containing the prefix and token - """ - - from frappe.integrations.oauth2 import get_oauth_server - from frappe.oauth import get_url_delimiter - - form_dict = frappe.local.form_dict - token = authorization_header[1] - req = frappe.request - parsed_url = urlparse(req.url) - access_token = {"access_token": token} - uri = ( - parsed_url.scheme + "://" + parsed_url.netloc + parsed_url.path + "?" + urlencode(access_token) - ) - http_method = req.method - headers = req.headers - body = req.get_data() - if req.content_type and "multipart/form-data" in req.content_type: - body = None - - try: - required_scopes = frappe.db.get_value("OAuth Bearer Token", token, "scopes").split( - get_url_delimiter() - ) - valid, oauthlib_request = get_oauth_server().verify_request( - uri, http_method, body, headers, required_scopes - ) - if valid: - frappe.set_user(frappe.db.get_value("OAuth Bearer Token", token, "user")) - frappe.local.form_dict = form_dict - except AttributeError: - pass - - -def validate_auth_via_api_keys(authorization_header): - """ - Authenticate request using API keys and set session user - - Args: - authorization_header (list of str): The 'Authorization' header containing the prefix and token - """ - - try: - auth_type, auth_token = authorization_header - authorization_source = frappe.get_request_header("Frappe-Authorization-Source") - if auth_type.lower() == "basic": - api_key, api_secret = frappe.safe_decode(base64.b64decode(auth_token)).split(":") - validate_api_key_secret(api_key, api_secret, authorization_source) - elif auth_type.lower() == "token": - api_key, api_secret = auth_token.split(":") - validate_api_key_secret(api_key, api_secret, authorization_source) - except binascii.Error: - frappe.throw( - _("Failed to decode token, please provide a valid base64-encoded token."), - frappe.InvalidAuthorizationToken, - ) - except (AttributeError, TypeError, ValueError): - pass - - -def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): - """frappe_authorization_source to provide api key and secret for a doctype apart from User""" - doctype = frappe_authorization_source or "User" - doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) - form_dict = frappe.local.form_dict - doc_secret = frappe.utils.password.get_decrypted_password(doctype, doc, fieldname="api_secret") - if api_secret == doc_secret: - if doctype == "User": - user = frappe.db.get_value(doctype="User", filters={"api_key": api_key}, fieldname=["name"]) - else: - user = frappe.db.get_value(doctype, doc, "user") - if frappe.local.login_manager.user in ("", "Guest"): - frappe.set_user(user) - frappe.local.form_dict = form_dict - - -def validate_auth_via_hooks(): - for auth_hook in frappe.get_hooks("auth_hooks", []): - frappe.get_attr(auth_hook)() diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py new file mode 100644 index 0000000000..c49cbd6c18 --- /dev/null +++ b/frappe/api/__init__.py @@ -0,0 +1,157 @@ +# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# License: MIT. See LICENSE +import base64 +import binascii +from urllib.parse import urlencode, urlparse + +from werkzeug.exceptions import NotFound +from werkzeug.routing import Map +from werkzeug.wrappers import Request + +import frappe +import frappe.client +import frappe.handler +from frappe import _ +from frappe.utils.response import build_response + + +def handle(request: Request): + """ + Handler for `/api` methods + + ### Examples: + + `/api/method/{methodname}` will call a whitelisted method + + `/api/resource/{doctype}` will query a table + examples: + - `?fields=["name", "owner"]` + - `?filters=[["Task", "name", "like", "%005"]]` + - `?limit_start=0` + - `?limit_page_length=20` + + `/api/resource/{doctype}/{name}` will point to a resource + `GET` will return doclist + `POST` will insert + `PUT` will update + `DELETE` will delete + + `/api/resource/{doctype}/{name}?run_method={method}` will run a whitelisted controller method + """ + + try: + endpoint, arguments = API_URL_MAP.bind_to_environ(request.environ).match() + except NotFound: # Wrap 404 - backward compatiblity + raise frappe.DoesNotExistError + + endpoint(**arguments) + return build_response("json") + + +def validate_auth(): + """ + Authenticate and sets user for the request. + """ + authorization_header = frappe.get_request_header("Authorization", "").split(" ") + + if len(authorization_header) == 2: + validate_oauth(authorization_header) + validate_auth_via_api_keys(authorization_header) + + validate_auth_via_hooks() + + +def validate_oauth(authorization_header): + """ + Authenticate request using OAuth and set session user + + Args: + authorization_header (list of str): The 'Authorization' header containing the prefix and token + """ + + from frappe.integrations.oauth2 import get_oauth_server + from frappe.oauth import get_url_delimiter + + form_dict = frappe.local.form_dict + token = authorization_header[1] + req = frappe.request + parsed_url = urlparse(req.url) + access_token = {"access_token": token} + uri = ( + parsed_url.scheme + "://" + parsed_url.netloc + parsed_url.path + "?" + urlencode(access_token) + ) + http_method = req.method + headers = req.headers + body = req.get_data() + if req.content_type and "multipart/form-data" in req.content_type: + body = None + + try: + required_scopes = frappe.db.get_value("OAuth Bearer Token", token, "scopes").split( + get_url_delimiter() + ) + valid, oauthlib_request = get_oauth_server().verify_request( + uri, http_method, body, headers, required_scopes + ) + if valid: + frappe.set_user(frappe.db.get_value("OAuth Bearer Token", token, "user")) + frappe.local.form_dict = form_dict + except AttributeError: + pass + + +def validate_auth_via_api_keys(authorization_header): + """ + Authenticate request using API keys and set session user + + Args: + authorization_header (list of str): The 'Authorization' header containing the prefix and token + """ + + try: + auth_type, auth_token = authorization_header + authorization_source = frappe.get_request_header("Frappe-Authorization-Source") + if auth_type.lower() == "basic": + api_key, api_secret = frappe.safe_decode(base64.b64decode(auth_token)).split(":") + validate_api_key_secret(api_key, api_secret, authorization_source) + elif auth_type.lower() == "token": + api_key, api_secret = auth_token.split(":") + validate_api_key_secret(api_key, api_secret, authorization_source) + except binascii.Error: + frappe.throw( + _("Failed to decode token, please provide a valid base64-encoded token."), + frappe.InvalidAuthorizationToken, + ) + except (AttributeError, TypeError, ValueError): + pass + + +def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): + """frappe_authorization_source to provide api key and secret for a doctype apart from User""" + doctype = frappe_authorization_source or "User" + doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) + form_dict = frappe.local.form_dict + doc_secret = frappe.utils.password.get_decrypted_password(doctype, doc, fieldname="api_secret") + if api_secret == doc_secret: + if doctype == "User": + user = frappe.db.get_value(doctype="User", filters={"api_key": api_key}, fieldname=["name"]) + else: + user = frappe.db.get_value(doctype, doc, "user") + if frappe.local.login_manager.user in ("", "Guest"): + frappe.set_user(user) + frappe.local.form_dict = form_dict + + +def validate_auth_via_hooks(): + for auth_hook in frappe.get_hooks("auth_hooks", []): + frappe.get_attr(auth_hook)() + + +# Merge all API version routing rules +from frappe.api.v1 import url_rules as v1_rules + +url_rules = [ + *v1_rules, +] + +API_URL_MAP = Map(url_rules) diff --git a/frappe/api/v1.py b/frappe/api/v1.py new file mode 100644 index 0000000000..cf2932e869 --- /dev/null +++ b/frappe/api/v1.py @@ -0,0 +1,133 @@ +import json + +from werkzeug.routing import Rule + +import frappe +from frappe import _ +from frappe.utils.data import sbool + + +def handle_rpc_call(method: str): + # TODO: inline this weird circular calls + frappe.local.form_dict.cmd = method + return frappe.handler.handle() + + +def get_doc_list(doctype: str): + if frappe.local.form_dict.get("fields"): + frappe.local.form_dict["fields"] = json.loads(frappe.local.form_dict["fields"]) + + # set limit of records for frappe.get_list + frappe.local.form_dict.setdefault( + "limit_page_length", + frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, + ) + + # convert strings to native types - only as_dict and debug accept bool + for param in ["as_dict", "debug"]: + param_val = frappe.local.form_dict.get(param) + if param_val is not None: + frappe.local.form_dict[param] = sbool(param_val) + + # evaluate frappe.get_list + data = frappe.call(frappe.client.get_list, doctype, **frappe.local.form_dict) + + # set frappe.get_list result to response + frappe.local.response.update({"data": data}) + + +def create_doc(doctype: str): + data = get_request_form_data() + data.update({"doctype": doctype}) + + # insert document from request data + doc = frappe.get_doc(data).insert() + + # set response data + frappe.local.response.update({"data": doc.as_dict()}) + + # commit for POST requests + frappe.db.commit() + + +def read_doc(doctype: str, name: str): + # Backward compatiblity + if "run_method" in frappe.local.form_dict: + execute_doc_method(doctype, name) + return + + doc = frappe.get_doc(doctype, name) + if not doc.has_permission("read"): + raise frappe.PermissionError + doc.apply_fieldlevel_read_permissions() + frappe.local.response.update({"data": doc}) + + +def update_doc(doctype: str, name: str): + data = get_request_form_data() + + doc = frappe.get_doc(doctype, name, for_update=True) + + if "flags" in data: + del data["flags"] + + # Not checking permissions here because it's checked in doc.save + doc.update(data) + + frappe.local.response.update({"data": doc.save().as_dict()}) + + # check for child table doctype + if doc.get("parenttype"): + frappe.get_doc(doc.parenttype, doc.parent).save() + frappe.db.commit() + + +def delete_doc(doctype: str, name: str): + # Not checking permissions here because it's checked in delete_doc + frappe.delete_doc(doctype, name, ignore_missing=False) + frappe.local.response.http_status_code = 202 + frappe.local.response.message = "ok" + frappe.db.commit() + + +def execute_doc_method(doctype: str, name: str, method: str | None = None): + method = method or frappe.local.form_dict.pop("run_method") + doc = frappe.get_doc(doctype, name) + doc.is_whitelisted(method) + + if frappe.local.request.method == "GET": + if not doc.has_permission("read"): + frappe.throw(_("Not permitted"), frappe.PermissionError) + frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + + elif frappe.local.request.method == "POST": + if not doc.has_permission("write"): + frappe.throw(_("Not permitted"), frappe.PermissionError) + + frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.db.commit() + + +def get_request_form_data(): + if frappe.local.form_dict.data is None: + data = frappe.safe_decode(frappe.local.request.get_data()) + else: + data = frappe.local.form_dict.data + + try: + return frappe.parse_json(data) + except ValueError: + return frappe.local.form_dict + + +url_rules = [ + Rule("/api/method/", endpoint=handle_rpc_call), + Rule("/api/resource/", methods=["GET"], endpoint=get_doc_list), + Rule("/api/resource/", methods=["POST"], endpoint=create_doc), + Rule("/api/resource//", methods=["GET"], endpoint=read_doc), + Rule("/api/resource//", methods=["PUT"], endpoint=update_doc), + Rule("/api/resource//", methods=["DELETE"], endpoint=delete_doc), + Rule( + "/api/resource//", methods=["POST"], endpoint=execute_doc_method + ), +] diff --git a/frappe/app.py b/frappe/app.py index 3d4c2d591f..2be9bad757 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -106,7 +106,7 @@ def application(request: Request): response = frappe.handler.handle() elif request.path.startswith("/api/"): - response = frappe.api.handle() + response = frappe.api.handle(request) elif request.path.startswith("/backups"): response = frappe.utils.response.download_backup(request.path) From 1b51914a838bb8f2b1cc697e664e743297466451 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 18:16:31 +0530 Subject: [PATCH 03/44] refactor: create two API versions --- frappe/api/__init__.py | 17 ++++-- frappe/api/v1.py | 16 +++--- frappe/api/v2.py | 125 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 15 deletions(-) create mode 100644 frappe/api/v2.py diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index c49cbd6c18..e2de93111b 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -5,7 +5,7 @@ import binascii from urllib.parse import urlencode, urlparse from werkzeug.exceptions import NotFound -from werkzeug.routing import Map +from werkzeug.routing import Map, Submount from werkzeug.wrappers import Request import frappe @@ -149,9 +149,14 @@ def validate_auth_via_hooks(): # Merge all API version routing rules from frappe.api.v1 import url_rules as v1_rules +from frappe.api.v2 import url_rules as v2_rules -url_rules = [ - *v1_rules, -] - -API_URL_MAP = Map(url_rules) +API_URL_MAP = Map( + [ + # V1 routes + Submount("/api", v1_rules), + Submount("/api/v1", v1_rules), + Submount("/api/v2", v2_rules), + ], + strict_slashes=False, # Allows skipping trailing slashes +) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index cf2932e869..0d1de01da7 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -121,13 +121,11 @@ def get_request_form_data(): url_rules = [ - Rule("/api/method/", endpoint=handle_rpc_call), - Rule("/api/resource/", methods=["GET"], endpoint=get_doc_list), - Rule("/api/resource/", methods=["POST"], endpoint=create_doc), - Rule("/api/resource//", methods=["GET"], endpoint=read_doc), - Rule("/api/resource//", methods=["PUT"], endpoint=update_doc), - Rule("/api/resource//", methods=["DELETE"], endpoint=delete_doc), - Rule( - "/api/resource//", methods=["POST"], endpoint=execute_doc_method - ), + Rule("/method/", endpoint=handle_rpc_call), + Rule("/resource/", methods=["GET"], endpoint=get_doc_list), + Rule("/resource/", methods=["POST"], endpoint=create_doc), + Rule("/resource//", methods=["GET"], endpoint=read_doc), + Rule("/resource//", methods=["PUT"], endpoint=update_doc), + Rule("/resource//", methods=["DELETE"], endpoint=delete_doc), + Rule("/resource//", methods=["POST"], endpoint=execute_doc_method), ] diff --git a/frappe/api/v2.py b/frappe/api/v2.py new file mode 100644 index 0000000000..1fbb74693d --- /dev/null +++ b/frappe/api/v2.py @@ -0,0 +1,125 @@ +import json + +from werkzeug.routing import Rule + +import frappe +from frappe import _ +from frappe.utils.data import sbool + + +def handle_rpc_call(method: str): + # TODO: inline this weird circular calls + frappe.local.form_dict.cmd = method + return frappe.handler.handle() + + +def get_doc_list(doctype: str): + if frappe.local.form_dict.get("fields"): + frappe.local.form_dict["fields"] = json.loads(frappe.local.form_dict["fields"]) + + # set limit of records for frappe.get_list + frappe.local.form_dict.setdefault( + "limit_page_length", + frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, + ) + + # convert strings to native types - only as_dict and debug accept bool + for param in ["as_dict", "debug"]: + param_val = frappe.local.form_dict.get(param) + if param_val is not None: + frappe.local.form_dict[param] = sbool(param_val) + + # evaluate frappe.get_list + data = frappe.call(frappe.client.get_list, doctype, **frappe.local.form_dict) + + # set frappe.get_list result to response + frappe.local.response.update({"data": data}) + + +def create_doc(doctype: str): + data = get_request_form_data() + data.update({"doctype": doctype}) + + # insert document from request data + doc = frappe.get_doc(data).insert() + + # set response data + frappe.local.response.update({"data": doc.as_dict()}) + + # commit for POST requests + frappe.db.commit() + + +def read_doc(doctype: str, name: str): + doc = frappe.get_doc(doctype, name) + if not doc.has_permission("read"): + raise frappe.PermissionError + doc.apply_fieldlevel_read_permissions() + frappe.local.response.update({"data": doc}) + + +def update_doc(doctype: str, name: str): + data = get_request_form_data() + + doc = frappe.get_doc(doctype, name, for_update=True) + + if "flags" in data: + del data["flags"] + + # Not checking permissions here because it's checked in doc.save + doc.update(data) + + frappe.local.response.update({"data": doc.save().as_dict()}) + + # check for child table doctype + if doc.get("parenttype"): + frappe.get_doc(doc.parenttype, doc.parent).save() + frappe.db.commit() + + +def delete_doc(doctype: str, name: str): + # Not checking permissions here because it's checked in delete_doc + frappe.delete_doc(doctype, name, ignore_missing=False) + frappe.local.response.http_status_code = 202 + frappe.local.response.message = "ok" + frappe.db.commit() + + +def execute_doc_method(doctype: str, name: str, method: str | None = None): + method = method or frappe.local.form_dict.pop("run_method") + doc = frappe.get_doc(doctype, name) + doc.is_whitelisted(method) + + if frappe.local.request.method == "GET": + if not doc.has_permission("read"): + frappe.throw(_("Not permitted"), frappe.PermissionError) + frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + + elif frappe.local.request.method == "POST": + if not doc.has_permission("write"): + frappe.throw(_("Not permitted"), frappe.PermissionError) + + frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.db.commit() + + +def get_request_form_data(): + if frappe.local.form_dict.data is None: + data = frappe.safe_decode(frappe.local.request.get_data()) + else: + data = frappe.local.form_dict.data + + try: + return frappe.parse_json(data) + except ValueError: + return frappe.local.form_dict + + +url_rules = [ + Rule("/method/", endpoint=handle_rpc_call), + Rule("/resource/", methods=["GET"], endpoint=get_doc_list), + Rule("/resource/", methods=["POST"], endpoint=create_doc), + Rule("/resource//", methods=["GET"], endpoint=read_doc), + Rule("/resource//", methods=["PUT"], endpoint=update_doc), + Rule("/resource//", methods=["DELETE"], endpoint=delete_doc), +] From 5f32952b30b8082f0d706b7b0321c7b65d4eac0d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 18:25:14 +0530 Subject: [PATCH 04/44] feat(APIv2): Execute doc method using API e.g. `POST /api/v2/resource/Sales Order/SO-0001/submit` --- frappe/api/v2.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 1fbb74693d..1b3896df00 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -122,4 +122,9 @@ url_rules = [ Rule("/resource//", methods=["GET"], endpoint=read_doc), Rule("/resource//", methods=["PUT"], endpoint=update_doc), Rule("/resource//", methods=["DELETE"], endpoint=delete_doc), + Rule( + "/resource///", + methods=["GET", "POST"], + endpoint=execute_doc_method, + ), ] From 11dd961d81769212d74359669486ab5ff4542cd5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 18:48:52 +0530 Subject: [PATCH 05/44] refactor!: Method whitelisting Document.whitelist doesn't work, no idea why it's doing all weird `__func__` business. `@frappe.whitelist()` works just fine. --- frappe/model/document.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 410d889e58..2d57922064 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -22,6 +22,7 @@ from frappe.model.workflow import set_workflow_state_on_action, validate_workflo from frappe.types import DF from frappe.utils import compare, cstr, date_diff, file_lock, flt, get_datetime_str, now from frappe.utils.data import get_absolute_url +from frappe.utils.deprecations import deprecated from frappe.utils.global_search import update_global_search if TYPE_CHECKING: @@ -139,12 +140,6 @@ class Document(BaseDocument): def is_locked(self): return file_lock.lock_exists(self.get_signature()) - @staticmethod - def whitelist(fn): - """Decorator: Whitelist method to be called remotely via REST API.""" - frappe.whitelist()(fn) - return fn - def load_from_db(self): """Load document and children from database and create properties from fields""" @@ -1016,19 +1011,16 @@ class Document(BaseDocument): elif alert.event == "Method" and method == alert.method: _evaluate_alert(alert) - @whitelist.__func__ def _submit(self): """Submit the document. Sets `docstatus` = 1, then saves.""" self.docstatus = DocStatus.submitted() return self.save() - @whitelist.__func__ def _cancel(self): """Cancel the document. Sets `docstatus` = 2, then saves.""" self.docstatus = DocStatus.cancelled() return self.save() - @whitelist.__func__ def _rename( self, name: str, merge: bool = False, force: bool = False, validate_rename: bool = True ): @@ -1038,17 +1030,17 @@ class Document(BaseDocument): self.name = rename_doc(doc=self, new=name, merge=merge, force=force, validate=validate_rename) self.reload() - @whitelist.__func__ + @frappe.whitelist() def submit(self): """Submit the document. Sets `docstatus` = 1, then saves.""" return self._submit() - @whitelist.__func__ + @frappe.whitelist() def cancel(self): """Cancel the document. Sets `docstatus` = 2, then saves.""" return self._cancel() - @whitelist.__func__ + @frappe.whitelist() def rename( self, name: str, merge: bool = False, force: bool = False, validate_rename: bool = True ): From b064d124408a56c8264fa8376632ccc6ce20753b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 19:02:32 +0530 Subject: [PATCH 06/44] feat: Shorthand for whitelisted function call in controllers ```diff - GET /api/method/frappe.core.doctype.user.user.get_timezones + GET /api/method/User/get_timezones ``` --- frappe/api/v1.py | 2 ++ frappe/api/v2.py | 11 ++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 0d1de01da7..d6fa5a365c 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -8,6 +8,8 @@ from frappe.utils.data import sbool def handle_rpc_call(method: str): + import frappe.handler + # TODO: inline this weird circular calls frappe.local.form_dict.cmd = method return frappe.handler.handle() diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 1b3896df00..8a00770e55 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -7,8 +7,16 @@ from frappe import _ from frappe.utils.data import sbool -def handle_rpc_call(method: str): +def handle_rpc_call(method: str, doctype: str | None = None): # TODO: inline this weird circular calls + import frappe.handler + from frappe.modules.utils import load_doctype_module + + if doctype: + # TODO: HACKY implementation, simplify all this before merge + module = load_doctype_module(doctype) + method = module.__name__ + "." + method + frappe.local.form_dict.cmd = method return frappe.handler.handle() @@ -117,6 +125,7 @@ def get_request_form_data(): url_rules = [ Rule("/method/", endpoint=handle_rpc_call), + Rule("/method//", endpoint=handle_rpc_call), Rule("/resource/", methods=["GET"], endpoint=get_doc_list), Rule("/resource/", methods=["POST"], endpoint=create_doc), Rule("/resource//", methods=["GET"], endpoint=read_doc), From e0f87dc4e1ee7781fd5d29e1bb93468078314d5c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 19:37:49 +0530 Subject: [PATCH 07/44] refactor!: move OAuth and token auth code to auth.py This doesn't belong in api.py --- frappe/api/__init__.py | 103 ----------------------------------------- frappe/app.py | 4 +- frappe/auth.py | 103 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 104 insertions(+), 106 deletions(-) diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index e2de93111b..054c633104 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -1,9 +1,5 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import base64 -import binascii -from urllib.parse import urlencode, urlparse - from werkzeug.exceptions import NotFound from werkzeug.routing import Map, Submount from werkzeug.wrappers import Request @@ -48,105 +44,6 @@ def handle(request: Request): return build_response("json") -def validate_auth(): - """ - Authenticate and sets user for the request. - """ - authorization_header = frappe.get_request_header("Authorization", "").split(" ") - - if len(authorization_header) == 2: - validate_oauth(authorization_header) - validate_auth_via_api_keys(authorization_header) - - validate_auth_via_hooks() - - -def validate_oauth(authorization_header): - """ - Authenticate request using OAuth and set session user - - Args: - authorization_header (list of str): The 'Authorization' header containing the prefix and token - """ - - from frappe.integrations.oauth2 import get_oauth_server - from frappe.oauth import get_url_delimiter - - form_dict = frappe.local.form_dict - token = authorization_header[1] - req = frappe.request - parsed_url = urlparse(req.url) - access_token = {"access_token": token} - uri = ( - parsed_url.scheme + "://" + parsed_url.netloc + parsed_url.path + "?" + urlencode(access_token) - ) - http_method = req.method - headers = req.headers - body = req.get_data() - if req.content_type and "multipart/form-data" in req.content_type: - body = None - - try: - required_scopes = frappe.db.get_value("OAuth Bearer Token", token, "scopes").split( - get_url_delimiter() - ) - valid, oauthlib_request = get_oauth_server().verify_request( - uri, http_method, body, headers, required_scopes - ) - if valid: - frappe.set_user(frappe.db.get_value("OAuth Bearer Token", token, "user")) - frappe.local.form_dict = form_dict - except AttributeError: - pass - - -def validate_auth_via_api_keys(authorization_header): - """ - Authenticate request using API keys and set session user - - Args: - authorization_header (list of str): The 'Authorization' header containing the prefix and token - """ - - try: - auth_type, auth_token = authorization_header - authorization_source = frappe.get_request_header("Frappe-Authorization-Source") - if auth_type.lower() == "basic": - api_key, api_secret = frappe.safe_decode(base64.b64decode(auth_token)).split(":") - validate_api_key_secret(api_key, api_secret, authorization_source) - elif auth_type.lower() == "token": - api_key, api_secret = auth_token.split(":") - validate_api_key_secret(api_key, api_secret, authorization_source) - except binascii.Error: - frappe.throw( - _("Failed to decode token, please provide a valid base64-encoded token."), - frappe.InvalidAuthorizationToken, - ) - except (AttributeError, TypeError, ValueError): - pass - - -def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): - """frappe_authorization_source to provide api key and secret for a doctype apart from User""" - doctype = frappe_authorization_source or "User" - doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) - form_dict = frappe.local.form_dict - doc_secret = frappe.utils.password.get_decrypted_password(doctype, doc, fieldname="api_secret") - if api_secret == doc_secret: - if doctype == "User": - user = frappe.db.get_value(doctype="User", filters={"api_key": api_key}, fieldname=["name"]) - else: - user = frappe.db.get_value(doctype, doc, "user") - if frappe.local.login_manager.user in ("", "Guest"): - frappe.set_user(user) - frappe.local.form_dict = form_dict - - -def validate_auth_via_hooks(): - for auth_hook in frappe.get_hooks("auth_hooks", []): - frappe.get_attr(auth_hook)() - - # Merge all API version routing rules from frappe.api.v1 import url_rules as v1_rules from frappe.api.v2 import url_rules as v2_rules diff --git a/frappe/app.py b/frappe/app.py index 2be9bad757..58cb51d52e 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -22,7 +22,7 @@ import frappe.rate_limiter import frappe.recorder import frappe.utils.response from frappe import _ -from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest +from frappe.auth import SAFE_HTTP_METHODS, UNSAFE_HTTP_METHODS, HTTPRequest, validate_auth from frappe.middlewares import StaticDataMiddleware from frappe.utils import CallbackManager, cint, get_site_name from frappe.utils.data import escape_html @@ -94,7 +94,7 @@ def application(request: Request): init_request(request) - frappe.api.validate_auth() + validate_auth() if request.method == "OPTIONS": response = Response() diff --git a/frappe/auth.py b/frappe/auth.py index d1259e1aaf..01a1323a5b 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -1,6 +1,8 @@ # Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See LICENSE -from urllib.parse import quote +import base64 +import binascii +from urllib.parse import quote, urlencode, urlparse import frappe import frappe.database @@ -547,3 +549,102 @@ class LoginAttemptTracker: ): return False return True + + +def validate_auth(): + """ + Authenticate and sets user for the request. + """ + authorization_header = frappe.get_request_header("Authorization", "").split(" ") + + if len(authorization_header) == 2: + validate_oauth(authorization_header) + validate_auth_via_api_keys(authorization_header) + + validate_auth_via_hooks() + + +def validate_oauth(authorization_header): + """ + Authenticate request using OAuth and set session user + + Args: + authorization_header (list of str): The 'Authorization' header containing the prefix and token + """ + + from frappe.integrations.oauth2 import get_oauth_server + from frappe.oauth import get_url_delimiter + + form_dict = frappe.local.form_dict + token = authorization_header[1] + req = frappe.request + parsed_url = urlparse(req.url) + access_token = {"access_token": token} + uri = ( + parsed_url.scheme + "://" + parsed_url.netloc + parsed_url.path + "?" + urlencode(access_token) + ) + http_method = req.method + headers = req.headers + body = req.get_data() + if req.content_type and "multipart/form-data" in req.content_type: + body = None + + try: + required_scopes = frappe.db.get_value("OAuth Bearer Token", token, "scopes").split( + get_url_delimiter() + ) + valid, oauthlib_request = get_oauth_server().verify_request( + uri, http_method, body, headers, required_scopes + ) + if valid: + frappe.set_user(frappe.db.get_value("OAuth Bearer Token", token, "user")) + frappe.local.form_dict = form_dict + except AttributeError: + pass + + +def validate_auth_via_api_keys(authorization_header): + """ + Authenticate request using API keys and set session user + + Args: + authorization_header (list of str): The 'Authorization' header containing the prefix and token + """ + + try: + auth_type, auth_token = authorization_header + authorization_source = frappe.get_request_header("Frappe-Authorization-Source") + if auth_type.lower() == "basic": + api_key, api_secret = frappe.safe_decode(base64.b64decode(auth_token)).split(":") + validate_api_key_secret(api_key, api_secret, authorization_source) + elif auth_type.lower() == "token": + api_key, api_secret = auth_token.split(":") + validate_api_key_secret(api_key, api_secret, authorization_source) + except binascii.Error: + frappe.throw( + _("Failed to decode token, please provide a valid base64-encoded token."), + frappe.InvalidAuthorizationToken, + ) + except (AttributeError, TypeError, ValueError): + pass + + +def validate_api_key_secret(api_key, api_secret, frappe_authorization_source=None): + """frappe_authorization_source to provide api key and secret for a doctype apart from User""" + doctype = frappe_authorization_source or "User" + doc = frappe.db.get_value(doctype=doctype, filters={"api_key": api_key}, fieldname=["name"]) + form_dict = frappe.local.form_dict + doc_secret = frappe.utils.password.get_decrypted_password(doctype, doc, fieldname="api_secret") + if api_secret == doc_secret: + if doctype == "User": + user = frappe.db.get_value(doctype="User", filters={"api_key": api_key}, fieldname=["name"]) + else: + user = frappe.db.get_value(doctype, doc, "user") + if frappe.local.login_manager.user in ("", "Guest"): + frappe.set_user(user) + frappe.local.form_dict = form_dict + + +def validate_auth_via_hooks(): + for auth_hook in frappe.get_hooks("auth_hooks", []): + frappe.get_attr(auth_hook)() From d5a21a26769ef0a487ecfed42a50ffbde682336b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 19:46:31 +0530 Subject: [PATCH 08/44] fix: rename type validation `None` can be passed which is acceptable here. TODO: Make slackdict accept none as bool and convert to False --- frappe/model/document.py | 4 +--- frappe/utils/typing_validations.py | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 2d57922064..9cfb3057bd 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1041,9 +1041,7 @@ class Document(BaseDocument): return self._cancel() @frappe.whitelist() - def rename( - self, name: str, merge: bool = False, force: bool = False, validate_rename: bool = True - ): + def rename(self, name: str, merge=False, force=False, validate_rename=True): """Rename the document to `name`. This transforms the current object.""" return self._rename(name=name, merge=merge, force=force, validate_rename=validate_rename) diff --git a/frappe/utils/typing_validations.py b/frappe/utils/typing_validations.py index 3f24c5eb19..cd8e736fe6 100644 --- a/frappe/utils/typing_validations.py +++ b/frappe/utils/typing_validations.py @@ -1,7 +1,7 @@ from collections.abc import Callable from functools import lru_cache, wraps from inspect import _empty, isclass, signature -from types import EllipsisType +from types import EllipsisType, NoneType from typing import ForwardRef, TypeVar, Union from pydantic import ConfigDict From 507343f4f6f730c2ccd6d86e4981c76c21b5b22c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 20:12:24 +0530 Subject: [PATCH 09/44] fix: double response processing --- frappe/app.py | 3 ++- frappe/handler.py | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/frappe/app.py b/frappe/app.py index 58cb51d52e..add62c2bbd 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -103,7 +103,8 @@ def application(request: Request): deprecation_warning( f"{frappe.form_dict.cmd}: Sending `cmd` for RPC calls is deprecated, call REST API instead `/api/method/cmd`" ) - response = frappe.handler.handle() + frappe.handler.handle() + response = frappe.utils.response.build_response("json") elif request.path.startswith("/api/"): response = frappe.api.handle(request) diff --git a/frappe/handler.py b/frappe/handler.py index 60c3c53f07..fe438d4cae 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -57,8 +57,6 @@ def handle(): # add the response to `message` label frappe.response["message"] = data - return build_response("json") - def execute_cmd(cmd, from_async=False): """execute a request as python module""" From 26b3f6ae961f21c4ede13d6d0eb1edd9c9a4c5f2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 20:57:47 +0530 Subject: [PATCH 10/44] fix: pass on custom responses as is without any post processing --- frappe/api/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index 054c633104..39e95ba1c3 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -2,7 +2,7 @@ # License: MIT. See LICENSE from werkzeug.exceptions import NotFound from werkzeug.routing import Map, Submount -from werkzeug.wrappers import Request +from werkzeug.wrappers import Request, Response import frappe import frappe.client @@ -40,7 +40,10 @@ def handle(request: Request): except NotFound: # Wrap 404 - backward compatiblity raise frappe.DoesNotExistError - endpoint(**arguments) + resp = endpoint(**arguments) + if isinstance(resp, Response): + return resp + return build_response("json") From b6b4b98c0f88643e47878f5f53b5d1d2b1c4fcbe Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:07:22 +0530 Subject: [PATCH 11/44] refactor(APIv2): `message` -> `data` --- frappe/api/v2.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 8a00770e55..782f6183c5 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -1,6 +1,7 @@ import json from werkzeug.routing import Rule +from werkzeug.wrappers import Response import frappe from frappe import _ @@ -8,17 +9,23 @@ from frappe.utils.data import sbool def handle_rpc_call(method: str, doctype: str | None = None): - # TODO: inline this weird circular calls - import frappe.handler + from frappe.handler import execute_cmd from frappe.modules.utils import load_doctype_module if doctype: - # TODO: HACKY implementation, simplify all this before merge + # Expand to cover actual method module = load_doctype_module(doctype) method = module.__name__ + "." + method - frappe.local.form_dict.cmd = method - return frappe.handler.handle() + if method == "login": + return + + data = execute_cmd(method) + if isinstance(data, Response): + # method returns a response object, pass it on + return data + + frappe.response["data"] = data def get_doc_list(doctype: str): From f9e120c1040e577acc673422c97cc0a51fb8e8ca Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:11:20 +0530 Subject: [PATCH 12/44] refactor: remove unnessary manual commits --- frappe/api/v1.py | 6 ------ frappe/api/v2.py | 10 +--------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index d6fa5a365c..44496c82f2 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -48,9 +48,6 @@ def create_doc(doctype: str): # set response data frappe.local.response.update({"data": doc.as_dict()}) - # commit for POST requests - frappe.db.commit() - def read_doc(doctype: str, name: str): # Backward compatiblity @@ -81,7 +78,6 @@ def update_doc(doctype: str, name: str): # check for child table doctype if doc.get("parenttype"): frappe.get_doc(doc.parenttype, doc.parent).save() - frappe.db.commit() def delete_doc(doctype: str, name: str): @@ -89,7 +85,6 @@ def delete_doc(doctype: str, name: str): frappe.delete_doc(doctype, name, ignore_missing=False) frappe.local.response.http_status_code = 202 frappe.local.response.message = "ok" - frappe.db.commit() def execute_doc_method(doctype: str, name: str, method: str | None = None): @@ -107,7 +102,6 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): frappe.throw(_("Not permitted"), frappe.PermissionError) frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) - frappe.db.commit() def get_request_form_data(): diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 782f6183c5..a36d422311 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -13,7 +13,7 @@ def handle_rpc_call(method: str, doctype: str | None = None): from frappe.modules.utils import load_doctype_module if doctype: - # Expand to cover actual method + # Expand to run actual method module = load_doctype_module(doctype) method = module.__name__ + "." + method @@ -61,9 +61,6 @@ def create_doc(doctype: str): # set response data frappe.local.response.update({"data": doc.as_dict()}) - # commit for POST requests - frappe.db.commit() - def read_doc(doctype: str, name: str): doc = frappe.get_doc(doctype, name) @@ -89,15 +86,11 @@ def update_doc(doctype: str, name: str): # check for child table doctype if doc.get("parenttype"): frappe.get_doc(doc.parenttype, doc.parent).save() - frappe.db.commit() def delete_doc(doctype: str, name: str): - # Not checking permissions here because it's checked in delete_doc frappe.delete_doc(doctype, name, ignore_missing=False) frappe.local.response.http_status_code = 202 - frappe.local.response.message = "ok" - frappe.db.commit() def execute_doc_method(doctype: str, name: str, method: str | None = None): @@ -115,7 +108,6 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): frappe.throw(_("Not permitted"), frappe.PermissionError) frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) - frappe.db.commit() def get_request_form_data(): From 0b0b7f7f600abdae37e45db062abb06db1affc60 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:20:43 +0530 Subject: [PATCH 13/44] test: check that document actually got deleted --- frappe/tests/test_api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 5098158e0f..44e5d2d264 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -200,6 +200,9 @@ class TestResourceAPI(FrappeAPITestCase): response = self.delete(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") self.assertEqual(response.status_code, 202) self.assertDictEqual(response.json, {"message": "ok"}) + + response = self.get(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") + self.assertEqual(response.status_code, 404) self.GENERATED_DOCUMENTS.remove(doc_to_delete) non_existent_doc = frappe.generate_hash(length=12) From 8ce6426c752516e13eae46e68f075a0df5e0c106 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:24:12 +0530 Subject: [PATCH 14/44] refactor: Drop `frappe.local` for proxies --- frappe/api/v1.py | 48 ++++++++++++++++++++++++------------------------ frappe/api/v2.py | 42 +++++++++++++++++++++--------------------- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 44496c82f2..9ce7b9bd71 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -11,31 +11,31 @@ def handle_rpc_call(method: str): import frappe.handler # TODO: inline this weird circular calls - frappe.local.form_dict.cmd = method + frappe.form_dict.cmd = method return frappe.handler.handle() def get_doc_list(doctype: str): - if frappe.local.form_dict.get("fields"): - frappe.local.form_dict["fields"] = json.loads(frappe.local.form_dict["fields"]) + if frappe.form_dict.get("fields"): + frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) # set limit of records for frappe.get_list - frappe.local.form_dict.setdefault( + frappe.form_dict.setdefault( "limit_page_length", - frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, + frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, ) # convert strings to native types - only as_dict and debug accept bool for param in ["as_dict", "debug"]: - param_val = frappe.local.form_dict.get(param) + param_val = frappe.form_dict.get(param) if param_val is not None: - frappe.local.form_dict[param] = sbool(param_val) + frappe.form_dict[param] = sbool(param_val) # evaluate frappe.get_list - data = frappe.call(frappe.client.get_list, doctype, **frappe.local.form_dict) + data = frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) # set frappe.get_list result to response - frappe.local.response.update({"data": data}) + frappe.response.update({"data": data}) def create_doc(doctype: str): @@ -46,12 +46,12 @@ def create_doc(doctype: str): doc = frappe.get_doc(data).insert() # set response data - frappe.local.response.update({"data": doc.as_dict()}) + frappe.response.update({"data": doc.as_dict()}) def read_doc(doctype: str, name: str): # Backward compatiblity - if "run_method" in frappe.local.form_dict: + if "run_method" in frappe.form_dict: execute_doc_method(doctype, name) return @@ -59,7 +59,7 @@ def read_doc(doctype: str, name: str): if not doc.has_permission("read"): raise frappe.PermissionError doc.apply_fieldlevel_read_permissions() - frappe.local.response.update({"data": doc}) + frappe.response.update({"data": doc}) def update_doc(doctype: str, name: str): @@ -73,7 +73,7 @@ def update_doc(doctype: str, name: str): # Not checking permissions here because it's checked in doc.save doc.update(data) - frappe.local.response.update({"data": doc.save().as_dict()}) + frappe.response.update({"data": doc.save().as_dict()}) # check for child table doctype if doc.get("parenttype"): @@ -83,37 +83,37 @@ def update_doc(doctype: str, name: str): def delete_doc(doctype: str, name: str): # Not checking permissions here because it's checked in delete_doc frappe.delete_doc(doctype, name, ignore_missing=False) - frappe.local.response.http_status_code = 202 - frappe.local.response.message = "ok" + frappe.response.http_status_code = 202 + frappe.response.message = "ok" def execute_doc_method(doctype: str, name: str, method: str | None = None): - method = method or frappe.local.form_dict.pop("run_method") + method = method or frappe.form_dict.pop("run_method") doc = frappe.get_doc(doctype, name) doc.is_whitelisted(method) - if frappe.local.request.method == "GET": + if frappe.request.method == "GET": if not doc.has_permission("read"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) - elif frappe.local.request.method == "POST": + elif frappe.request.method == "POST": if not doc.has_permission("write"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) def get_request_form_data(): - if frappe.local.form_dict.data is None: - data = frappe.safe_decode(frappe.local.request.get_data()) + if frappe.form_dict.data is None: + data = frappe.safe_decode(frappe.request.get_data()) else: - data = frappe.local.form_dict.data + data = frappe.form_dict.data try: return frappe.parse_json(data) except ValueError: - return frappe.local.form_dict + return frappe.form_dict url_rules = [ diff --git a/frappe/api/v2.py b/frappe/api/v2.py index a36d422311..18417022f2 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -29,26 +29,26 @@ def handle_rpc_call(method: str, doctype: str | None = None): def get_doc_list(doctype: str): - if frappe.local.form_dict.get("fields"): - frappe.local.form_dict["fields"] = json.loads(frappe.local.form_dict["fields"]) + if frappe.form_dict.get("fields"): + frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) # set limit of records for frappe.get_list - frappe.local.form_dict.setdefault( + frappe.form_dict.setdefault( "limit_page_length", - frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, + frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, ) # convert strings to native types - only as_dict and debug accept bool for param in ["as_dict", "debug"]: - param_val = frappe.local.form_dict.get(param) + param_val = frappe.form_dict.get(param) if param_val is not None: - frappe.local.form_dict[param] = sbool(param_val) + frappe.form_dict[param] = sbool(param_val) # evaluate frappe.get_list - data = frappe.call(frappe.client.get_list, doctype, **frappe.local.form_dict) + data = frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) # set frappe.get_list result to response - frappe.local.response.update({"data": data}) + frappe.response.update({"data": data}) def create_doc(doctype: str): @@ -59,7 +59,7 @@ def create_doc(doctype: str): doc = frappe.get_doc(data).insert() # set response data - frappe.local.response.update({"data": doc.as_dict()}) + frappe.response.update({"data": doc.as_dict()}) def read_doc(doctype: str, name: str): @@ -67,7 +67,7 @@ def read_doc(doctype: str, name: str): if not doc.has_permission("read"): raise frappe.PermissionError doc.apply_fieldlevel_read_permissions() - frappe.local.response.update({"data": doc}) + frappe.response.update({"data": doc}) def update_doc(doctype: str, name: str): @@ -81,7 +81,7 @@ def update_doc(doctype: str, name: str): # Not checking permissions here because it's checked in doc.save doc.update(data) - frappe.local.response.update({"data": doc.save().as_dict()}) + frappe.response.update({"data": doc.save().as_dict()}) # check for child table doctype if doc.get("parenttype"): @@ -90,36 +90,36 @@ def update_doc(doctype: str, name: str): def delete_doc(doctype: str, name: str): frappe.delete_doc(doctype, name, ignore_missing=False) - frappe.local.response.http_status_code = 202 + frappe.response.http_status_code = 202 def execute_doc_method(doctype: str, name: str, method: str | None = None): - method = method or frappe.local.form_dict.pop("run_method") + method = method or frappe.form_dict.pop("run_method") doc = frappe.get_doc(doctype, name) doc.is_whitelisted(method) - if frappe.local.request.method == "GET": + if frappe.request.method == "GET": if not doc.has_permission("read"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) - elif frappe.local.request.method == "POST": + elif frappe.request.method == "POST": if not doc.has_permission("write"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.local.response.update({"data": doc.run_method(method, **frappe.local.form_dict)}) + frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) def get_request_form_data(): - if frappe.local.form_dict.data is None: - data = frappe.safe_decode(frappe.local.request.get_data()) + if frappe.form_dict.data is None: + data = frappe.safe_decode(frappe.request.get_data()) else: - data = frappe.local.form_dict.data + data = frappe.form_dict.data try: return frappe.parse_json(data) except ValueError: - return frappe.local.form_dict + return frappe.form_dict url_rules = [ From b57ca948abbb96301117bf8fbd28dbbc21e47432 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:27:55 +0530 Subject: [PATCH 15/44] refactor: common API methods to utils --- frappe/api/v1.py | 23 ++--------------------- frappe/api/v2.py | 22 ++-------------------- 2 files changed, 4 insertions(+), 41 deletions(-) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 9ce7b9bd71..5ffab5e047 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -4,38 +4,19 @@ from werkzeug.routing import Rule import frappe from frappe import _ +from frappe.api.utils import document_list from frappe.utils.data import sbool def handle_rpc_call(method: str): import frappe.handler - # TODO: inline this weird circular calls frappe.form_dict.cmd = method return frappe.handler.handle() def get_doc_list(doctype: str): - if frappe.form_dict.get("fields"): - frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) - - # set limit of records for frappe.get_list - frappe.form_dict.setdefault( - "limit_page_length", - frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, - ) - - # convert strings to native types - only as_dict and debug accept bool - for param in ["as_dict", "debug"]: - param_val = frappe.form_dict.get(param) - if param_val is not None: - frappe.form_dict[param] = sbool(param_val) - - # evaluate frappe.get_list - data = frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) - - # set frappe.get_list result to response - frappe.response.update({"data": data}) + frappe.response.update({"data": document_list(doctype)}) def create_doc(doctype: str): diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 18417022f2..d97d354b4b 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -5,6 +5,7 @@ from werkzeug.wrappers import Response import frappe from frappe import _ +from frappe.api.utils import document_list from frappe.utils.data import sbool @@ -29,26 +30,7 @@ def handle_rpc_call(method: str, doctype: str | None = None): def get_doc_list(doctype: str): - if frappe.form_dict.get("fields"): - frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) - - # set limit of records for frappe.get_list - frappe.form_dict.setdefault( - "limit_page_length", - frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, - ) - - # convert strings to native types - only as_dict and debug accept bool - for param in ["as_dict", "debug"]: - param_val = frappe.form_dict.get(param) - if param_val is not None: - frappe.form_dict[param] = sbool(param_val) - - # evaluate frappe.get_list - data = frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) - - # set frappe.get_list result to response - frappe.response.update({"data": data}) + frappe.response.update({"data": document_list(doctype)}) def create_doc(doctype: str): From 627b777a9ce7330c2daaa47c6e9120b3151d1bfc Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 21:36:11 +0530 Subject: [PATCH 16/44] refactor!: manual creation of response This contains minor breaking change where `delete` API call now returns `OK` in `data` key instead of `message`. --- frappe/api/__init__.py | 27 +++++++------- frappe/api/utils.py | 65 ++++++++++++++++++++++++++++++++ frappe/api/v1.py | 56 +++------------------------ frappe/api/v2.py | 81 +++++----------------------------------- frappe/tests/test_api.py | 2 +- 5 files changed, 95 insertions(+), 136 deletions(-) create mode 100644 frappe/api/utils.py diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index 39e95ba1c3..b0e08bb255 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -6,33 +6,32 @@ from werkzeug.wrappers import Request, Response import frappe import frappe.client -import frappe.handler from frappe import _ from frappe.utils.response import build_response def handle(request: Request): """ - Handler for `/api` methods + Entry point for `/api` methods. - ### Examples: + APIs are versioned using second part of path. + v1 -> `/api/v1/*` + v2 -> `/api/v2/*` - `/api/method/{methodname}` will call a whitelisted method + Different versions have different specification but broadly following things are supported: - `/api/resource/{doctype}` will query a table + - `/api/method/{methodname}` will call a whitelisted method + - `/api/resource/{doctype}` will query a table examples: - `?fields=["name", "owner"]` - `?filters=[["Task", "name", "like", "%005"]]` - `?limit_start=0` - `?limit_page_length=20` - - `/api/resource/{doctype}/{name}` will point to a resource - `GET` will return doclist + - `/api/resource/{doctype}/{name}` will point to a resource + `GET` will return document `POST` will insert `PUT` will update `DELETE` will delete - - `/api/resource/{doctype}/{name}?run_method={method}` will run a whitelisted controller method """ try: @@ -40,10 +39,12 @@ def handle(request: Request): except NotFound: # Wrap 404 - backward compatiblity raise frappe.DoesNotExistError - resp = endpoint(**arguments) - if isinstance(resp, Response): - return resp + data = endpoint(**arguments) + if isinstance(data, Response): + return data + if data is not None: + frappe.response["data"] = data return build_response("json") diff --git a/frappe/api/utils.py b/frappe/api/utils.py new file mode 100644 index 0000000000..38680dcdf7 --- /dev/null +++ b/frappe/api/utils.py @@ -0,0 +1,65 @@ +import json + +import frappe +from frappe.utils.data import sbool + + +def document_list(doctype: str): + if frappe.form_dict.get("fields"): + frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) + + # set limit of records for frappe.get_list + frappe.form_dict.setdefault( + "limit_page_length", + frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, + ) + + # convert strings to native types - only as_dict and debug accept bool + for param in ["as_dict", "debug"]: + param_val = frappe.form_dict.get(param) + if param_val is not None: + frappe.form_dict[param] = sbool(param_val) + + # evaluate frappe.get_list + return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) + + +def create_doc(doctype: str): + data = get_request_form_data() + data.pop("doctype", None) + return frappe.new_doc(doctype, **data).insert() + + +def update_doc(doctype: str, name: str): + data = get_request_form_data() + + doc = frappe.get_doc(doctype, name, for_update=True) + if "flags" in data: + del data["flags"] + + doc.update(data) + doc.save() + + # check for child table doctype + if doc.get("parenttype"): + frappe.get_doc(doc.parenttype, doc.parent).save() + + return doc + + +def delete_doc(doctype: str, name: str): + frappe.delete_doc(doctype, name, ignore_missing=False) + frappe.response.http_status_code = 202 + return "ok" + + +def get_request_form_data(): + if frappe.form_dict.data is None: + data = frappe.safe_decode(frappe.request.get_data()) + else: + data = frappe.form_dict.data + + try: + return frappe.parse_json(data) + except ValueError: + return frappe.form_dict diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 5ffab5e047..fcf8b80b8e 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -1,11 +1,8 @@ -import json - from werkzeug.routing import Rule import frappe from frappe import _ -from frappe.api.utils import document_list -from frappe.utils.data import sbool +from frappe.api.utils import create_doc, delete_doc, document_list, update_doc def handle_rpc_call(method: str): @@ -15,57 +12,16 @@ def handle_rpc_call(method: str): return frappe.handler.handle() -def get_doc_list(doctype: str): - frappe.response.update({"data": document_list(doctype)}) - - -def create_doc(doctype: str): - data = get_request_form_data() - data.update({"doctype": doctype}) - - # insert document from request data - doc = frappe.get_doc(data).insert() - - # set response data - frappe.response.update({"data": doc.as_dict()}) - - def read_doc(doctype: str, name: str): # Backward compatiblity if "run_method" in frappe.form_dict: - execute_doc_method(doctype, name) - return + return execute_doc_method(doctype, name) doc = frappe.get_doc(doctype, name) if not doc.has_permission("read"): raise frappe.PermissionError doc.apply_fieldlevel_read_permissions() - frappe.response.update({"data": doc}) - - -def update_doc(doctype: str, name: str): - data = get_request_form_data() - - doc = frappe.get_doc(doctype, name, for_update=True) - - if "flags" in data: - del data["flags"] - - # Not checking permissions here because it's checked in doc.save - doc.update(data) - - frappe.response.update({"data": doc.save().as_dict()}) - - # check for child table doctype - if doc.get("parenttype"): - frappe.get_doc(doc.parenttype, doc.parent).save() - - -def delete_doc(doctype: str, name: str): - # Not checking permissions here because it's checked in delete_doc - frappe.delete_doc(doctype, name, ignore_missing=False) - frappe.response.http_status_code = 202 - frappe.response.message = "ok" + return doc def execute_doc_method(doctype: str, name: str, method: str | None = None): @@ -76,13 +32,13 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): if frappe.request.method == "GET": if not doc.has_permission("read"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) + return doc.run_method(method, **frappe.form_dict) elif frappe.request.method == "POST": if not doc.has_permission("write"): frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) + return doc.run_method(method, **frappe.form_dict) def get_request_form_data(): @@ -99,7 +55,7 @@ def get_request_form_data(): url_rules = [ Rule("/method/", endpoint=handle_rpc_call), - Rule("/resource/", methods=["GET"], endpoint=get_doc_list), + Rule("/resource/", methods=["GET"], endpoint=document_list), Rule("/resource/", methods=["POST"], endpoint=create_doc), Rule("/resource//", methods=["GET"], endpoint=read_doc), Rule("/resource//", methods=["PUT"], endpoint=update_doc), diff --git a/frappe/api/v2.py b/frappe/api/v2.py index d97d354b4b..513d7b8d6a 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -1,12 +1,8 @@ -import json - from werkzeug.routing import Rule -from werkzeug.wrappers import Response import frappe from frappe import _ -from frappe.api.utils import document_list -from frappe.utils.data import sbool +from frappe.api.utils import create_doc, delete_doc, document_list, update_doc def handle_rpc_call(method: str, doctype: str | None = None): @@ -21,58 +17,14 @@ def handle_rpc_call(method: str, doctype: str | None = None): if method == "login": return - data = execute_cmd(method) - if isinstance(data, Response): - # method returns a response object, pass it on - return data - - frappe.response["data"] = data - - -def get_doc_list(doctype: str): - frappe.response.update({"data": document_list(doctype)}) - - -def create_doc(doctype: str): - data = get_request_form_data() - data.update({"doctype": doctype}) - - # insert document from request data - doc = frappe.get_doc(data).insert() - - # set response data - frappe.response.update({"data": doc.as_dict()}) + return execute_cmd(method) def read_doc(doctype: str, name: str): doc = frappe.get_doc(doctype, name) - if not doc.has_permission("read"): - raise frappe.PermissionError + doc.check_permission("read") doc.apply_fieldlevel_read_permissions() - frappe.response.update({"data": doc}) - - -def update_doc(doctype: str, name: str): - data = get_request_form_data() - - doc = frappe.get_doc(doctype, name, for_update=True) - - if "flags" in data: - del data["flags"] - - # Not checking permissions here because it's checked in doc.save - doc.update(data) - - frappe.response.update({"data": doc.save().as_dict()}) - - # check for child table doctype - if doc.get("parenttype"): - frappe.get_doc(doc.parenttype, doc.parent).save() - - -def delete_doc(doctype: str, name: str): - frappe.delete_doc(doctype, name, ignore_missing=False) - frappe.response.http_status_code = 202 + return doc def execute_doc_method(doctype: str, name: str, method: str | None = None): @@ -81,33 +33,18 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): doc.is_whitelisted(method) if frappe.request.method == "GET": - if not doc.has_permission("read"): - frappe.throw(_("Not permitted"), frappe.PermissionError) - frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) + doc.check_permission("read") + return doc.run_method(method, **frappe.form_dict) elif frappe.request.method == "POST": - if not doc.has_permission("write"): - frappe.throw(_("Not permitted"), frappe.PermissionError) - - frappe.response.update({"data": doc.run_method(method, **frappe.form_dict)}) - - -def get_request_form_data(): - if frappe.form_dict.data is None: - data = frappe.safe_decode(frappe.request.get_data()) - else: - data = frappe.form_dict.data - - try: - return frappe.parse_json(data) - except ValueError: - return frappe.form_dict + doc.check_permission("write") + return doc.run_method(method, **frappe.form_dict) url_rules = [ Rule("/method/", endpoint=handle_rpc_call), Rule("/method//", endpoint=handle_rpc_call), - Rule("/resource/", methods=["GET"], endpoint=get_doc_list), + Rule("/resource/", methods=["GET"], endpoint=document_list), Rule("/resource/", methods=["POST"], endpoint=create_doc), Rule("/resource//", methods=["GET"], endpoint=read_doc), Rule("/resource//", methods=["PUT"], endpoint=update_doc), diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 44e5d2d264..7ce30295a3 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -199,7 +199,7 @@ class TestResourceAPI(FrappeAPITestCase): doc_to_delete = choice(self.GENERATED_DOCUMENTS) response = self.delete(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") self.assertEqual(response.status_code, 202) - self.assertDictEqual(response.json, {"message": "ok"}) + self.assertDictEqual(response.json, {"data": "ok"}) response = self.get(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") self.assertEqual(response.status_code, 404) From 0c61e6174310333950757e3a6b36b4c696b4d0d7 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 22:47:03 +0530 Subject: [PATCH 17/44] fix: delete oauth stuff when deleting users --- frappe/core/doctype/user/user.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 1af7af72e5..f90e54c658 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -549,6 +549,10 @@ class User(Document): # delete user permissions frappe.db.delete("User Permission", {"user": self.name}) + # Delete OAuth data + frappe.db.delete("OAuth Authorization Code", {"user": self.name}) + frappe.db.delete("Token Cache", {"user": self.name}) + def before_rename(self, old_name, new_name, merge=False): frappe.clear_cache(user=old_name) self.validate_rename(old_name, new_name) From 275f16060ae6b6a627a11da8c8479683cbc10528 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 3 Sep 2023 22:52:14 +0530 Subject: [PATCH 18/44] fix: route ambiguitities and `/` in name v1 and v2: - Use path convertor to consume entire document which MIGHT contain `/` - Drop string convertor, as it's the default - Dont merge slashes - avoid consuming something that's part of name v2: - `SI/BLAH/BLAH/submit` is not simple to parse without ambiguitity - `SI/BLAH/BLAH/method/submit` removes ambiguitity for 99.99% cases. --- frappe/api/__init__.py | 1 + frappe/api/utils.py | 1 + frappe/api/v1.py | 14 +++++++------- frappe/api/v2.py | 18 ++++++++++-------- .../connected_app/test_connected_app.py | 2 +- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index b0e08bb255..9dc4ba63ac 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -60,4 +60,5 @@ API_URL_MAP = Map( Submount("/api/v2", v2_rules), ], strict_slashes=False, # Allows skipping trailing slashes + merge_slashes=False, ) diff --git a/frappe/api/utils.py b/frappe/api/utils.py index 38680dcdf7..fcca8d43d7 100644 --- a/frappe/api/utils.py +++ b/frappe/api/utils.py @@ -48,6 +48,7 @@ def update_doc(doctype: str, name: str): def delete_doc(doctype: str, name: str): + # TODO: child doc handling frappe.delete_doc(doctype, name, ignore_missing=False) frappe.response.http_status_code = 202 return "ok" diff --git a/frappe/api/v1.py b/frappe/api/v1.py index fcf8b80b8e..8aefd0f6f4 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -54,11 +54,11 @@ def get_request_form_data(): url_rules = [ - Rule("/method/", endpoint=handle_rpc_call), - Rule("/resource/", methods=["GET"], endpoint=document_list), - Rule("/resource/", methods=["POST"], endpoint=create_doc), - Rule("/resource//", methods=["GET"], endpoint=read_doc), - Rule("/resource//", methods=["PUT"], endpoint=update_doc), - Rule("/resource//", methods=["DELETE"], endpoint=delete_doc), - Rule("/resource//", methods=["POST"], endpoint=execute_doc_method), + Rule("/method/", endpoint=handle_rpc_call), + Rule("/resource/", methods=["GET"], endpoint=document_list), + Rule("/resource/", methods=["POST"], endpoint=create_doc), + Rule("/resource///", methods=["GET"], endpoint=read_doc), + Rule("/resource///", methods=["PUT"], endpoint=update_doc), + Rule("/resource///", methods=["DELETE"], endpoint=delete_doc), + Rule("/resource///", methods=["POST"], endpoint=execute_doc_method), ] diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 513d7b8d6a..b8dc192308 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -42,15 +42,17 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): url_rules = [ - Rule("/method/", endpoint=handle_rpc_call), - Rule("/method//", endpoint=handle_rpc_call), - Rule("/resource/", methods=["GET"], endpoint=document_list), - Rule("/resource/", methods=["POST"], endpoint=create_doc), - Rule("/resource//", methods=["GET"], endpoint=read_doc), - Rule("/resource//", methods=["PUT"], endpoint=update_doc), - Rule("/resource//", methods=["DELETE"], endpoint=delete_doc), + Rule("/method/", endpoint=handle_rpc_call), + Rule("/method//", endpoint=handle_rpc_call), + Rule("/resource/", methods=["GET"], endpoint=document_list), + # TODO: bulk insert with array + Rule("/resource/", methods=["POST"], endpoint=create_doc), + # TODO: get_value + Rule("/resource///", methods=["GET"], endpoint=read_doc), + Rule("/resource///", methods=["PUT"], endpoint=update_doc), + Rule("/resource///", methods=["DELETE"], endpoint=delete_doc), Rule( - "/resource///", + "/resource///method//", methods=["GET", "POST"], endpoint=execute_doc_method, ), diff --git a/frappe/integrations/doctype/connected_app/test_connected_app.py b/frappe/integrations/doctype/connected_app/test_connected_app.py index 88441db6b2..49ed6236ff 100644 --- a/frappe/integrations/doctype/connected_app/test_connected_app.py +++ b/frappe/integrations/doctype/connected_app/test_connected_app.py @@ -126,7 +126,7 @@ class TestConnectedApp(FrappeTestCase): def delete_if_exists(attribute): doc = getattr(self, attribute, None) if doc: - doc.delete() + doc.delete(force=True) delete_if_exists("token_cache") delete_if_exists("connected_app") From 825692128f7296b8825314dfbb4aef32d3947958 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 11:42:49 +0530 Subject: [PATCH 19/44] chore: separate out v1 and v2 Any common utils end up becoming a problem, duplicating code here is better than coupling potentially breaking behaviour --- frappe/api/utils.py | 66 ------------------------------------------- frappe/api/v1.py | 54 ++++++++++++++++++++++++++++++++++- frappe/api/v2.py | 68 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 118 insertions(+), 70 deletions(-) diff --git a/frappe/api/utils.py b/frappe/api/utils.py index fcca8d43d7..e69de29bb2 100644 --- a/frappe/api/utils.py +++ b/frappe/api/utils.py @@ -1,66 +0,0 @@ -import json - -import frappe -from frappe.utils.data import sbool - - -def document_list(doctype: str): - if frappe.form_dict.get("fields"): - frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) - - # set limit of records for frappe.get_list - frappe.form_dict.setdefault( - "limit_page_length", - frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, - ) - - # convert strings to native types - only as_dict and debug accept bool - for param in ["as_dict", "debug"]: - param_val = frappe.form_dict.get(param) - if param_val is not None: - frappe.form_dict[param] = sbool(param_val) - - # evaluate frappe.get_list - return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) - - -def create_doc(doctype: str): - data = get_request_form_data() - data.pop("doctype", None) - return frappe.new_doc(doctype, **data).insert() - - -def update_doc(doctype: str, name: str): - data = get_request_form_data() - - doc = frappe.get_doc(doctype, name, for_update=True) - if "flags" in data: - del data["flags"] - - doc.update(data) - doc.save() - - # check for child table doctype - if doc.get("parenttype"): - frappe.get_doc(doc.parenttype, doc.parent).save() - - return doc - - -def delete_doc(doctype: str, name: str): - # TODO: child doc handling - frappe.delete_doc(doctype, name, ignore_missing=False) - frappe.response.http_status_code = 202 - return "ok" - - -def get_request_form_data(): - if frappe.form_dict.data is None: - data = frappe.safe_decode(frappe.request.get_data()) - else: - data = frappe.form_dict.data - - try: - return frappe.parse_json(data) - except ValueError: - return frappe.form_dict diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 8aefd0f6f4..07aec5aee2 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -1,8 +1,30 @@ +import json + from werkzeug.routing import Rule import frappe from frappe import _ -from frappe.api.utils import create_doc, delete_doc, document_list, update_doc +from frappe.utils.data import sbool + + +def document_list(doctype: str): + if frappe.form_dict.get("fields"): + frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) + + # set limit of records for frappe.get_list + frappe.form_dict.setdefault( + "limit_page_length", + frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, + ) + + # convert strings to native types - only as_dict and debug accept bool + for param in ["as_dict", "debug"]: + param_val = frappe.form_dict.get(param) + if param_val is not None: + frappe.form_dict[param] = sbool(param_val) + + # evaluate frappe.get_list + return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) def handle_rpc_call(method: str): @@ -12,6 +34,36 @@ def handle_rpc_call(method: str): return frappe.handler.handle() +def create_doc(doctype: str): + data = get_request_form_data() + data.pop("doctype", None) + return frappe.new_doc(doctype, **data).insert() + + +def update_doc(doctype: str, name: str): + data = get_request_form_data() + + doc = frappe.get_doc(doctype, name, for_update=True) + if "flags" in data: + del data["flags"] + + doc.update(data) + doc.save() + + # check for child table doctype + if doc.get("parenttype"): + frappe.get_doc(doc.parenttype, doc.parent).save() + + return doc + + +def delete_doc(doctype: str, name: str): + # TODO: child doc handling + frappe.delete_doc(doctype, name, ignore_missing=False) + frappe.response.http_status_code = 202 + return "ok" + + def read_doc(doctype: str, name: str): # Backward compatiblity if "run_method" in frappe.form_dict: diff --git a/frappe/api/v2.py b/frappe/api/v2.py index b8dc192308..3089e139a1 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -1,8 +1,10 @@ +import json + from werkzeug.routing import Rule import frappe from frappe import _ -from frappe.api.utils import create_doc, delete_doc, document_list, update_doc +from frappe.utils.data import sbool def handle_rpc_call(method: str, doctype: str | None = None): @@ -27,6 +29,68 @@ def read_doc(doctype: str, name: str): return doc +def document_list(doctype: str): + if frappe.form_dict.get("fields"): + frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) + + # set limit of records for frappe.get_list + frappe.form_dict.setdefault( + "limit_page_length", + frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, + ) + + # convert strings to native types - only as_dict and debug accept bool + for param in ["as_dict", "debug"]: + param_val = frappe.form_dict.get(param) + if param_val is not None: + frappe.form_dict[param] = sbool(param_val) + + # evaluate frappe.get_list + return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) + + +def create_doc(doctype: str): + data = get_request_form_data() + data.pop("doctype", None) + return frappe.new_doc(doctype, **data).insert() + + +def update_doc(doctype: str, name: str): + data = get_request_form_data() + + doc = frappe.get_doc(doctype, name, for_update=True) + if "flags" in data: + del data["flags"] + + doc.update(data) + doc.save() + + # check for child table doctype + if doc.get("parenttype"): + frappe.get_doc(doc.parenttype, doc.parent).save() + + return doc + + +def delete_doc(doctype: str, name: str): + # TODO: child doc handling + frappe.delete_doc(doctype, name, ignore_missing=False) + frappe.response.http_status_code = 202 + return "ok" + + +def get_request_form_data(): + if frappe.form_dict.data is None: + data = frappe.safe_decode(frappe.request.get_data()) + else: + data = frappe.form_dict.data + + try: + return frappe.parse_json(data) + except ValueError: + return frappe.form_dict + + def execute_doc_method(doctype: str, name: str, method: str | None = None): method = method or frappe.form_dict.pop("run_method") doc = frappe.get_doc(doctype, name) @@ -45,9 +109,7 @@ url_rules = [ Rule("/method/", endpoint=handle_rpc_call), Rule("/method//", endpoint=handle_rpc_call), Rule("/resource/", methods=["GET"], endpoint=document_list), - # TODO: bulk insert with array Rule("/resource/", methods=["POST"], endpoint=create_doc), - # TODO: get_value Rule("/resource///", methods=["GET"], endpoint=read_doc), Rule("/resource///", methods=["PUT"], endpoint=update_doc), Rule("/resource///", methods=["DELETE"], endpoint=delete_doc), From 985c897c95dd39f2d38690a5b32fdbc7019d2902 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 11:46:08 +0530 Subject: [PATCH 20/44] refactor: simplify request form data --- frappe/api/v2.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 3089e139a1..8d16a260d1 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -50,13 +50,13 @@ def document_list(doctype: str): def create_doc(doctype: str): - data = get_request_form_data() + data = frappe.form_dict data.pop("doctype", None) return frappe.new_doc(doctype, **data).insert() def update_doc(doctype: str, name: str): - data = get_request_form_data() + data = frappe.form_dict doc = frappe.get_doc(doctype, name, for_update=True) if "flags" in data: @@ -79,18 +79,6 @@ def delete_doc(doctype: str, name: str): return "ok" -def get_request_form_data(): - if frappe.form_dict.data is None: - data = frappe.safe_decode(frappe.request.get_data()) - else: - data = frappe.form_dict.data - - try: - return frappe.parse_json(data) - except ValueError: - return frappe.form_dict - - def execute_doc_method(doctype: str, name: str, method: str | None = None): method = method or frappe.form_dict.pop("run_method") doc = frappe.get_doc(doctype, name) From 947e7f68647854ba380d8a8b336b77322664bdd9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 12:18:53 +0530 Subject: [PATCH 21/44] test: refactor API test cases for paramterization --- frappe/tests/test_api.py | 96 ++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 7ce30295a3..3ad76885f1 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -1,20 +1,21 @@ import json import sys from contextlib import contextmanager +from functools import cached_property from random import choice from threading import Thread from time import time from unittest.mock import patch +from urllib.parse import urljoin import requests from filetype import guess_mime -from semantic_version import Version from werkzeug.test import TestResponse import frappe from frappe.installer import update_site_config from frappe.tests.utils import FrappeTestCase, patch_hooks -from frappe.utils import cint, get_site_url, get_test_client +from frappe.utils import cint, get_test_client, get_url try: _site = frappe.local.site @@ -74,24 +75,32 @@ class ThreadWithReturnValue(Thread): class FrappeAPITestCase(FrappeTestCase): - SITE = frappe.local.site - SITE_URL = get_site_url(SITE) - RESOURCE_URL = f"{SITE_URL}/api/resource" + PREFIX = "api" TEST_CLIENT = get_test_client() @property + def site_url(self): + return get_url() + + def resource_path(self, *parts): + return self.get_path("resource", *parts) + + def method_path(self, *method): + return self.get_path("method", *method) + + def get_path(self, *parts): + return urljoin(self.site_url, "/".join((self.PREFIX, *parts))) + + @cached_property def sid(self) -> str: - if not getattr(self, "_sid", None): - from frappe.auth import CookieManager, LoginManager - from frappe.utils import set_request + from frappe.auth import CookieManager, LoginManager + from frappe.utils import set_request - set_request(path="/") - frappe.local.cookie_manager = CookieManager() - frappe.local.login_manager = LoginManager() - frappe.local.login_manager.login_as("Administrator") - self._sid = frappe.session.sid - - return self._sid + set_request(path="/") + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + frappe.local.login_manager.login_as("Administrator") + return frappe.session.sid def get(self, path: str, params: dict | None = None, **kwargs) -> TestResponse: return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}) @@ -126,31 +135,31 @@ class TestResourceAPI(FrappeAPITestCase): def test_unauthorized_call(self): # test 1: fetch documents without auth - response = requests.get(f"{self.RESOURCE_URL}/{self.DOCTYPE}") + response = requests.get(self.resource_path(self.DOCTYPE)) self.assertEqual(response.status_code, 403) def test_get_list(self): # test 2: fetch documents without params - response = self.get(f"/api/resource/{self.DOCTYPE}", {"sid": self.sid}) + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid}) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIn("data", response.json) def test_get_list_limit(self): # test 3: fetch data with limit - response = self.get(f"/api/resource/{self.DOCTYPE}", {"sid": self.sid, "limit": 2}) + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "limit": 2}) self.assertEqual(response.status_code, 200) self.assertEqual(len(response.json["data"]), 2) def test_get_list_dict(self): # test 4: fetch response as (not) dict - response = self.get(f"/api/resource/{self.DOCTYPE}", {"sid": self.sid, "as_dict": True}) + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": True}) json = frappe._dict(response.json) self.assertEqual(response.status_code, 200) self.assertIsInstance(json.data, list) self.assertIsInstance(json.data[0], dict) - response = self.get(f"/api/resource/{self.DOCTYPE}", {"sid": self.sid, "as_dict": False}) + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": False}) json = frappe._dict(response.json) self.assertEqual(response.status_code, 200) self.assertIsInstance(json.data, list) @@ -158,7 +167,8 @@ class TestResourceAPI(FrappeAPITestCase): def test_get_list_debug(self): # test 5: fetch response with debug - response = self.get(f"/api/resource/{self.DOCTYPE}", {"sid": self.sid, "debug": True}) + with suppress_stdout(): + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "debug": True}) self.assertEqual(response.status_code, 200) self.assertIn("exc", response.json) self.assertIsInstance(response.json["exc"], str) @@ -167,55 +177,56 @@ class TestResourceAPI(FrappeAPITestCase): def test_get_list_fields(self): # test 6: fetch response with fields response = self.get( - f"/api/resource/{self.DOCTYPE}", {"sid": self.sid, "fields": '["description"]'} + self.resource_path(self.DOCTYPE), {"sid": self.sid, "fields": '["description"]'} ) self.assertEqual(response.status_code, 200) json = frappe._dict(response.json) self.assertIn("description", json.data[0]) def test_create_document(self): - # test 7: POST method on /api/resource to create doc + # test 7: POST method on {self.resource_path} to create doc data = {"description": frappe.mock("paragraph"), "sid": self.sid} - response = self.post(f"/api/resource/{self.DOCTYPE}", data) + response = self.post(self.resource_path(self.DOCTYPE), data) self.assertEqual(response.status_code, 200) docname = response.json["data"]["name"] self.assertIsInstance(docname, str) self.GENERATED_DOCUMENTS.append(docname) def test_update_document(self): - # test 8: PUT method on /api/resource to update doc + # test 8: PUT method on {self.resource_path} to update doc generated_desc = frappe.mock("paragraph") data = {"description": generated_desc, "sid": self.sid} random_doc = choice(self.GENERATED_DOCUMENTS) desc_before_update = frappe.db.get_value(self.DOCTYPE, random_doc, "description") - response = self.put(f"/api/resource/{self.DOCTYPE}/{random_doc}", data=data) + response = self.put(self.resource_path(self.DOCTYPE, random_doc), data=data) self.assertEqual(response.status_code, 200) self.assertNotEqual(response.json["data"]["description"], desc_before_update) self.assertEqual(response.json["data"]["description"], generated_desc) def test_delete_document(self): - # test 9: DELETE method on /api/resource + # test 9: DELETE method on {self.resource_path} doc_to_delete = choice(self.GENERATED_DOCUMENTS) - response = self.delete(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") + response = self.delete(self.resource_path(self.DOCTYPE, doc_to_delete)) self.assertEqual(response.status_code, 202) self.assertDictEqual(response.json, {"data": "ok"}) - response = self.get(f"/api/resource/{self.DOCTYPE}/{doc_to_delete}") + response = self.get(self.resource_path(self.DOCTYPE, doc_to_delete)) self.assertEqual(response.status_code, 404) self.GENERATED_DOCUMENTS.remove(doc_to_delete) non_existent_doc = frappe.generate_hash(length=12) with suppress_stdout(): - response = self.delete(f"/api/resource/{self.DOCTYPE}/{non_existent_doc}") + response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) self.assertEqual(response.status_code, 404) self.assertDictEqual(response.json, {}) def test_run_doc_method(self): # test 10: Run whitelisted method on doc via /api/resource # status_code is 403 if no other tests are run before this - it's not logged in - self.post("/api/resource/Website Theme/Standard", {"run_method": "get_apps"}) - response = self.get("/api/resource/Website Theme/Standard", {"run_method": "get_apps"}) + self.post(self.resource_path("Website Theme", "Standard"), {"run_method": "get_apps"}) + response = self.get(self.resource_path("Website Theme", "Standard"), {"run_method": "get_apps"}) + self.assertIn(response.status_code, (403, 200)) if response.status_code == 403: @@ -235,8 +246,6 @@ class TestResourceAPI(FrappeAPITestCase): class TestMethodAPI(FrappeAPITestCase): - METHOD_PATH = "/api/method" - def setUp(self): if self._testMethodName == "test_auth_cycle": from frappe.core.doctype.user.user import generate_keys @@ -246,14 +255,14 @@ class TestMethodAPI(FrappeAPITestCase): def test_ping(self): # test 2: test for /api/method/ping - response = self.get(f"{self.METHOD_PATH}/ping") + response = self.get(self.method_path("ping")) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertEqual(response.json["message"], "pong") def test_get_user_info(self): # test 3: test for /api/method/frappe.realtime.get_user_info - response = self.get(f"{self.METHOD_PATH}/frappe.realtime.get_user_info") + response = self.get(self.method_path("frappe.realtime.get_user_info")) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIn(response.json.get("message").get("user"), ("Administrator", "Guest")) @@ -264,7 +273,7 @@ class TestMethodAPI(FrappeAPITestCase): user = frappe.get_doc("User", "Administrator") api_key, api_secret = user.api_key, user.get_password("api_secret") authorization_token = f"{api_key}:{api_secret}" - response = self.get("/api/method/frappe.auth.get_logged_user") + response = self.get(self.method_path("frappe.auth.get_logged_user")) self.assertEqual(response.status_code, 200) self.assertEqual(response.json["message"], "Administrator") @@ -272,9 +281,9 @@ class TestMethodAPI(FrappeAPITestCase): authorization_token = None def test_404s(self): - response = self.get("/api/rest", {"sid": self.sid}) + response = self.get(self.get_path("rest"), {"sid": self.sid}) self.assertEqual(response.status_code, 404) - response = self.get("/api/resource/User/NonExistent@s.com", {"sid": self.sid}) + response = self.get(self.resource_path("User", "NonExistent@s.com"), {"sid": self.sid}) self.assertEqual(response.status_code, 404) @@ -282,8 +291,6 @@ class TestReadOnlyMode(FrappeAPITestCase): """During migration if read only mode can be enabled. Test if reads work well and writes are blocked""" - REQ_PATH = "/api/resource/ToDo" - @classmethod def setUpClass(cls): super().setUpClass() @@ -293,13 +300,16 @@ class TestReadOnlyMode(FrappeAPITestCase): update_site_config("maintenance_mode", 1) def test_reads(self): - response = self.get(self.REQ_PATH, {"sid": self.sid}) + response = self.get(self.resource_path("ToDo"), {"sid": self.sid}) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIsInstance(response.json["data"], list) def test_blocked_writes(self): - response = self.post(self.REQ_PATH, {"description": frappe.mock("paragraph"), "sid": self.sid}) + with suppress_stdout(): + response = self.post( + self.resource_path("ToDo"), {"description": frappe.mock("paragraph"), "sid": self.sid} + ) self.assertEqual(response.status_code, 503) self.assertEqual(response.json["exc_type"], "InReadOnlyMode") From 0a244b10fcbf158c1e226b98168d2501b0f4776b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 13:05:25 +0530 Subject: [PATCH 22/44] test: basic tests v2 API --- frappe/__init__.py | 2 +- frappe/core/doctype/user/user.py | 4 +- frappe/tests/test_api.py | 121 +++++++++++++++++++++++++++---- 3 files changed, 109 insertions(+), 18 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 43c33d436b..b3847143e9 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -169,7 +169,7 @@ lang = local("lang") # This if block is never executed when running the code. It is only used for # telling static code analyzer where to find dynamically defined attributes. -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no cover from werkzeug.wrappers import Request from frappe.database.mariadb.database import MariaDBDatabase diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index f90e54c658..f22c62050e 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -779,7 +779,7 @@ def get_timezones(): @frappe.whitelist() -def get_all_roles(arg=None): +def get_all_roles(): """return all roles""" active_domains = frappe.get_active_domains() @@ -793,7 +793,7 @@ def get_all_roles(arg=None): order_by="name", ) - return [role.get("name") for role in roles] + return sorted([role.get("name") for role in roles]) @frappe.whitelist() diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 3ad76885f1..0d2565303b 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -13,6 +13,7 @@ from filetype import guess_mime from werkzeug.test import TestResponse import frappe +from frappe.core.doctype.user.user import get_timezones from frappe.installer import update_site_config from frappe.tests.utils import FrappeTestCase, patch_hooks from frappe.utils import cint, get_test_client, get_url @@ -74,8 +75,24 @@ class ThreadWithReturnValue(Thread): return self._return +def parameterize(*api_versions): + def decorator(f): + def wrapper(*args, **kwargs): + original_version = args[0].version + try: + for api_version in api_versions: + args[0].version = api_version + f(*args, **kwargs) + finally: + args[0].version = original_version + + return wrapper + + return decorator + + class FrappeAPITestCase(FrappeTestCase): - PREFIX = "api" + version = "" # Empty implies v1 TEST_CLIENT = get_test_client() @property @@ -89,7 +106,7 @@ class FrappeAPITestCase(FrappeTestCase): return self.get_path("method", *method) def get_path(self, *parts): - return urljoin(self.site_url, "/".join((self.PREFIX, *parts))) + return urljoin(self.site_url, "/".join(("api", self.version, *parts))) @cached_property def sid(self) -> str: @@ -103,13 +120,13 @@ class FrappeAPITestCase(FrappeTestCase): return frappe.session.sid def get(self, path: str, params: dict | None = None, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"data": params, **kwargs}) + return make_request(target=self.TEST_CLIENT.get, args=(path,), kwargs={"json": params, **kwargs}) def post(self, path, data, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.post, args=(path,), kwargs={"data": data, **kwargs}) + return make_request(target=self.TEST_CLIENT.post, args=(path,), kwargs={"json": data, **kwargs}) def put(self, path, data, **kwargs) -> TestResponse: - return make_request(target=self.TEST_CLIENT.put, args=(path,), kwargs={"data": data, **kwargs}) + return make_request(target=self.TEST_CLIENT.put, args=(path,), kwargs={"json": data, **kwargs}) def delete(self, path, **kwargs) -> TestResponse: return make_request(target=self.TEST_CLIENT.delete, args=(path,), kwargs=kwargs) @@ -133,11 +150,13 @@ class TestResourceAPI(FrappeAPITestCase): frappe.delete_doc_if_exists(cls.DOCTYPE, name) frappe.db.commit() + @parameterize("", "v1", "v2") def test_unauthorized_call(self): # test 1: fetch documents without auth response = requests.get(self.resource_path(self.DOCTYPE)) self.assertEqual(response.status_code, 403) + @parameterize("", "v1", "v2") def test_get_list(self): # test 2: fetch documents without params response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid}) @@ -145,12 +164,14 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(response.json, dict) self.assertIn("data", response.json) + @parameterize("", "v1", "v2") def test_get_list_limit(self): # test 3: fetch data with limit response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "limit": 2}) self.assertEqual(response.status_code, 200) self.assertEqual(len(response.json["data"]), 2) + @parameterize("", "v1", "v2") def test_get_list_dict(self): # test 4: fetch response as (not) dict response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": True}) @@ -165,6 +186,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(json.data, list) self.assertIsInstance(json.data[0], list) + @parameterize("", "v1", "v2") def test_get_list_debug(self): # test 5: fetch response with debug with suppress_stdout(): @@ -174,6 +196,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(response.json["exc"], str) self.assertIsInstance(eval(response.json["exc"]), list) + @parameterize("", "v1", "v2") def test_get_list_fields(self): # test 6: fetch response with fields response = self.get( @@ -183,6 +206,7 @@ class TestResourceAPI(FrappeAPITestCase): json = frappe._dict(response.json) self.assertIn("description", json.data[0]) + @parameterize("", "v1", "v2") def test_create_document(self): # test 7: POST method on {self.resource_path} to create doc data = {"description": frappe.mock("paragraph"), "sid": self.sid} @@ -192,18 +216,21 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(docname, str) self.GENERATED_DOCUMENTS.append(docname) + @parameterize("", "v1", "v2") def test_update_document(self): # test 8: PUT method on {self.resource_path} to update doc generated_desc = frappe.mock("paragraph") data = {"description": generated_desc, "sid": self.sid} random_doc = choice(self.GENERATED_DOCUMENTS) - desc_before_update = frappe.db.get_value(self.DOCTYPE, random_doc, "description") response = self.put(self.resource_path(self.DOCTYPE, random_doc), data=data) self.assertEqual(response.status_code, 200) - self.assertNotEqual(response.json["data"]["description"], desc_before_update) self.assertEqual(response.json["data"]["description"], generated_desc) + response = self.get(self.resource_path(self.DOCTYPE, random_doc)) + self.assertEqual(response.json["data"]["description"], generated_desc) + + @parameterize("", "v1", "v2") def test_delete_document(self): # test 9: DELETE method on {self.resource_path} doc_to_delete = choice(self.GENERATED_DOCUMENTS) @@ -221,6 +248,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertEqual(response.status_code, 404) self.assertDictEqual(response.json, {}) + @parameterize("", "v1") def test_run_doc_method(self): # test 10: Run whitelisted method on doc via /api/resource # status_code is 403 if no other tests are run before this - it's not logged in @@ -245,14 +273,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(data[0], dict) -class TestMethodAPI(FrappeAPITestCase): - def setUp(self): - if self._testMethodName == "test_auth_cycle": - from frappe.core.doctype.user.user import generate_keys - - generate_keys("Administrator") - frappe.db.commit() - +class TestMethodAPIV1(FrappeAPITestCase): def test_ping(self): # test 2: test for /api/method/ping response = self.get(self.method_path("ping")) @@ -270,6 +291,7 @@ class TestMethodAPI(FrappeAPITestCase): def test_auth_cycle(self): # test 4: Pass authorization token in request global authorization_token + generate_admin_keys() user = frappe.get_doc("User", "Administrator") api_key, api_secret = user.api_key, user.get_password("api_secret") authorization_token = f"{api_key}:{api_secret}" @@ -287,6 +309,66 @@ class TestMethodAPI(FrappeAPITestCase): self.assertEqual(response.status_code, 404) +class TestResourceAPIV2(FrappeAPITestCase): + version = "v2" + + def setUp(self) -> None: + self.post(self.method_path("login"), {"sid": self.sid}) + return super().setUp() + + def test_execute_doc_method(self): + response = self.get(self.resource_path("Website Theme", "Standard", "method", "get_apps")) + self.assertEqual(response.json["data"][0]["name"], "frappe") + + +class TestMethodAPIV2(FrappeAPITestCase): + version = "v2" + + def test_ping(self): + # test 2: test for /api/method/ping + response = self.get(self.method_path("ping")) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertEqual(response.json["data"], "pong") + + def test_get_user_info(self): + # test 3: test for /api/method/frappe.realtime.get_user_info + response = self.get(self.method_path("frappe.realtime.get_user_info")) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertIn(response.json.get("data").get("user"), ("Administrator", "Guest")) + + def test_auth_cycle(self): + # test 4: Pass authorization token in request + global authorization_token + + generate_admin_keys() + user = frappe.get_doc("User", "Administrator") + api_key, api_secret = user.api_key, user.get_password("api_secret") + authorization_token = f"{api_key}:{api_secret}" + response = self.get(self.method_path("frappe.auth.get_logged_user")) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"], "Administrator") + + authorization_token = None + + def test_404s(self): + response = self.get(self.get_path("rest"), {"sid": self.sid}) + self.assertEqual(response.status_code, 404) + response = self.get(self.resource_path("User", "NonExistent@s.com"), {"sid": self.sid}) + self.assertEqual(response.status_code, 404) + + def test_shorthand_controller_methods(self): + shorthand_response = self.get(self.method_path("User", "get_all_roles"), {"sid": self.sid}) + self.assertIn("Blogger", shorthand_response.json["data"]) + + expanded_response = self.get( + self.method_path("frappe.core.doctype.user.user.get_all_roles"), {"sid": self.sid} + ) + self.assertEqual(expanded_response.data, shorthand_response.data) + + class TestReadOnlyMode(FrappeAPITestCase): """During migration if read only mode can be enabled. Test if reads work well and writes are blocked""" @@ -299,12 +381,14 @@ class TestReadOnlyMode(FrappeAPITestCase): # XXX: this has potential to crumble rest of the test suite. update_site_config("maintenance_mode", 1) + @parameterize("", "v1", "v2") def test_reads(self): response = self.get(self.resource_path("ToDo"), {"sid": self.sid}) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIsInstance(response.json["data"], list) + @parameterize("", "v1", "v2") def test_blocked_writes(self): with suppress_stdout(): response = self.post( @@ -381,3 +465,10 @@ class TestResponse(FrappeAPITestCase): self.assertEqual(response.status_code, 200) self.assertIn("text/csv", response.headers["content-type"]) self.assertGreater(cint(response.headers["content-length"]), 0) + + +def generate_admin_keys(): + from frappe.core.doctype.user.user import generate_keys + + generate_keys("Administrator") + frappe.db.commit() From e96ecab00e17ef4aeee5efdb1bc3c23791fe9da0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 15:18:40 +0530 Subject: [PATCH 23/44] refactor: OAuth flow without breaking routing convention Appending `/connected-app` after method breaks routing. --- .../doctype/connected_app/connected_app.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/frappe/integrations/doctype/connected_app/connected_app.py b/frappe/integrations/doctype/connected_app/connected_app.py index d571b2ba00..d6b173d040 100644 --- a/frappe/integrations/doctype/connected_app/connected_app.py +++ b/frappe/integrations/doctype/connected_app/connected_app.py @@ -48,7 +48,8 @@ class ConnectedApp(Document): def validate(self): base_url = frappe.utils.get_url() callback_path = ( - "/api/method/frappe.integrations.doctype.connected_app.connected_app.callback/" + self.name + "/api/method/frappe.integrations.doctype.connected_app.connected_app.callback" + + f"?app={self.name}" ) self.redirect_uri = urljoin(base_url, callback_path) @@ -148,7 +149,7 @@ class ConnectedApp(Document): @frappe.whitelist(methods=["GET"], allow_guest=True) -def callback(code=None, state=None): +def callback(code=None, state=None, app=None): """Handle client's code. Called during the oauthorization flow by the remote oAuth2 server to @@ -161,11 +162,7 @@ def callback(code=None, state=None): frappe.local.response["location"] = "/login?" + urlencode({"redirect-to": frappe.request.url}) return - path = frappe.request.path[1:].split("/") - if len(path) != 4 or not path[3]: - frappe.throw(_("Invalid Parameters.")) - - connected_app = frappe.get_doc("Connected App", path[3]) + connected_app = frappe.get_doc("Connected App", app) token_cache = frappe.get_doc("Token Cache", connected_app.name + "-" + frappe.session.user) if state != token_cache.state: From 10ace97ebb9d651d4385c5903f00b04823b969ab Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 15:25:03 +0530 Subject: [PATCH 24/44] fix: backward compatiblity for method calls in v1 Preserves complete path just like before. This broke oauth as oauth code was expecting 4th part in path even though it's never been supported in API methods --- frappe/api/v1.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/api/v1.py b/frappe/api/v1.py index 07aec5aee2..d2758f45d5 100644 --- a/frappe/api/v1.py +++ b/frappe/api/v1.py @@ -30,6 +30,8 @@ def document_list(doctype: str): def handle_rpc_call(method: str): import frappe.handler + method = method.split("/")[0] # for backward compatiblity + frappe.form_dict.cmd = method return frappe.handler.handle() @@ -106,7 +108,7 @@ def get_request_form_data(): url_rules = [ - Rule("/method/", endpoint=handle_rpc_call), + Rule("/method/", endpoint=handle_rpc_call), Rule("/resource/", methods=["GET"], endpoint=document_list), Rule("/resource/", methods=["POST"], endpoint=create_doc), Rule("/resource///", methods=["GET"], endpoint=read_doc), From e63f0c895db1e44789cac1081bd2d19c3cfe9bde Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 15:45:39 +0530 Subject: [PATCH 25/44] refactor: `resource` => `document` This lets us create three types of APIs: - Document APIs that operate on documents - DocType APIs that operate on collections - list, count, search - Method APIs that are RPC calls --- frappe/api/v2.py | 24 +++++++++++++++++------- frappe/tests/test_api.py | 9 ++++++++- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 8d16a260d1..ba122efb87 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -1,3 +1,12 @@ +"""REST API v2 + +This file defines routes and implementation for REST API. + +Note: + - All functions in this file should be treated as "whitelisted" as they are exposed via routes + - None of the functions present here should be called from python code, their location and + internal implementation can change without treating it as "breaking change". +""" import json from werkzeug.routing import Rule @@ -12,11 +21,12 @@ def handle_rpc_call(method: str, doctype: str | None = None): from frappe.modules.utils import load_doctype_module if doctype: - # Expand to run actual method + # Expand to run actual method from doctype controller module = load_doctype_module(doctype) method = module.__name__ + "." + method if method == "login": + # Login works implicitly right now. return return execute_cmd(method) @@ -96,13 +106,13 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): url_rules = [ Rule("/method/", endpoint=handle_rpc_call), Rule("/method//", endpoint=handle_rpc_call), - Rule("/resource/", methods=["GET"], endpoint=document_list), - Rule("/resource/", methods=["POST"], endpoint=create_doc), - Rule("/resource///", methods=["GET"], endpoint=read_doc), - Rule("/resource///", methods=["PUT"], endpoint=update_doc), - Rule("/resource///", methods=["DELETE"], endpoint=delete_doc), + Rule("/document/", methods=["GET"], endpoint=document_list), + Rule("/document/", methods=["POST"], endpoint=create_doc), + Rule("/document///", methods=["GET"], endpoint=read_doc), + Rule("/document///", methods=["PUT"], endpoint=update_doc), + Rule("/document///", methods=["DELETE"], endpoint=delete_doc), Rule( - "/resource///method//", + "/document///method//", methods=["GET", "POST"], endpoint=execute_doc_method, ), diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 0d2565303b..db097638fa 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -91,6 +91,13 @@ def parameterize(*api_versions): return decorator +resource_key = { + "": "resource", + "v1": "resource", + "v2": "document", +} + + class FrappeAPITestCase(FrappeTestCase): version = "" # Empty implies v1 TEST_CLIENT = get_test_client() @@ -100,7 +107,7 @@ class FrappeAPITestCase(FrappeTestCase): return get_url() def resource_path(self, *parts): - return self.get_path("resource", *parts) + return self.get_path(resource_key[self.version], *parts) def method_path(self, *method): return self.get_path("method", *method) From bfb463e814e1f65cd56c4587dd4612d677dc0328 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 16:35:42 +0530 Subject: [PATCH 26/44] refactor!: merge handle.py This has several breaking changes for v2: 1. No support for following methods which were implicitly present in default namespace. - run_doc_method - ping - web_logout - logout - uploadfile - upload_file - download_file --- frappe/api/v2.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index ba122efb87..4594d3243c 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -12,12 +12,13 @@ import json from werkzeug.routing import Rule import frappe -from frappe import _ +from frappe import _, is_whitelisted +from frappe.core.doctype.server_script.server_script_utils import get_server_script_map +from frappe.handler import is_valid_http_method, run_server_script from frappe.utils.data import sbool def handle_rpc_call(method: str, doctype: str | None = None): - from frappe.handler import execute_cmd from frappe.modules.utils import load_doctype_module if doctype: @@ -29,7 +30,25 @@ def handle_rpc_call(method: str, doctype: str | None = None): # Login works implicitly right now. return - return execute_cmd(method) + for hook in frappe.get_hooks("override_whitelisted_methods", {}).get(method, []): + # override using the first hook + method = hook + break + + # via server script + server_script = get_server_script_map().get("_api", {}).get(method) + if server_script: + return run_server_script(server_script) + + try: + method = frappe.get_attr(method) + except Exception as e: + frappe.throw(_("Failed to get method {0} with {1}").format(method, e)) + + is_whitelisted(method) + is_valid_http_method(method) + + return frappe.call(method, **frappe.form_dict) def read_doc(doctype: str, name: str): From d117e2c08bdb8881e8f0e49b5d47def8a89cf588 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 16:38:09 +0530 Subject: [PATCH 27/44] fix!: Last overriden method should be considered --- frappe/api/v2.py | 4 ++-- frappe/handler.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 4594d3243c..130072d50e 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -30,8 +30,8 @@ def handle_rpc_call(method: str, doctype: str | None = None): # Login works implicitly right now. return - for hook in frappe.get_hooks("override_whitelisted_methods", {}).get(method, []): - # override using the first hook + for hook in reversed(frappe.get_hooks("override_whitelisted_methods", {}).get(method, [])): + # override using the last hook method = hook break diff --git a/frappe/handler.py b/frappe/handler.py index fe438d4cae..db99a47a43 100644 --- a/frappe/handler.py +++ b/frappe/handler.py @@ -61,7 +61,7 @@ def handle(): def execute_cmd(cmd, from_async=False): """execute a request as python module""" for hook in reversed(frappe.get_hooks("override_whitelisted_methods", {}).get(cmd, [])): - # override using the first hook + # override using the last hook cmd = hook break From 232f080044b9303b6d3e7d87fda60b989a7cae6c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 16:57:58 +0530 Subject: [PATCH 28/44] feat: run_doc_method v2 --- frappe/api/v2.py | 42 +++++++++++++++++++++++++++++++++++++++- frappe/tests/test_api.py | 39 ++++++++++++++++++++++++++++++------- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 130072d50e..ec184214d4 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -8,11 +8,12 @@ Note: internal implementation can change without treating it as "breaking change". """ import json +from typing import Any from werkzeug.routing import Rule import frappe -from frappe import _, is_whitelisted +from frappe import _, get_newargs, is_whitelisted from frappe.core.doctype.server_script.server_script_utils import get_server_script_map from frappe.handler import is_valid_http_method, run_server_script from frappe.utils.data import sbool @@ -122,8 +123,47 @@ def execute_doc_method(doctype: str, name: str, method: str | None = None): return doc.run_method(method, **frappe.form_dict) +def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): + """run a whitelisted controller method on in-memory document. + + + This is useful for building clients that don't necessarily encode all the business logic but + call server side function on object to validate and modify the doc. + + The doc CAN exists in DB too and can write to DB as well if method is POST. + """ + + if isinstance(document, str): + document = frappe.parse_json(document) + + if kwargs is None: + kwargs = {} + + doc = frappe.get_doc(document) + doc._original_modified = doc.modified + doc.check_if_latest() + + if not doc.has_permission("read"): + raise frappe.PermissionError + + method_obj = getattr(doc, method) + fn = getattr(method_obj, "__func__", method_obj) + is_whitelisted(fn) + is_valid_http_method(fn) + + new_kwargs = get_newargs(fn, kwargs) + response = doc.run_method(method, **new_kwargs) + frappe.response.docs.append(doc) # send modified document and result both. + return response + + url_rules = [ Rule("/method/", endpoint=handle_rpc_call), + Rule( + "/method/run_doc_method", + methods=["GET", "POST"], + endpoint=lambda: frappe.call(run_doc_method, **frappe.form_dict), + ), Rule("/method//", endpoint=handle_rpc_call), Rule("/document/", methods=["GET"], endpoint=document_list), Rule("/document/", methods=["POST"], endpoint=create_doc), diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index db097638fa..a5025a7c98 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -215,7 +215,6 @@ class TestResourceAPI(FrappeAPITestCase): @parameterize("", "v1", "v2") def test_create_document(self): - # test 7: POST method on {self.resource_path} to create doc data = {"description": frappe.mock("paragraph"), "sid": self.sid} response = self.post(self.resource_path(self.DOCTYPE), data) self.assertEqual(response.status_code, 200) @@ -225,7 +224,6 @@ class TestResourceAPI(FrappeAPITestCase): @parameterize("", "v1", "v2") def test_update_document(self): - # test 8: PUT method on {self.resource_path} to update doc generated_desc = frappe.mock("paragraph") data = {"description": generated_desc, "sid": self.sid} random_doc = choice(self.GENERATED_DOCUMENTS) @@ -239,7 +237,6 @@ class TestResourceAPI(FrappeAPITestCase): @parameterize("", "v1", "v2") def test_delete_document(self): - # test 9: DELETE method on {self.resource_path} doc_to_delete = choice(self.GENERATED_DOCUMENTS) response = self.delete(self.resource_path(self.DOCTYPE, doc_to_delete)) self.assertEqual(response.status_code, 202) @@ -331,22 +328,23 @@ class TestResourceAPIV2(FrappeAPITestCase): class TestMethodAPIV2(FrappeAPITestCase): version = "v2" + def setUp(self) -> None: + self.post(self.method_path("login"), {"sid": self.sid}) + return super().setUp() + def test_ping(self): - # test 2: test for /api/method/ping - response = self.get(self.method_path("ping")) + response = self.get(self.method_path("frappe.ping")) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertEqual(response.json["data"], "pong") def test_get_user_info(self): - # test 3: test for /api/method/frappe.realtime.get_user_info response = self.get(self.method_path("frappe.realtime.get_user_info")) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIn(response.json.get("data").get("user"), ("Administrator", "Guest")) def test_auth_cycle(self): - # test 4: Pass authorization token in request global authorization_token generate_admin_keys() @@ -375,6 +373,33 @@ class TestMethodAPIV2(FrappeAPITestCase): ) self.assertEqual(expanded_response.data, shorthand_response.data) + def test_run_doc_method_in_memory(self): + dns = frappe.get_doc("Document Naming Settings") + + # Check that simple API can be called. + response = self.get( + self.method_path("run_doc_method"), + { + "sid": self.sid, + "document": dns.as_dict(), + "method": "get_transactions_and_prefixes", + }, + ) + self.assertTrue(response.json["data"]) + self.assertGreaterEqual(len(response.json["docs"]), 1) + + # Call with known and unknown arguments, only known should get passed + response = self.get( + self.method_path("run_doc_method"), + { + "sid": self.sid, + "document": dns.as_dict(), + "method": "get_options", + "kwargs": {"doctype": "Webhook", "unknown": "what"}, + }, + ) + self.assertEqual(response.status_code, 200) + class TestReadOnlyMode(FrappeAPITestCase): """During migration if read only mode can be enabled. From 7996f76ebbd9cfce00d6a1ba6b76d9737dfe018b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:02:43 +0530 Subject: [PATCH 29/44] fix: Better delete doc for child tables --- frappe/api/v2.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index ec184214d4..761305983c 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -103,8 +103,9 @@ def update_doc(doctype: str, name: str): def delete_doc(doctype: str, name: str): - # TODO: child doc handling - frappe.delete_doc(doctype, name, ignore_missing=False) + import frappe.client + + frappe.client.delete_doc(doctype, name) frappe.response.http_status_code = 202 return "ok" From 43028f51d96c1324b9af48f3106aac6327d3144a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 17:56:38 +0530 Subject: [PATCH 30/44] refactor: simplify v2 implementations --- frappe/api/v2.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 761305983c..2a94e0cf34 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -18,6 +18,11 @@ from frappe.core.doctype.server_script.server_script_utils import get_server_scr from frappe.handler import is_valid_http_method, run_server_script from frappe.utils.data import sbool +PERMISSION_MAP = { + "GET": "read", + "POST": "write", +} + def handle_rpc_call(method: str, doctype: str | None = None): from frappe.modules.utils import load_doctype_module @@ -89,9 +94,7 @@ def update_doc(doctype: str, name: str): data = frappe.form_dict doc = frappe.get_doc(doctype, name, for_update=True) - if "flags" in data: - del data["flags"] - + data.pop("flags", None) doc.update(data) doc.save() @@ -111,17 +114,18 @@ def delete_doc(doctype: str, name: str): def execute_doc_method(doctype: str, name: str, method: str | None = None): + """Get a document from DB and execute method on it. + + Use cases: + - Submitting/cancelling document + - Triggering some kind of update on a document + """ method = method or frappe.form_dict.pop("run_method") doc = frappe.get_doc(doctype, name) doc.is_whitelisted(method) - if frappe.request.method == "GET": - doc.check_permission("read") - return doc.run_method(method, **frappe.form_dict) - - elif frappe.request.method == "POST": - doc.check_permission("write") - return doc.run_method(method, **frappe.form_dict) + doc.check_permission(PERMISSION_MAP[frappe.request.method]) + return doc.run_method(method, **frappe.form_dict) def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): From 824229ce0af9862578b44d782c1291e1aae6fe20 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:14:46 +0530 Subject: [PATCH 31/44] refactor: no need to cast bools manually --- frappe/api/v2.py | 16 ++-------------- frappe/client.py | 4 ++-- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 2a94e0cf34..02ba0f6017 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -13,10 +13,10 @@ from typing import Any from werkzeug.routing import Rule import frappe +import frappe.client from frappe import _, get_newargs, is_whitelisted from frappe.core.doctype.server_script.server_script_utils import get_server_script_map from frappe.handler import is_valid_http_method, run_server_script -from frappe.utils.data import sbool PERMISSION_MAP = { "GET": "read", @@ -69,17 +69,7 @@ def document_list(doctype: str): frappe.form_dict["fields"] = json.loads(frappe.form_dict["fields"]) # set limit of records for frappe.get_list - frappe.form_dict.setdefault( - "limit_page_length", - frappe.form_dict.limit or frappe.form_dict.limit_page_length or 20, - ) - - # convert strings to native types - only as_dict and debug accept bool - for param in ["as_dict", "debug"]: - param_val = frappe.form_dict.get(param) - if param_val is not None: - frappe.form_dict[param] = sbool(param_val) - + frappe.form_dict.limit_page_length = frappe.form_dict.limit or 20 # evaluate frappe.get_list return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) @@ -106,8 +96,6 @@ def update_doc(doctype: str, name: str): def delete_doc(doctype: str, name: str): - import frappe.client - frappe.client.delete_doc(doctype, name) frappe.response.http_status_code = 202 return "ok" diff --git a/frappe/client.py b/frappe/client.py index 85e99a6534..6439e9d71d 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -32,8 +32,8 @@ def get_list( limit_start=None, limit_page_length=20, parent=None, - debug=False, - as_dict=True, + debug: bool = False, + as_dict: bool = True, or_filters=None, ): """Returns a list of records by filters, fields, ordering and limit From 67a595705651ba8f2806097040bf8de56509d65c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:17:55 +0530 Subject: [PATCH 32/44] fix: check for write permission if POST method --- frappe/api/v2.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 02ba0f6017..c08707267c 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -136,8 +136,7 @@ def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): doc._original_modified = doc.modified doc.check_if_latest() - if not doc.has_permission("read"): - raise frappe.PermissionError + doc.check_permission(PERMISSION_MAP[frappe.request.method]) method_obj = getattr(doc, method) fn = getattr(method_obj, "__func__", method_obj) From c8cd658d26f6e7af61d8c23a906b80d03d30d3c4 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:21:15 +0530 Subject: [PATCH 33/44] fix: add login and logout methods --- frappe/api/v2.py | 17 +++++++++++++---- frappe/tests/test_api.py | 7 ++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index c08707267c..dafb0923f5 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -32,10 +32,6 @@ def handle_rpc_call(method: str, doctype: str | None = None): module = load_doctype_module(doctype) method = module.__name__ + "." + method - if method == "login": - # Login works implicitly right now. - return - for hook in reversed(frappe.get_hooks("override_whitelisted_methods", {}).get(method, [])): # override using the last hook method = hook @@ -57,6 +53,16 @@ def handle_rpc_call(method: str, doctype: str | None = None): return frappe.call(method, **frappe.form_dict) +def login(): + """Login happens implicitly, this function doesn't do anything.""" + pass + + +def logout(): + frappe.local.login_manager.logout() + frappe.db.commit() + + def read_doc(doctype: str, name: str): doc = frappe.get_doc(doctype, name) doc.check_permission("read") @@ -150,6 +156,9 @@ def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): url_rules = [ + Rule("/method/login", endpoint=login), + Rule("/method/logout", endpoint=logout), + Rule("/method/ping", endpoint=frappe.ping), Rule("/method/", endpoint=handle_rpc_call), Rule( "/method/run_doc_method", diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index a5025a7c98..e6bc617192 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -333,7 +333,7 @@ class TestMethodAPIV2(FrappeAPITestCase): return super().setUp() def test_ping(self): - response = self.get(self.method_path("frappe.ping")) + response = self.get(self.method_path("ping")) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertEqual(response.json["data"], "pong") @@ -373,6 +373,11 @@ class TestMethodAPIV2(FrappeAPITestCase): ) self.assertEqual(expanded_response.data, shorthand_response.data) + def test_logout(self): + self.post(self.method_path("logout"), {"sid": self.sid}) + response = self.get(self.method_path("ping")) + self.assertFalse(response.request.cookies["sid"]) + def test_run_doc_method_in_memory(self): dns = frappe.get_doc("Document Naming Settings") From cfd3fb934175cd00a3c91c9aa3a3fa2c208ccd35 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:31:00 +0530 Subject: [PATCH 34/44] refactor: PUT == PATCH Correct conventions for partial updates --- frappe/api/v2.py | 2 +- frappe/tests/test_api.py | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index dafb0923f5..991a348578 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -169,7 +169,7 @@ url_rules = [ Rule("/document/", methods=["GET"], endpoint=document_list), Rule("/document/", methods=["POST"], endpoint=create_doc), Rule("/document///", methods=["GET"], endpoint=read_doc), - Rule("/document///", methods=["PUT"], endpoint=update_doc), + Rule("/document///", methods=["PATCH", "PUT"], endpoint=update_doc), Rule("/document///", methods=["DELETE"], endpoint=delete_doc), Rule( "/document///method//", diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index e6bc617192..54800ebd8c 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -135,6 +135,9 @@ class FrappeAPITestCase(FrappeTestCase): def put(self, path, data, **kwargs) -> TestResponse: return make_request(target=self.TEST_CLIENT.put, args=(path,), kwargs={"json": data, **kwargs}) + def patch(self, path, data, **kwargs) -> TestResponse: + return make_request(target=self.TEST_CLIENT.patch, args=(path,), kwargs={"json": data, **kwargs}) + def delete(self, path, **kwargs) -> TestResponse: return make_request(target=self.TEST_CLIENT.delete, args=(path,), kwargs=kwargs) @@ -222,7 +225,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(docname, str) self.GENERATED_DOCUMENTS.append(docname) - @parameterize("", "v1", "v2") + @parameterize("", "v1") def test_update_document(self): generated_desc = frappe.mock("paragraph") data = {"description": generated_desc, "sid": self.sid} @@ -313,7 +316,7 @@ class TestMethodAPIV1(FrappeAPITestCase): self.assertEqual(response.status_code, 404) -class TestResourceAPIV2(FrappeAPITestCase): +class TestResourceAPIV2(TestResourceAPI): version = "v2" def setUp(self) -> None: @@ -324,6 +327,18 @@ class TestResourceAPIV2(FrappeAPITestCase): response = self.get(self.resource_path("Website Theme", "Standard", "method", "get_apps")) self.assertEqual(response.json["data"][0]["name"], "frappe") + def test_update_document(self): + generated_desc = frappe.mock("paragraph") + data = {"description": generated_desc, "sid": self.sid} + random_doc = choice(self.GENERATED_DOCUMENTS) + + response = self.patch(self.resource_path(self.DOCTYPE, random_doc), data=data) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"]["description"], generated_desc) + + response = self.get(self.resource_path(self.DOCTYPE, random_doc)) + self.assertEqual(response.json["data"]["description"], generated_desc) + class TestMethodAPIV2(FrappeAPITestCase): version = "v2" From 47538f76011a932fb5cd8a351069f420b03d0630 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 5 Sep 2023 18:49:22 +0530 Subject: [PATCH 35/44] feat: doctype collection level APIs - meta, count --- frappe/api/v2.py | 10 ++++++++++ frappe/desk/reportview.py | 2 +- frappe/tests/test_api.py | 22 +++++++++++++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 991a348578..938ee5b420 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -155,6 +155,14 @@ def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): return response +def count(doctype: str) -> int: + from frappe.desk.reportview import get_count + + # TODO: Rewrite + frappe.form_dict.doctype = doctype + return get_count() + + url_rules = [ Rule("/method/login", endpoint=login), Rule("/method/logout", endpoint=logout), @@ -176,4 +184,6 @@ url_rules = [ methods=["GET", "POST"], endpoint=execute_doc_method, ), + Rule("/doctype//meta", methods=["GET"], endpoint=frappe.get_meta), + Rule("/doctype//count", methods=["GET"], endpoint=count), ] diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 102a708895..f8d4632655 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -46,7 +46,7 @@ def get_list(): @frappe.whitelist() @frappe.read_only() -def get_count(): +def get_count() -> int: args = get_form_params() if is_virtual_doctype(args.doctype): diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 54800ebd8c..aa7ec2186e 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -112,6 +112,9 @@ class FrappeAPITestCase(FrappeTestCase): def method_path(self, *method): return self.get_path("method", *method) + def doctype_path(self, *method): + return self.get_path("doctype", *method) + def get_path(self, *parts): return urljoin(self.site_url, "/".join(("api", self.version, *parts))) @@ -316,7 +319,7 @@ class TestMethodAPIV1(FrappeAPITestCase): self.assertEqual(response.status_code, 404) -class TestResourceAPIV2(TestResourceAPI): +class TestDocumentAPIV2(TestResourceAPI): version = "v2" def setUp(self) -> None: @@ -421,6 +424,23 @@ class TestMethodAPIV2(FrappeAPITestCase): self.assertEqual(response.status_code, 200) +class TestDocTypeAPIV2(FrappeAPITestCase): + version = "v2" + + def setUp(self) -> None: + self.post(self.method_path("login"), {"sid": self.sid}) + return super().setUp() + + def test_meta(self): + response = self.get(self.doctype_path("ToDo", "meta")) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"]["name"], "ToDo") + + def test_count(self): + response = self.get(self.doctype_path("ToDo", "count")) + self.assertIsInstance(response.json["data"], int) + + class TestReadOnlyMode(FrappeAPITestCase): """During migration if read only mode can be enabled. Test if reads work well and writes are blocked""" From dfcb69ab21aa6c11960d97861fe5fca063765b8c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Sep 2023 11:36:04 +0530 Subject: [PATCH 36/44] refactor: use reportview get_count implementation Count reported should be according to permissions --- frappe/api/v2.py | 19 +++++++++++-------- frappe/desk/reportview.py | 2 +- frappe/tests/test_api.py | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index 938ee5b420..d8b453ee6c 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -80,6 +80,14 @@ def document_list(doctype: str): return frappe.call(frappe.client.get_list, doctype, **frappe.form_dict) +def count(doctype: str) -> int: + from frappe.desk.reportview import get_count + + frappe.form_dict.doctype = doctype + + return get_count() + + def create_doc(doctype: str): data = frappe.form_dict data.pop("doctype", None) @@ -155,15 +163,8 @@ def run_doc_method(method: str, document: dict[str, Any] | str, kwargs=None): return response -def count(doctype: str) -> int: - from frappe.desk.reportview import get_count - - # TODO: Rewrite - frappe.form_dict.doctype = doctype - return get_count() - - url_rules = [ + # RPC calls Rule("/method/login", endpoint=login), Rule("/method/logout", endpoint=logout), Rule("/method/ping", endpoint=frappe.ping), @@ -174,6 +175,7 @@ url_rules = [ endpoint=lambda: frappe.call(run_doc_method, **frappe.form_dict), ), Rule("/method//", endpoint=handle_rpc_call), + # Document level APIs Rule("/document/", methods=["GET"], endpoint=document_list), Rule("/document/", methods=["POST"], endpoint=create_doc), Rule("/document///", methods=["GET"], endpoint=read_doc), @@ -184,6 +186,7 @@ url_rules = [ methods=["GET", "POST"], endpoint=execute_doc_method, ), + # Collection level APIs Rule("/doctype//meta", methods=["GET"], endpoint=frappe.get_meta), Rule("/doctype//count", methods=["GET"], endpoint=count), ] diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index f8d4632655..fd299af819 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -65,7 +65,7 @@ def execute(doctype, *args, **kwargs): def get_form_params(): - """Stringify GET request parameters.""" + """parse GET request parameters.""" data = frappe._dict(frappe.local.form_dict) clean_params(data) validate_args(data) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index aa7ec2186e..08a240717e 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -152,7 +152,7 @@ class TestResourceAPI(FrappeAPITestCase): @classmethod def setUpClass(cls): super().setUpClass() - for _ in range(10): + for _ in range(20): doc = frappe.get_doc({"doctype": "ToDo", "description": frappe.mock("paragraph")}).insert() cls.GENERATED_DOCUMENTS.append(doc.name) frappe.db.commit() From 3cc2ca8fc7a7bddcbab3667c974e4558e2f73c67 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Sep 2023 12:07:17 +0530 Subject: [PATCH 37/44] test: old API messages --- frappe/tests/test_api.py | 69 ++++++++++++++++++++++++++++++++++++++++ frappe/utils/response.py | 1 + 2 files changed, 70 insertions(+) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 08a240717e..7458ae5d09 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -318,6 +318,35 @@ class TestMethodAPIV1(FrappeAPITestCase): response = self.get(self.resource_path("User", "NonExistent@s.com"), {"sid": self.sid}) self.assertEqual(response.status_code, 404) + def test_logs(self): + method = "frappe.tests.test_api.test" + + def get_message(resp, msg_type): + return frappe.parse_json(frappe.parse_json(frappe.parse_json(resp.json)[msg_type])[0]) + + expected_message = "Failed" + response = self.get(self.method_path(method), {"sid": self.sid, "message": expected_message}) + self.assertEqual(get_message(response, "_server_messages").message, expected_message) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True} + ) + self.assertEqual(get_message(response, "_server_messages").message, expected_message) + self.assertEqual(response.json["exc_type"], "ValidationError") + self.assertIn("Traceback", response.json["exc"]) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), + {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, + ) + self.assertNotIn("_server_messages", response.json) + self.assertIn("ZeroDivisionError", response.json["exception"]) # WHY? + self.assertIn("Traceback", response.json["exc"]) + class TestDocumentAPIV2(TestResourceAPI): version = "v2" @@ -423,6 +452,35 @@ class TestMethodAPIV2(FrappeAPITestCase): ) self.assertEqual(response.status_code, 200) + def test_logs(self): + method = "frappe.tests.test_api.test" + + def get_message(resp, msg_type): + return frappe.parse_json(frappe.parse_json(frappe.parse_json(resp.json)[msg_type])[0]) + + expected_message = "Failed" + response = self.get(self.method_path(method), {"sid": self.sid, "message": expected_message}) + self.assertEqual(get_message(response, "_server_messages").message, expected_message) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True} + ) + self.assertEqual(get_message(response, "_server_messages").message, expected_message) + self.assertEqual(response.json["exc_type"], "ValidationError") + self.assertIn("Traceback", response.json["exc"]) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), + {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, + ) + self.assertNotIn("_server_messages", response.json) + self.assertIn("ZeroDivisionError", response.json["exception"]) # WHY? + self.assertIn("Traceback", response.json["exc"]) + class TestDocTypeAPIV2(FrappeAPITestCase): version = "v2" @@ -544,3 +602,14 @@ def generate_admin_keys(): generate_keys("Administrator") frappe.db.commit() + + +@frappe.whitelist() +def test(*, fail=False, handled=True, message="Failed"): + if fail: + if handled: + frappe.throw(message) + else: + 1 / 0 + else: + frappe.msgprint(message) diff --git a/frappe/utils/response.py b/frappe/utils/response.py index fcd7b49a0f..d6cea3775f 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -126,6 +126,7 @@ def as_binary(): def make_logs(response=None): + # TODO: v2 API """make strings for msgprint and errprint""" from frappe.utils.error import guess_exception_source From 411819ef81b5a9ab24bbbe0163e847e9802ee978 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Sep 2023 12:47:00 +0530 Subject: [PATCH 38/44] feat: v2 for message response strucutre --- frappe/api/__init__.py | 20 ++++++++++++++++++-- frappe/tests/test_api.py | 27 +++++++++++++++------------ frappe/utils/response.py | 31 ++++++++++++++++++++++++------- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index 9dc4ba63ac..e4f6056004 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -1,5 +1,7 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +from enum import Enum + from werkzeug.exceptions import NotFound from werkzeug.routing import Map, Submount from werkzeug.wrappers import Request, Response @@ -10,6 +12,11 @@ from frappe import _ from frappe.utils.response import build_response +class ApiVersion(str, Enum): + V1 = "v1" + V2 = "v2" + + def handle(request: Request): """ Entry point for `/api` methods. @@ -56,9 +63,18 @@ API_URL_MAP = Map( [ # V1 routes Submount("/api", v1_rules), - Submount("/api/v1", v1_rules), - Submount("/api/v2", v2_rules), + Submount(f"/api/{ApiVersion.V1.value}", v1_rules), + Submount(f"/api/{ApiVersion.V2.value}", v2_rules), ], strict_slashes=False, # Allows skipping trailing slashes merge_slashes=False, ) + + +def get_api_version() -> ApiVersion | None: + if not frappe.request or not frappe.request.path.startswith("/api"): + return + + if frappe.request.path.startswith(f"/api/{ApiVersion.V2.value}"): + return ApiVersion.V2 + return ApiVersion.V1 diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 7458ae5d09..91eef7fb1c 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -455,21 +455,23 @@ class TestMethodAPIV2(FrappeAPITestCase): def test_logs(self): method = "frappe.tests.test_api.test" - def get_message(resp, msg_type): - return frappe.parse_json(frappe.parse_json(frappe.parse_json(resp.json)[msg_type])[0]) + expected_message = "Failed v2" + response = self.get( + self.method_path(method), {"sid": self.sid, "message": expected_message} + ).json - expected_message = "Failed" - response = self.get(self.method_path(method), {"sid": self.sid, "message": expected_message}) - self.assertEqual(get_message(response, "_server_messages").message, expected_message) + self.assertIsInstance(response["messages"], list) + self.assertEqual(response["messages"][0]["message"], expected_message) # Cause handled failured with suppress_stdout(): response = self.get( self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True} - ) - self.assertEqual(get_message(response, "_server_messages").message, expected_message) - self.assertEqual(response.json["exc_type"], "ValidationError") - self.assertIn("Traceback", response.json["exc"]) + ).json + self.assertIsInstance(response["errors"], list) + self.assertEqual(response["errors"][0]["message"], expected_message) + self.assertEqual(response["errors"][0]["type"], "ValidationError") + self.assertIn("Traceback", response["errors"][0]["exception"]) # Cause handled failured with suppress_stdout(): @@ -477,9 +479,10 @@ class TestMethodAPIV2(FrappeAPITestCase): self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, ) - self.assertNotIn("_server_messages", response.json) - self.assertIn("ZeroDivisionError", response.json["exception"]) # WHY? - self.assertIn("Traceback", response.json["exc"]) + + self.assertIsInstance(response["errors"], list) + self.assertEqual(response["errors"][0]["type"], "ZeroDivisionError") + self.assertIn("Traceback", response["errors"][0]["exception"]) class TestDocTypeAPIV2(FrappeAPITestCase): diff --git a/frappe/utils/response.py b/frappe/utils/response.py index d6cea3775f..4abefbafb4 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -6,7 +6,7 @@ import decimal import json import mimetypes import os -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Literal from urllib.parse import quote import werkzeug.utils @@ -21,7 +21,7 @@ import frappe.sessions import frappe.utils from frappe import _ from frappe.core.doctype.access_log.access_log import make_access_log -from frappe.utils import cint, format_timedelta +from frappe.utils import format_timedelta if TYPE_CHECKING: from frappe.core.doctype.file.file import File @@ -99,6 +99,7 @@ def as_raw(): def as_json(): make_logs() + response = Response() if frappe.local.response.http_status_code: response.status_code = frappe.local.response["http_status_code"] @@ -125,13 +126,22 @@ def as_binary(): return response -def make_logs(response=None): - # TODO: v2 API +def make_logs(): """make strings for msgprint and errprint""" + + from frappe.api import ApiVersion, get_api_version + + match get_api_version(): + case ApiVersion.V1: + _make_logs_v1() + case ApiVersion.V2: + _make_logs_v2() + + +def _make_logs_v1(): from frappe.utils.error import guess_exception_source - if not response: - response = frappe.local.response + response = frappe.local.response if frappe.error_log: if source := guess_exception_source(frappe.local.error_log and frappe.local.error_log[0]["exc"]): @@ -143,13 +153,20 @@ def make_logs(response=None): [frappe.utils.cstr(d) for d in frappe.local.message_log] ) - if frappe.debug_log and frappe.conf.get("logging") or False: + if frappe.debug_log and frappe.conf.get("logging"): response["_debug_messages"] = json.dumps(frappe.local.debug_log) if frappe.flags.error_message: response["_error_message"] = frappe.flags.error_message +def _make_logs_v2(): + response = frappe.local.response + + if frappe.local.message_log: + response["messages"] = [frappe.parse_json(l) for l in frappe.local.message_log] + + def json_handler(obj): """serialize non-serializable data for json""" from collections.abc import Iterable From 018ed845bd2706b0afdba283078ff1e654be1934 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 11 Sep 2023 18:59:19 +0530 Subject: [PATCH 39/44] refactor: defer unnecessary json-dumping of messages Also avoid accessing locals where interface is present like for popping last message. --- frappe/__init__.py | 2 +- frappe/boot.py | 3 +-- frappe/core/doctype/data_import/test_importer.py | 2 +- frappe/core/doctype/deleted_document/deleted_document.py | 4 ++-- .../document_naming_settings/document_naming_settings.py | 3 +-- frappe/core/doctype/file/test_file.py | 2 +- frappe/core/doctype/user/test_user.py | 2 +- frappe/desk/desktop.py | 3 +-- frappe/desk/doctype/desktop_icon/desktop_icon.py | 3 +-- frappe/desk/form/linked_with.py | 6 ++---- frappe/email/doctype/email_account/email_account.py | 4 ++-- frappe/model/document.py | 4 ++-- frappe/tests/test_api.py | 1 + frappe/utils/nestedset.py | 2 +- frappe/utils/response.py | 6 ++---- frappe/website/doctype/web_page_view/web_page_view.py | 3 +-- 16 files changed, 21 insertions(+), 29 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index b3847143e9..9addfc0785 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -534,7 +534,7 @@ def msgprint( if wide: out.wide = wide - message_log.append(json.dumps(out)) + message_log.append(out) if raise_exception and hasattr(raise_exception, "__name__"): local.response["exc_type"] = raise_exception.__name__ diff --git a/frappe/boot.py b/frappe/boot.py index 3f6e3a6a39..c36927637a 100644 --- a/frappe/boot.py +++ b/frappe/boot.py @@ -295,8 +295,7 @@ def add_home_page(bootinfo, docs): docs.append(page) bootinfo["home_page"] = page.name except (frappe.DoesNotExistError, frappe.PermissionError): - if frappe.message_log: - frappe.message_log.pop() + frappe.clear_last_message() bootinfo["home_page"] = "Workspaces" diff --git a/frappe/core/doctype/data_import/test_importer.py b/frappe/core/doctype/data_import/test_importer.py index 978f5792dd..965e34c3e6 100644 --- a/frappe/core/doctype/data_import/test_importer.py +++ b/frappe/core/doctype/data_import/test_importer.py @@ -63,7 +63,7 @@ class TestImporter(FrappeTestCase): def test_data_import_without_mandatory_values(self): import_file = get_import_file("sample_import_file_without_mandatory") data_import = self.get_importer(doctype_name, import_file) - frappe.local.message_log = [] + frappe.clear_messages() data_import.start_import() data_import.reload() diff --git a/frappe/core/doctype/deleted_document/deleted_document.py b/frappe/core/doctype/deleted_document/deleted_document.py index b5b35206e9..aa6239c279 100644 --- a/frappe/core/doctype/deleted_document/deleted_document.py +++ b/frappe/core/doctype/deleted_document/deleted_document.py @@ -73,11 +73,11 @@ def bulk_restore(docnames): restored.append(d) except frappe.DocumentAlreadyRestored: - frappe.message_log.pop() + frappe.clear_last_message() invalid.append(d) except Exception: - frappe.message_log.pop() + frappe.clear_last_message() failed.append(d) frappe.db.rollback() diff --git a/frappe/core/doctype/document_naming_settings/document_naming_settings.py b/frappe/core/doctype/document_naming_settings/document_naming_settings.py index 3ec4147ec7..ddb25dd262 100644 --- a/frappe/core/doctype/document_naming_settings/document_naming_settings.py +++ b/frappe/core/doctype/document_naming_settings/document_naming_settings.py @@ -248,8 +248,7 @@ class DocumentNamingSettings(Document): doc = self._fetch_last_doc_if_available() return "\n".join(NamingSeries(series).get_preview(doc=doc)) except Exception as e: - if frappe.message_log: - frappe.message_log.pop() + frappe.clear_last_message() return _("Failed to generate names from the series") + f"\n{str(e)}" def _fetch_last_doc_if_available(self): diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 42f349aef4..43dc51c8b1 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -501,7 +501,7 @@ class TestFile(FrappeTestCase): test_file.file_url = frappe.utils.get_url("unknown.jpg") test_file.make_thumbnail(suffix="xs") self.assertEqual( - json.loads(frappe.message_log[0]).get("message"), + frappe.message_log[0].get("message"), f"File '{frappe.utils.get_url('unknown.jpg')}' not found", ) self.assertEqual(test_file.thumbnail_url, None) diff --git a/frappe/core/doctype/user/test_user.py b/frappe/core/doctype/user/test_user.py index 1ca0a56ec0..dc973c9e8f 100644 --- a/frappe/core/doctype/user/test_user.py +++ b/frappe/core/doctype/user/test_user.py @@ -420,7 +420,7 @@ class TestUser(FrappeTestCase): self.assertEqual(update_password(new_password, key=test_user.reset_password_key), "/") update_password(old_password, old_password=new_password) self.assertEqual( - json.loads(frappe.message_log[0]).get("message"), + frappe.message_log[0].get("message"), "Password reset instructions have been sent to your email", ) diff --git a/frappe/desk/desktop.py b/frappe/desk/desktop.py index 4701879982..8ae20f7bb0 100644 --- a/frappe/desk/desktop.py +++ b/frappe/desk/desktop.py @@ -22,8 +22,7 @@ def handle_not_exist(fn): try: return fn(*args, **kwargs) except DoesNotExistError: - if frappe.message_log: - frappe.message_log.pop() + frappe.clear_last_message() return [] return wrapper diff --git a/frappe/desk/doctype/desktop_icon/desktop_icon.py b/frappe/desk/doctype/desktop_icon/desktop_icon.py index 524285f85d..7901ef9500 100644 --- a/frappe/desk/doctype/desktop_icon/desktop_icon.py +++ b/frappe/desk/doctype/desktop_icon/desktop_icon.py @@ -283,8 +283,7 @@ def set_desktop_icons(visible_list, ignore_duplicate=True): raise e else: visible_list.remove(module_name) - if frappe.message_log: - frappe.message_log.pop() + frappe.clear_last_message() # set the order set_order(visible_list) diff --git a/frappe/desk/form/linked_with.py b/frappe/desk/form/linked_with.py index 8f569b5a9e..d9414ef706 100644 --- a/frappe/desk/form/linked_with.py +++ b/frappe/desk/form/linked_with.py @@ -427,8 +427,7 @@ def get_linked_docs(doctype: str, name: str, linkinfo: dict | None = None) -> di link_meta_bundle = frappe.desk.form.load.get_meta_bundle(dt) except Exception as e: if isinstance(e, frappe.DoesNotExistError): - if frappe.local.message_log: - frappe.local.message_log.pop() + frappe.clear_last_message() continue linkmeta = link_meta_bundle[0] @@ -502,8 +501,7 @@ def get_linked_docs(doctype: str, name: str, linkinfo: dict | None = None) -> di ret = None except frappe.PermissionError: - if frappe.local.message_log: - frappe.local.message_log.pop() + frappe.clear_last_message() continue diff --git a/frappe/email/doctype/email_account/email_account.py b/frappe/email/doctype/email_account/email_account.py index 149e42dd80..24d548cb37 100755 --- a/frappe/email/doctype/email_account/email_account.py +++ b/frappe/email/doctype/email_account/email_account.py @@ -309,7 +309,7 @@ class EmailAccount(Document): except OSError: if in_receive: # timeout while connecting, see receive.py connect method - description = frappe.message_log.pop() if frappe.message_log else "Socket Error" + description = frappe.clear_last_message() if frappe.message_log else "Socket Error" if test_internet(): self.db_set("no_failed", self.no_failed + 1) if self.no_failed > 2: @@ -496,7 +496,7 @@ class EmailAccount(Document): } ) except assign_to.DuplicateToDoError: - frappe.message_log.pop() + frappe.clear_last_message() pass else: self.set_failed_attempts_count(self.get_failed_attempts_count() + 1) diff --git a/frappe/model/document.py b/frappe/model/document.py index 9cfb3057bd..212ae1e6f4 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1628,8 +1628,8 @@ def execute_action(__doctype, __name, __action, **kwargs): frappe.db.rollback() # add a comment (?) - if frappe.local.message_log: - msg = json.loads(frappe.local.message_log[-1]).get("message") + if frappe.message_log: + msg = frappe.message_log[-1].get("message") else: msg = "
" + frappe.get_traceback() + "
" diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 91eef7fb1c..a39d6a07ba 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -154,6 +154,7 @@ class TestResourceAPI(FrappeAPITestCase): super().setUpClass() for _ in range(20): doc = frappe.get_doc({"doctype": "ToDo", "description": frappe.mock("paragraph")}).insert() + cls.GENERATED_DOCUMENTS = [] cls.GENERATED_DOCUMENTS.append(doc.name) frappe.db.commit() diff --git a/frappe/utils/nestedset.py b/frappe/utils/nestedset.py index 01b7576876..2beaa63447 100644 --- a/frappe/utils/nestedset.py +++ b/frappe/utils/nestedset.py @@ -292,7 +292,7 @@ class NestedSet(Document): update_nsm(self) except frappe.DoesNotExistError: if self.flags.on_rollback: - frappe.message_log.pop() + frappe.clear_last_message() else: raise diff --git a/frappe/utils/response.py b/frappe/utils/response.py index 4abefbafb4..e05be5994b 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -149,9 +149,7 @@ def _make_logs_v1(): response["exc"] = json.dumps([frappe.utils.cstr(d["exc"]) for d in frappe.local.error_log]) if frappe.local.message_log: - response["_server_messages"] = json.dumps( - [frappe.utils.cstr(d) for d in frappe.local.message_log] - ) + response["_server_messages"] = json.dumps([json.dumps(d) for d in frappe.local.message_log]) if frappe.debug_log and frappe.conf.get("logging"): response["_debug_messages"] = json.dumps(frappe.local.debug_log) @@ -164,7 +162,7 @@ def _make_logs_v2(): response = frappe.local.response if frappe.local.message_log: - response["messages"] = [frappe.parse_json(l) for l in frappe.local.message_log] + response["messages"] = frappe.local.message_log def json_handler(obj): diff --git a/frappe/website/doctype/web_page_view/web_page_view.py b/frappe/website/doctype/web_page_view/web_page_view.py index 21b1c00a1f..7c18d1ff66 100644 --- a/frappe/website/doctype/web_page_view/web_page_view.py +++ b/frappe/website/doctype/web_page_view/web_page_view.py @@ -93,8 +93,7 @@ def make_view_log( else: view.insert(ignore_permissions=True) except Exception: - if frappe.message_log: - frappe.message_log.pop() + frappe.clear_last_message() @frappe.whitelist() From 6fd97bcbcf27a2c36c50d8c53f1c1b2ebfae1092 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 18 Sep 2023 17:28:33 +0530 Subject: [PATCH 40/44] fix: Add old file upload for v2 --- frappe/api/v2.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/api/v2.py b/frappe/api/v2.py index d8b453ee6c..06b6eab04e 100644 --- a/frappe/api/v2.py +++ b/frappe/api/v2.py @@ -16,7 +16,7 @@ import frappe import frappe.client from frappe import _, get_newargs, is_whitelisted from frappe.core.doctype.server_script.server_script_utils import get_server_script_map -from frappe.handler import is_valid_http_method, run_server_script +from frappe.handler import is_valid_http_method, run_server_script, upload_file PERMISSION_MAP = { "GET": "read", @@ -168,6 +168,7 @@ url_rules = [ Rule("/method/login", endpoint=login), Rule("/method/logout", endpoint=logout), Rule("/method/ping", endpoint=frappe.ping), + Rule("/method/upload_file", endpoint=upload_file), Rule("/method/", endpoint=handle_rpc_call), Rule( "/method/run_doc_method", From e2714c3e1ce0f11beab3d1a338e333a476d6fdc5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 18 Sep 2023 15:50:31 +0530 Subject: [PATCH 41/44] feat: v2 error and debug log structure --- frappe/__init__.py | 12 +++++------ frappe/tests/test_api.py | 30 +++++++++++++++++++-------- frappe/utils/response.py | 45 ++++++++++++++++++++++++++++++++-------- 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 9addfc0785..0b9b340c8d 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -488,9 +488,12 @@ def msgprint( def _raise_exception(): if raise_exception: if inspect.isclass(raise_exception) and issubclass(raise_exception, Exception): - raise raise_exception(msg) + exc = raise_exception(msg) else: - raise ValidationError(msg) + exc = ValidationError(msg) + if out.__frappe_exc_id: + exc.__frappe_exc_id = out.__frappe_exc_id + raise exc if flags.mute_messages: _raise_exception() @@ -527,6 +530,7 @@ def msgprint( if raise_exception: out.raise_exception = 1 + out.__frappe_exc_id = generate_hash() if primary_action: out.primary_action = primary_action @@ -535,10 +539,6 @@ def msgprint( out.wide = wide message_log.append(out) - - if raise_exception and hasattr(raise_exception, "__name__"): - local.response["exc_type"] = raise_exception.__name__ - _raise_exception() diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index a39d6a07ba..3825121834 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -200,7 +200,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(json.data, list) self.assertIsInstance(json.data[0], list) - @parameterize("", "v1", "v2") + @parameterize("", "v1") def test_get_list_debug(self): # test 5: fetch response with debug with suppress_stdout(): @@ -253,12 +253,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertEqual(response.status_code, 404) self.GENERATED_DOCUMENTS.remove(doc_to_delete) - non_existent_doc = frappe.generate_hash(length=12) - with suppress_stdout(): - response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) - self.assertEqual(response.status_code, 404) - self.assertDictEqual(response.json, {}) - @parameterize("", "v1") def test_run_doc_method(self): # test 10: Run whitelisted method on doc via /api/resource @@ -372,6 +366,15 @@ class TestDocumentAPIV2(TestResourceAPI): response = self.get(self.resource_path(self.DOCTYPE, random_doc)) self.assertEqual(response.json["data"]["description"], generated_desc) + def test_delete_document_non_existing(self): + non_existent_doc = frappe.generate_hash(length=12) + with suppress_stdout(): + response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json["errors"][0]["type"], "DoesNotExistError") + # 404s dont return exceptions + self.assertFalse(response.json["errors"][0].get("exception")) + class TestMethodAPIV2(FrappeAPITestCase): version = "v2" @@ -479,7 +482,7 @@ class TestMethodAPIV2(FrappeAPITestCase): response = self.get( self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, - ) + ).json self.assertIsInstance(response["errors"], list) self.assertEqual(response["errors"][0]["type"], "ZeroDivisionError") @@ -522,7 +525,7 @@ class TestReadOnlyMode(FrappeAPITestCase): self.assertIsInstance(response.json, dict) self.assertIsInstance(response.json["data"], list) - @parameterize("", "v1", "v2") + @parameterize("", "v1") def test_blocked_writes(self): with suppress_stdout(): response = self.post( @@ -531,6 +534,15 @@ class TestReadOnlyMode(FrappeAPITestCase): self.assertEqual(response.status_code, 503) self.assertEqual(response.json["exc_type"], "InReadOnlyMode") + @parameterize("v2") + def test_blocked_writes_v2(self): + with suppress_stdout(): + response = self.post( + self.resource_path("ToDo"), {"description": frappe.mock("paragraph"), "sid": self.sid} + ) + self.assertEqual(response.status_code, 503) + self.assertEqual(response.json["errors"][0]["type"], "InReadOnlyMode") + class TestWSGIApp(FrappeAPITestCase): def test_request_hooks(self): diff --git a/frappe/utils/response.py b/frappe/utils/response.py index e05be5994b..3fcf1a365e 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -6,6 +6,7 @@ import decimal import json import mimetypes import os +import sys from typing import TYPE_CHECKING, Literal from urllib.parse import quote @@ -29,22 +30,45 @@ if TYPE_CHECKING: def report_error(status_code): """Build error. Show traceback in developer mode""" - allow_traceback = frappe.get_system_settings("allow_error_traceback") if frappe.db else False - if ( - allow_traceback - and (status_code != 404 or frappe.conf.logging) + from frappe.api import ApiVersion, get_api_version + + allow_traceback = ( + (frappe.get_system_settings("allow_error_traceback") if frappe.db else False) and not frappe.local.flags.disable_traceback - ): - traceback = frappe.utils.get_traceback() - if traceback: - frappe.errprint(traceback) - frappe.local.response.exception = traceback.splitlines()[-1] + and (status_code != 404 or frappe.conf.logging) + ) + + traceback = frappe.utils.get_traceback() + exc_type, exc_value, _ = sys.exc_info() + + match get_api_version(): + case ApiVersion.V1: + if allow_traceback: + frappe.errprint(traceback) + frappe.response.exception = traceback.splitlines()[-1] + frappe.response["exc_type"] = exc_type.__name__ + case ApiVersion.V2: + error_log = {"type": exc_type.__name__} + if allow_traceback: + error_log["exception"] = traceback + _link_error_with_message_log(error_log, exc_value, frappe.message_log) + frappe.local.response.errors = [error_log] response = build_response("json") response.status_code = status_code return response +def _link_error_with_message_log(error_log, exception, message_logs): + for message in message_logs: + if message.get("__frappe_exc_id") == exception.__frappe_exc_id: + error_log.update(message) + message_logs.remove(message) + error_log.pop("raise_exception", None) + error_log.pop("__frappe_exc_id", None) + return + + def build_response(response_type=None): if "docs" in frappe.local.response and not frappe.local.response.docs: del frappe.local.response["docs"] @@ -164,6 +188,9 @@ def _make_logs_v2(): if frappe.local.message_log: response["messages"] = frappe.local.message_log + if frappe.debug_log and frappe.conf.get("logging"): + response["debug"] = [{"message": m} for m in frappe.local.debug_log] + def json_handler(obj): """serialize non-serializable data for json""" From 4dfb44d0a2f6cdc2600fb81f82999dfeae065e42 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 18 Sep 2023 18:48:04 +0530 Subject: [PATCH 42/44] fix: assume v1 if path is not set Old `cmd` calls will not work otherwise. --- frappe/api/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/api/__init__.py b/frappe/api/__init__.py index e4f6056004..5c504b2512 100644 --- a/frappe/api/__init__.py +++ b/frappe/api/__init__.py @@ -72,7 +72,7 @@ API_URL_MAP = Map( def get_api_version() -> ApiVersion | None: - if not frappe.request or not frappe.request.path.startswith("/api"): + if not frappe.request: return if frappe.request.path.startswith(f"/api/{ApiVersion.V2.value}"): From 76b4d209c7037a114a9cde01070fd6a75529f1d9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 18 Sep 2023 19:13:36 +0530 Subject: [PATCH 43/44] feat: frappe.call can accept api_version --- frappe/public/js/frappe/request.js | 19 +++++++++++++++---- frappe/public/js/frappe/ui/messages.js | 10 +++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/frappe/public/js/frappe/request.js b/frappe/public/js/frappe/request.js index b622037338..10a7158e4a 100644 --- a/frappe/public/js/frappe/request.js +++ b/frappe/public/js/frappe/request.js @@ -88,7 +88,11 @@ frappe.call = function (opts) { let url = opts.url; if (!url) { - url = "/api/method/" + args.cmd; + let prefix = "/api/method/"; + if (opts.api_version) { + prefix = `/api/${opts.api_version}/method/`; + } + url = prefix + args.cmd; if (window.cordova) { let host = frappe.request.url; host = host.slice(0, host.length - 1); @@ -116,6 +120,7 @@ frappe.call = function (opts) { // show_spinner: !opts.no_spinner, async: opts.async, silent: opts.silent, + api_version: opts.api_version, url, }); }; @@ -444,12 +449,18 @@ frappe.request.cleanup = function (opts, r) { } // show messages - if (r._server_messages && !opts.silent) { + // + let messages; + if (opts.api_version == "v2") { + messages = r.messages; + } else if (r._server_messages) { + messages = JSON.parse(r._server_messages); + } + if (messages && !opts.silent) { // show server messages if no handlers exist if (handlers.length === 0) { - r._server_messages = JSON.parse(r._server_messages); frappe.hide_msgprint(); - frappe.msgprint(r._server_messages); + frappe.msgprint(messages); } } diff --git a/frappe/public/js/frappe/ui/messages.js b/frappe/public/js/frappe/ui/messages.js index b8ed34d59d..c3fae2c9af 100644 --- a/frappe/public/js/frappe/ui/messages.js +++ b/frappe/public/js/frappe/ui/messages.js @@ -144,7 +144,15 @@ frappe.msgprint = function (msg, title, is_minimizable) { if (data.message instanceof Array) { let messages = data.message; - const exceptions = messages.map((m) => JSON.parse(m)).filter((m) => m.raise_exception); + const exceptions = messages + .map((m) => { + if (typeof m == "string") { + return JSON.parse(m); + } else { + return m; + } + }) + .filter((m) => m.raise_exception); // only show exceptions if any exceptions exist if (exceptions.length) { From a46389a7e4ed5a6d3e9c91e606ce575b6705bd33 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 17 Oct 2023 10:24:29 +0530 Subject: [PATCH 44/44] test: Split API v2 tests Duplication here seems better than weird and hard to understand parameterized version. --- frappe/tests/test_api.py | 203 +----------------------- frappe/tests/test_api_v2.py | 297 ++++++++++++++++++++++++++++++++++++ 2 files changed, 298 insertions(+), 202 deletions(-) create mode 100644 frappe/tests/test_api_v2.py diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 3825121834..9b14b308ed 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -13,7 +13,6 @@ from filetype import guess_mime from werkzeug.test import TestResponse import frappe -from frappe.core.doctype.user.user import get_timezones from frappe.installer import update_site_config from frappe.tests.utils import FrappeTestCase, patch_hooks from frappe.utils import cint, get_test_client, get_url @@ -75,22 +74,6 @@ class ThreadWithReturnValue(Thread): return self._return -def parameterize(*api_versions): - def decorator(f): - def wrapper(*args, **kwargs): - original_version = args[0].version - try: - for api_version in api_versions: - args[0].version = api_version - f(*args, **kwargs) - finally: - args[0].version = original_version - - return wrapper - - return decorator - - resource_key = { "": "resource", "v1": "resource", @@ -164,13 +147,11 @@ class TestResourceAPI(FrappeAPITestCase): frappe.delete_doc_if_exists(cls.DOCTYPE, name) frappe.db.commit() - @parameterize("", "v1", "v2") def test_unauthorized_call(self): # test 1: fetch documents without auth response = requests.get(self.resource_path(self.DOCTYPE)) self.assertEqual(response.status_code, 403) - @parameterize("", "v1", "v2") def test_get_list(self): # test 2: fetch documents without params response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid}) @@ -178,14 +159,12 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(response.json, dict) self.assertIn("data", response.json) - @parameterize("", "v1", "v2") def test_get_list_limit(self): # test 3: fetch data with limit response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "limit": 2}) self.assertEqual(response.status_code, 200) self.assertEqual(len(response.json["data"]), 2) - @parameterize("", "v1", "v2") def test_get_list_dict(self): # test 4: fetch response as (not) dict response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": True}) @@ -200,7 +179,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(json.data, list) self.assertIsInstance(json.data[0], list) - @parameterize("", "v1") def test_get_list_debug(self): # test 5: fetch response with debug with suppress_stdout(): @@ -210,7 +188,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(response.json["exc"], str) self.assertIsInstance(eval(response.json["exc"]), list) - @parameterize("", "v1", "v2") def test_get_list_fields(self): # test 6: fetch response with fields response = self.get( @@ -220,7 +197,6 @@ class TestResourceAPI(FrappeAPITestCase): json = frappe._dict(response.json) self.assertIn("description", json.data[0]) - @parameterize("", "v1", "v2") def test_create_document(self): data = {"description": frappe.mock("paragraph"), "sid": self.sid} response = self.post(self.resource_path(self.DOCTYPE), data) @@ -229,7 +205,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(docname, str) self.GENERATED_DOCUMENTS.append(docname) - @parameterize("", "v1") def test_update_document(self): generated_desc = frappe.mock("paragraph") data = {"description": generated_desc, "sid": self.sid} @@ -242,7 +217,6 @@ class TestResourceAPI(FrappeAPITestCase): response = self.get(self.resource_path(self.DOCTYPE, random_doc)) self.assertEqual(response.json["data"]["description"], generated_desc) - @parameterize("", "v1", "v2") def test_delete_document(self): doc_to_delete = choice(self.GENERATED_DOCUMENTS) response = self.delete(self.resource_path(self.DOCTYPE, doc_to_delete)) @@ -253,7 +227,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertEqual(response.status_code, 404) self.GENERATED_DOCUMENTS.remove(doc_to_delete) - @parameterize("", "v1") def test_run_doc_method(self): # test 10: Run whitelisted method on doc via /api/resource # status_code is 403 if no other tests are run before this - it's not logged in @@ -278,7 +251,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(data[0], dict) -class TestMethodAPIV1(FrappeAPITestCase): +class TestMethodAPI(FrappeAPITestCase): def test_ping(self): # test 2: test for /api/method/ping response = self.get(self.method_path("ping")) @@ -343,169 +316,6 @@ class TestMethodAPIV1(FrappeAPITestCase): self.assertIn("Traceback", response.json["exc"]) -class TestDocumentAPIV2(TestResourceAPI): - version = "v2" - - def setUp(self) -> None: - self.post(self.method_path("login"), {"sid": self.sid}) - return super().setUp() - - def test_execute_doc_method(self): - response = self.get(self.resource_path("Website Theme", "Standard", "method", "get_apps")) - self.assertEqual(response.json["data"][0]["name"], "frappe") - - def test_update_document(self): - generated_desc = frappe.mock("paragraph") - data = {"description": generated_desc, "sid": self.sid} - random_doc = choice(self.GENERATED_DOCUMENTS) - - response = self.patch(self.resource_path(self.DOCTYPE, random_doc), data=data) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.json["data"]["description"], generated_desc) - - response = self.get(self.resource_path(self.DOCTYPE, random_doc)) - self.assertEqual(response.json["data"]["description"], generated_desc) - - def test_delete_document_non_existing(self): - non_existent_doc = frappe.generate_hash(length=12) - with suppress_stdout(): - response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) - self.assertEqual(response.status_code, 404) - self.assertEqual(response.json["errors"][0]["type"], "DoesNotExistError") - # 404s dont return exceptions - self.assertFalse(response.json["errors"][0].get("exception")) - - -class TestMethodAPIV2(FrappeAPITestCase): - version = "v2" - - def setUp(self) -> None: - self.post(self.method_path("login"), {"sid": self.sid}) - return super().setUp() - - def test_ping(self): - response = self.get(self.method_path("ping")) - self.assertEqual(response.status_code, 200) - self.assertIsInstance(response.json, dict) - self.assertEqual(response.json["data"], "pong") - - def test_get_user_info(self): - response = self.get(self.method_path("frappe.realtime.get_user_info")) - self.assertEqual(response.status_code, 200) - self.assertIsInstance(response.json, dict) - self.assertIn(response.json.get("data").get("user"), ("Administrator", "Guest")) - - def test_auth_cycle(self): - global authorization_token - - generate_admin_keys() - user = frappe.get_doc("User", "Administrator") - api_key, api_secret = user.api_key, user.get_password("api_secret") - authorization_token = f"{api_key}:{api_secret}" - response = self.get(self.method_path("frappe.auth.get_logged_user")) - - self.assertEqual(response.status_code, 200) - self.assertEqual(response.json["data"], "Administrator") - - authorization_token = None - - def test_404s(self): - response = self.get(self.get_path("rest"), {"sid": self.sid}) - self.assertEqual(response.status_code, 404) - response = self.get(self.resource_path("User", "NonExistent@s.com"), {"sid": self.sid}) - self.assertEqual(response.status_code, 404) - - def test_shorthand_controller_methods(self): - shorthand_response = self.get(self.method_path("User", "get_all_roles"), {"sid": self.sid}) - self.assertIn("Blogger", shorthand_response.json["data"]) - - expanded_response = self.get( - self.method_path("frappe.core.doctype.user.user.get_all_roles"), {"sid": self.sid} - ) - self.assertEqual(expanded_response.data, shorthand_response.data) - - def test_logout(self): - self.post(self.method_path("logout"), {"sid": self.sid}) - response = self.get(self.method_path("ping")) - self.assertFalse(response.request.cookies["sid"]) - - def test_run_doc_method_in_memory(self): - dns = frappe.get_doc("Document Naming Settings") - - # Check that simple API can be called. - response = self.get( - self.method_path("run_doc_method"), - { - "sid": self.sid, - "document": dns.as_dict(), - "method": "get_transactions_and_prefixes", - }, - ) - self.assertTrue(response.json["data"]) - self.assertGreaterEqual(len(response.json["docs"]), 1) - - # Call with known and unknown arguments, only known should get passed - response = self.get( - self.method_path("run_doc_method"), - { - "sid": self.sid, - "document": dns.as_dict(), - "method": "get_options", - "kwargs": {"doctype": "Webhook", "unknown": "what"}, - }, - ) - self.assertEqual(response.status_code, 200) - - def test_logs(self): - method = "frappe.tests.test_api.test" - - expected_message = "Failed v2" - response = self.get( - self.method_path(method), {"sid": self.sid, "message": expected_message} - ).json - - self.assertIsInstance(response["messages"], list) - self.assertEqual(response["messages"][0]["message"], expected_message) - - # Cause handled failured - with suppress_stdout(): - response = self.get( - self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True} - ).json - self.assertIsInstance(response["errors"], list) - self.assertEqual(response["errors"][0]["message"], expected_message) - self.assertEqual(response["errors"][0]["type"], "ValidationError") - self.assertIn("Traceback", response["errors"][0]["exception"]) - - # Cause handled failured - with suppress_stdout(): - response = self.get( - self.method_path(method), - {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, - ).json - - self.assertIsInstance(response["errors"], list) - self.assertEqual(response["errors"][0]["type"], "ZeroDivisionError") - self.assertIn("Traceback", response["errors"][0]["exception"]) - - -class TestDocTypeAPIV2(FrappeAPITestCase): - version = "v2" - - def setUp(self) -> None: - self.post(self.method_path("login"), {"sid": self.sid}) - return super().setUp() - - def test_meta(self): - response = self.get(self.doctype_path("ToDo", "meta")) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.json["data"]["name"], "ToDo") - - def test_count(self): - response = self.get(self.doctype_path("ToDo", "count")) - self.assertIsInstance(response.json["data"], int) - - class TestReadOnlyMode(FrappeAPITestCase): """During migration if read only mode can be enabled. Test if reads work well and writes are blocked""" @@ -518,14 +328,12 @@ class TestReadOnlyMode(FrappeAPITestCase): # XXX: this has potential to crumble rest of the test suite. update_site_config("maintenance_mode", 1) - @parameterize("", "v1", "v2") def test_reads(self): response = self.get(self.resource_path("ToDo"), {"sid": self.sid}) self.assertEqual(response.status_code, 200) self.assertIsInstance(response.json, dict) self.assertIsInstance(response.json["data"], list) - @parameterize("", "v1") def test_blocked_writes(self): with suppress_stdout(): response = self.post( @@ -534,15 +342,6 @@ class TestReadOnlyMode(FrappeAPITestCase): self.assertEqual(response.status_code, 503) self.assertEqual(response.json["exc_type"], "InReadOnlyMode") - @parameterize("v2") - def test_blocked_writes_v2(self): - with suppress_stdout(): - response = self.post( - self.resource_path("ToDo"), {"description": frappe.mock("paragraph"), "sid": self.sid} - ) - self.assertEqual(response.status_code, 503) - self.assertEqual(response.json["errors"][0]["type"], "InReadOnlyMode") - class TestWSGIApp(FrappeAPITestCase): def test_request_hooks(self): diff --git a/frappe/tests/test_api_v2.py b/frappe/tests/test_api_v2.py new file mode 100644 index 0000000000..ba25b8b05d --- /dev/null +++ b/frappe/tests/test_api_v2.py @@ -0,0 +1,297 @@ +from random import choice + +import requests + +import frappe +from frappe.installer import update_site_config +from frappe.tests.test_api import FrappeAPITestCase, suppress_stdout + +authorization_token = None + + +resource_key = { + "": "resource", + "v1": "resource", + "v2": "document", +} + + +class TestResourceAPIV2(FrappeAPITestCase): + version = "v2" + DOCTYPE = "ToDo" + GENERATED_DOCUMENTS = [] + + @classmethod + def setUpClass(cls): + super().setUpClass() + for _ in range(20): + doc = frappe.get_doc({"doctype": "ToDo", "description": frappe.mock("paragraph")}).insert() + cls.GENERATED_DOCUMENTS = [] + cls.GENERATED_DOCUMENTS.append(doc.name) + frappe.db.commit() + + @classmethod + def tearDownClass(cls): + for name in cls.GENERATED_DOCUMENTS: + frappe.delete_doc_if_exists(cls.DOCTYPE, name) + frappe.db.commit() + + def test_unauthorized_call(self): + # test 1: fetch documents without auth + response = requests.get(self.resource_path(self.DOCTYPE)) + self.assertEqual(response.status_code, 403) + + def test_get_list(self): + # test 2: fetch documents without params + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid}) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertIn("data", response.json) + + def test_get_list_limit(self): + # test 3: fetch data with limit + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "limit": 2}) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.json["data"]), 2) + + def test_get_list_dict(self): + # test 4: fetch response as (not) dict + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": True}) + json = frappe._dict(response.json) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(json.data, list) + self.assertIsInstance(json.data[0], dict) + + response = self.get(self.resource_path(self.DOCTYPE), {"sid": self.sid, "as_dict": False}) + json = frappe._dict(response.json) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(json.data, list) + self.assertIsInstance(json.data[0], list) + + def test_get_list_fields(self): + # test 6: fetch response with fields + response = self.get( + self.resource_path(self.DOCTYPE), {"sid": self.sid, "fields": '["description"]'} + ) + self.assertEqual(response.status_code, 200) + json = frappe._dict(response.json) + self.assertIn("description", json.data[0]) + + def test_create_document(self): + data = {"description": frappe.mock("paragraph"), "sid": self.sid} + response = self.post(self.resource_path(self.DOCTYPE), data) + self.assertEqual(response.status_code, 200) + docname = response.json["data"]["name"] + self.assertIsInstance(docname, str) + self.GENERATED_DOCUMENTS.append(docname) + + def test_delete_document(self): + doc_to_delete = choice(self.GENERATED_DOCUMENTS) + response = self.delete(self.resource_path(self.DOCTYPE, doc_to_delete)) + self.assertEqual(response.status_code, 202) + self.assertDictEqual(response.json, {"data": "ok"}) + + response = self.get(self.resource_path(self.DOCTYPE, doc_to_delete)) + self.assertEqual(response.status_code, 404) + self.GENERATED_DOCUMENTS.remove(doc_to_delete) + + def test_execute_doc_method(self): + response = self.get(self.resource_path("Website Theme", "Standard", "method", "get_apps")) + self.assertEqual(response.json["data"][0]["name"], "frappe") + + def test_update_document(self): + generated_desc = frappe.mock("paragraph") + data = {"description": generated_desc, "sid": self.sid} + random_doc = choice(self.GENERATED_DOCUMENTS) + + response = self.patch(self.resource_path(self.DOCTYPE, random_doc), data=data) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"]["description"], generated_desc) + + response = self.get(self.resource_path(self.DOCTYPE, random_doc)) + self.assertEqual(response.json["data"]["description"], generated_desc) + + def test_delete_document_non_existing(self): + non_existent_doc = frappe.generate_hash(length=12) + with suppress_stdout(): + response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json["errors"][0]["type"], "DoesNotExistError") + # 404s dont return exceptions + self.assertFalse(response.json["errors"][0].get("exception")) + + +class TestMethodAPIV2(FrappeAPITestCase): + version = "v2" + + def setUp(self) -> None: + self.post(self.method_path("login"), {"sid": self.sid}) + return super().setUp() + + def test_ping(self): + response = self.get(self.method_path("ping")) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertEqual(response.json["data"], "pong") + + def test_get_user_info(self): + response = self.get(self.method_path("frappe.realtime.get_user_info")) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertIn(response.json.get("data").get("user"), ("Administrator", "Guest")) + + def test_auth_cycle(self): + global authorization_token + + generate_admin_keys() + user = frappe.get_doc("User", "Administrator") + api_key, api_secret = user.api_key, user.get_password("api_secret") + authorization_token = f"{api_key}:{api_secret}" + response = self.get(self.method_path("frappe.auth.get_logged_user")) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"], "Administrator") + + authorization_token = None + + def test_404s(self): + response = self.get(self.get_path("rest"), {"sid": self.sid}) + self.assertEqual(response.status_code, 404) + response = self.get(self.resource_path("User", "NonExistent@s.com"), {"sid": self.sid}) + self.assertEqual(response.status_code, 404) + + def test_shorthand_controller_methods(self): + shorthand_response = self.get(self.method_path("User", "get_all_roles"), {"sid": self.sid}) + self.assertIn("Blogger", shorthand_response.json["data"]) + + expanded_response = self.get( + self.method_path("frappe.core.doctype.user.user.get_all_roles"), {"sid": self.sid} + ) + self.assertEqual(expanded_response.data, shorthand_response.data) + + def test_logout(self): + self.post(self.method_path("logout"), {"sid": self.sid}) + response = self.get(self.method_path("ping")) + self.assertFalse(response.request.cookies["sid"]) + + def test_run_doc_method_in_memory(self): + dns = frappe.get_doc("Document Naming Settings") + + # Check that simple API can be called. + response = self.get( + self.method_path("run_doc_method"), + { + "sid": self.sid, + "document": dns.as_dict(), + "method": "get_transactions_and_prefixes", + }, + ) + self.assertTrue(response.json["data"]) + self.assertGreaterEqual(len(response.json["docs"]), 1) + + # Call with known and unknown arguments, only known should get passed + response = self.get( + self.method_path("run_doc_method"), + { + "sid": self.sid, + "document": dns.as_dict(), + "method": "get_options", + "kwargs": {"doctype": "Webhook", "unknown": "what"}, + }, + ) + self.assertEqual(response.status_code, 200) + + def test_logs(self): + method = "frappe.tests.test_api.test" + + expected_message = "Failed v2" + response = self.get( + self.method_path(method), {"sid": self.sid, "message": expected_message} + ).json + + self.assertIsInstance(response["messages"], list) + self.assertEqual(response["messages"][0]["message"], expected_message) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True} + ).json + self.assertIsInstance(response["errors"], list) + self.assertEqual(response["errors"][0]["message"], expected_message) + self.assertEqual(response["errors"][0]["type"], "ValidationError") + self.assertIn("Traceback", response["errors"][0]["exception"]) + + # Cause handled failured + with suppress_stdout(): + response = self.get( + self.method_path(method), + {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, + ).json + + self.assertIsInstance(response["errors"], list) + self.assertEqual(response["errors"][0]["type"], "ZeroDivisionError") + self.assertIn("Traceback", response["errors"][0]["exception"]) + + +class TestDocTypeAPIV2(FrappeAPITestCase): + version = "v2" + + def setUp(self) -> None: + self.post(self.method_path("login"), {"sid": self.sid}) + return super().setUp() + + def test_meta(self): + response = self.get(self.doctype_path("ToDo", "meta")) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json["data"]["name"], "ToDo") + + def test_count(self): + response = self.get(self.doctype_path("ToDo", "count")) + self.assertIsInstance(response.json["data"], int) + + +class TestReadOnlyMode(FrappeAPITestCase): + """During migration if read only mode can be enabled. + Test if reads work well and writes are blocked""" + + version = "v2" + + @classmethod + def setUpClass(cls): + super().setUpClass() + update_site_config("allow_reads_during_maintenance", 1) + cls.addClassCleanup(update_site_config, "maintenance_mode", 0) + update_site_config("maintenance_mode", 1) + + def test_reads(self): + response = self.get(self.resource_path("ToDo"), {"sid": self.sid}) + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json, dict) + self.assertIsInstance(response.json["data"], list) + + def test_blocked_writes_v2(self): + with suppress_stdout(): + response = self.post( + self.resource_path("ToDo"), {"description": frappe.mock("paragraph"), "sid": self.sid} + ) + self.assertEqual(response.status_code, 503) + self.assertEqual(response.json["errors"][0]["type"], "InReadOnlyMode") + + +def generate_admin_keys(): + from frappe.core.doctype.user.user import generate_keys + + generate_keys("Administrator") + frappe.db.commit() + + +@frappe.whitelist() +def test(*, fail=False, handled=True, message="Failed"): + if fail: + if handled: + frappe.throw(message) + else: + 1 / 0 + else: + frappe.msgprint(message)