From 724a5b2536850cfc0d34deb21688507c10903060 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 30 Apr 2021 21:09:24 +0530 Subject: [PATCH 1/6] fix: Evaluate boolean values better via /api/resource/ `GET /api/resource/ToDo?limit=10&debug=False&as_dict=0` would be received by the resource handler as debug="False" and as_dict="0" which are both truthy values. So, even though you requested for a list of lists response without debugging on, you'd get the exact opposite; debug on and a list of dicts. - Evaluate boolean values for `GET /api/resource/` - Added `limit` parameter as an alias for `limit_page_length` - Added `frappe.utils.data.sbool` that converts strings to bool values if applicable. - Added some seemingly stupid comments for the sake of consistency. --- frappe/api.py | 47 +++++++++++++++++++++++++++++--------------- frappe/utils/data.py | 20 +++++++++++++++++++ 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/frappe/api.py b/frappe/api.py index 9039ae0e5f..c69f76a755 100644 --- a/frappe/api.py +++ b/frappe/api.py @@ -11,6 +11,7 @@ import frappe.client import frappe.handler from frappe import _ from frappe.utils.response import build_response +from frappe.utils.data import sbool def handle(): @@ -108,25 +109,39 @@ def handle(): elif doctype: if frappe.local.request.method == "GET": - if frappe.local.form_dict.get('fields'): - frappe.local.form_dict['fields'] = json.loads(frappe.local.form_dict['fields']) - frappe.local.form_dict.setdefault('limit_page_length', 20) - frappe.local.response.update({ - "data": frappe.call( - frappe.client.get_list, - doctype, - **frappe.local.form_dict - ) - }) + # set fields for frappe.get_list + 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 + frappe.local.form_dict.update( + {x: sbool(y) for x, y in frappe.local.form_dict.items()} + ) + + # 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}) if frappe.local.request.method == "POST": + # fetch data from from dict data = get_request_form_data() - data.update({ - "doctype": doctype - }) - frappe.local.response.update({ - "data": frappe.get_doc(data).insert().as_dict() - }) + 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() else: raise frappe.DoesNotExistError diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 3ffa8dc874..9d81aac467 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -622,6 +622,26 @@ def ceil(s): def cstr(s, encoding='utf-8'): return frappe.as_unicode(s, encoding) +def sbool(x): + """Converts str object to Boolean if possible. + Example: + "true" becomes True + "1" becomes True + "{}" remains "{}" + + Args: + x (str): String to be converted to Bool + + Returns: + object: Returns Boolean or type(x) + """ + from distutils.util import strtobool + + try: + return bool(strtobool(x)) + except Exception: + return x + def rounded(num, precision=0): """round method for round halfs to nearest even algorithm aka banker's rounding - compatible with python3""" precision = cint(precision) From 98100437d0f3476c88814159929e3a73dbabea40 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 20 May 2021 16:52:48 +0530 Subject: [PATCH 2/6] chore: Renamed test files "appropriately" --- frappe/tests/{test_api.py => test_frappe_client.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename frappe/tests/{test_api.py => test_frappe_client.py} (100%) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_frappe_client.py similarity index 100% rename from frappe/tests/test_api.py rename to frappe/tests/test_frappe_client.py From bcbc14d047cc16b3847d03bc7db4b28d0b2f363a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 20 May 2021 17:11:12 +0530 Subject: [PATCH 3/6] chore: Rename test suite and remove unused imports --- frappe/tests/test_frappe_client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frappe/tests/test_frappe_client.py b/frappe/tests/test_frappe_client.py index 6453062877..e1cdbb6ccd 100644 --- a/frappe/tests/test_frappe_client.py +++ b/frappe/tests/test_frappe_client.py @@ -1,8 +1,7 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See license.txt -from __future__ import unicode_literals -import unittest, frappe, os +import unittest, frappe from frappe.core.doctype.user.user import generate_keys from frappe.frappeclient import FrappeClient, FrappeException from frappe.utils.data import get_url @@ -10,7 +9,7 @@ from frappe.utils.data import get_url import requests import base64 -class TestAPI(unittest.TestCase): +class TestFrappeClient(unittest.TestCase): def test_insert_many(self): server = FrappeClient(get_url(), "Administrator", "admin", verify=False) frappe.db.sql("delete from `tabNote` where title in ('Sing','a','song','of','sixpence')") From 4a814571a8008672cbae058351a21d9a8665b714 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 20 May 2021 19:26:11 +0530 Subject: [PATCH 4/6] test: Added tests for Frappe APIs * Added tests for /api/resource usage * Added tests for /api/method base endpoints --- frappe/tests/test_api.py | 170 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 frappe/tests/test_api.py diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py new file mode 100644 index 0000000000..4d00df22a5 --- /dev/null +++ b/frappe/tests/test_api.py @@ -0,0 +1,170 @@ +import unittest +from random import choice + +import requests +from semantic_version import Version + +import frappe +from frappe.utils import get_site_url + + +def maintain_state(f): + def wrapper(*args, **kwargs): + frappe.db.rollback() + r = f(*args, **kwargs) + frappe.db.commit() + return r + + return wrapper + + +class TestResourceAPI(unittest.TestCase): + SITE_URL = get_site_url(frappe.local.site) + RESOURCE_URL = f"{SITE_URL}/api/resource" + DOCTYPE = "ToDo" + GENERATED_DOCUMENTS = [] + + @classmethod + @maintain_state + def setUpClass(self): + for _ in range(10): + doc = frappe.get_doc( + {"doctype": "ToDo", "description": frappe.mock("paragraph")} + ).insert() + self.GENERATED_DOCUMENTS.append(doc.name) + + @classmethod + @maintain_state + def tearDownClass(self): + for name in self.GENERATED_DOCUMENTS: + frappe.delete_doc_if_exists(self.DOCTYPE, name) + + @property + def sid(self): + if not getattr(self, "_sid", None): + self._sid = requests.post( + f"{self.SITE_URL}/api/method/login", + data={ + "usr": "Administrator", + "pwd": "root" or frappe.conf.admin_password or "admin", + }, + ).cookies.get("sid") + + return self._sid + + def get(self, path, params=""): + return requests.get(f"{self.RESOURCE_URL}/{path}?sid={self.sid}{params}") + + def post(self, path, data): + return requests.post( + f"{self.RESOURCE_URL}/{path}?sid={self.sid}", data=frappe.as_json(data) + ) + + def put(self, path, data): + return requests.put( + f"{self.RESOURCE_URL}/{path}?sid={self.sid}", data=frappe.as_json(data) + ) + + def delete(self, path): + return requests.delete(f"{self.RESOURCE_URL}/{path}?sid={self.sid}") + + def test_unauthorized_call(self): + # test 1: fetch documents without auth + response = requests.get(f"{self.RESOURCE_URL}/{self.DOCTYPE}") + self.assertEqual(response.status_code, 403) + + def test_get_list(self): + # test 2: fetch documents without params + response = self.get(self.DOCTYPE) + 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.DOCTYPE, "&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.DOCTYPE, "&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.DOCTYPE, "&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_debug(self): + # test 5: fetch response with debug + response = self.get(self.DOCTYPE, "&debug=true") + self.assertEqual(response.status_code, 200) + self.assertIn("exc", response.json()) + self.assertIsInstance(response.json()["exc"], str) + self.assertIsInstance(eval(response.json()["exc"]), list) + + def test_get_list_fields(self): + # test 6: fetch response with fields + response = self.get(self.DOCTYPE, r'&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 + data = {"description": frappe.mock("paragraph")} + response = self.post(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 + generated_desc = frappe.mock("paragraph") + data = {"description": generated_desc} + random_doc = choice(self.GENERATED_DOCUMENTS) + desc_before_update = frappe.db.get_value(self.DOCTYPE, random_doc, "description") + + response = self.put(f"{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 + doc_to_delete = choice(self.GENERATED_DOCUMENTS) + response = self.delete(f"{self.DOCTYPE}/{doc_to_delete}") + self.assertEqual(response.status_code, 202) + self.assertDictEqual(response.json(), {"message": "ok"}) + + non_existent_doc = frappe.generate_hash(length=12) + response = self.delete(f"{self.DOCTYPE}/{non_existent_doc}") + self.assertEqual(response.status_code, 404) + self.assertDictEqual(response.json(), {}) + + +class TestMethodAPI(unittest.TestCase): + METHOD_URL = f"{get_site_url(frappe.local.site)}/api/method" + + def test_version(self): + # test 1: test for /api/method/version + response = requests.get(f"{self.METHOD_URL}/version") + json = frappe._dict(response.json()) + + self.assertEqual(response.status_code, 200) + self.assertIsInstance(json, dict) + self.assertIsInstance(json.message, str) + self.assertEqual(Version(json.message), Version(frappe.__version__)) + + def test_ping(self): + # test 2: test for /api/method/ping + response = requests.get(f"{self.METHOD_URL}/ping") + self.assertEqual(response.status_code, 200) + self.assertIsInstance(response.json(), dict) + self.assertEqual(response.json()['message'], "pong") From dfc23eb3a3e42869430cf686a29f0b4188894618 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 21 May 2021 11:22:19 +0530 Subject: [PATCH 5/6] fix(test): Set admin password correctly --- frappe/tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index 4d00df22a5..7e77aab779 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -46,7 +46,7 @@ class TestResourceAPI(unittest.TestCase): f"{self.SITE_URL}/api/method/login", data={ "usr": "Administrator", - "pwd": "root" or frappe.conf.admin_password or "admin", + "pwd": frappe.conf.admin_password or "admin", }, ).cookies.get("sid") From b215921a971e3b95ff6f93d63da3ffc70a10766f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 25 May 2021 21:44:54 +0530 Subject: [PATCH 6/6] fix: Convert only as_dict and debug values to bool Given the scope of its usage at this point, this becomes a problem when you'd have a field named y,n, true, false and order_by that field, or have the same values for a document name that parent parameter would accept out of all those that Frappe REST allows. --- frappe/api.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frappe/api.py b/frappe/api.py index c69f76a755..6427cbfbd8 100644 --- a/frappe/api.py +++ b/frappe/api.py @@ -119,10 +119,11 @@ def handle(): frappe.local.form_dict.limit or frappe.local.form_dict.limit_page_length or 20, ) - # convert strings to native types - frappe.local.form_dict.update( - {x: sbool(y) for x, y in frappe.local.form_dict.items()} - ) + # 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)