vassili/seitime-frappe
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Merge pull request #16805 from phot0n/autoinc-customize-form-fix

Browse source 
fix: don't allow setting autoincrement autoname from customize form
This commit is contained in:
Suraj Shetty 2022-05-13 07:27:15 +05:30 • committed by GitHub
commit a5995b2a10
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 170 additions and 76 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

67
frappe/core/doctype/doctype/doctype.py
View file

@ -100,6 +100,7 @@ class DocType(Document):
self.set_default_in_list_view()
self.set_default_translatable()
validate_series(self)
self.set("can_change_name_type", validate_autoincrement_autoname(self))
self.validate_document_type()
validate_fields(self)
@ -124,12 +125,6 @@ class DocType(Document):
if self.default_print_format and not self.custom:
frappe.throw(_("Standard DocType cannot have default print format, use Customize Form"))
if check_if_can_change_name_type(self):
change_name_column_type(
self.name,
"bigint" if self.autoname == "autoincrement" else f"varchar({frappe.db.VARCHAR_LEN})",
)
def validate_field_name_conflicts(self):
"""Check if field names dont conflict with controller properties and methods"""
core_doctypes = [
@ -374,6 +369,10 @@ class DocType(Document):
def on_update(self):
"""Update database schema, make controller templates if `custom` is not set and clear cache."""
if self.get("can_change_name_type"):
self.setup_autoincrement_and_sequence()
try:
frappe.db.updatedb(self.name, Meta(self))
except Exception as e:
@ -413,6 +412,17 @@ class DocType(Document):
clear_linked_doctype_cache()
def setup_autoincrement_and_sequence(self):
"""Changes name type and makes sequence on change (if required)"""
name_type = f"varchar({frappe.db.VARCHAR_LEN})"
if self.autoname == "autoincrement":
name_type = "bigint"
frappe.db.create_sequence(self.name, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
change_name_column_type(self.name, name_type)
def sync_global_search(self):
"""If global search settings are changed, rebuild search properties for this table"""
global_search_fields_before_update = [
@ -903,26 +913,25 @@ def validate_series(dt, autoname=None, name=None):
frappe.throw(_("Series {0} already used in {1}").format(prefix, used_in[0][0]))
def check_if_can_change_name_type(dt: DocType, raise_err: bool = True) -> bool:
def get_autoname_before_save(doctype: str, to_be_customized_dt: str) -> str:
if doctype == "Customize Form":
property_value = frappe.db.get_value(
"Property Setter", {"doc_type": to_be_customized_dt, "property": "autoname"}, "value"
)
def validate_autoincrement_autoname(dt: DocType) -> bool:
"""Checks if can doctype can change to/from autoincrement autoname"""
def get_autoname_before_save(dt: DocType) -> str:
if dt.name == "Customize Form":
property_value = frappe.db.get_value(
"Property Setter", {"doc_type": dt.doc_type, "property": "autoname"}, "value"
)
# initially no property setter is set,
# hence getting autoname value from the doctype itself
if not property_value:
return frappe.db.get_value("DocType", to_be_customized_dt, "autoname") or ""
return frappe.db.get_value("DocType", dt.doc_type, "autoname") or ""
return property_value
return getattr(dt.get_doc_before_save(), "autoname", "")
doctype_name = dt.doc_type if dt.doctype == "Customize Form" else dt.name
if not dt.is_new():
autoname_before_save = get_autoname_before_save(dt.doctype, doctype_name)
autoname_before_save = get_autoname_before_save(dt)
is_autoname_autoincrement = dt.autoname == "autoincrement"
if (
@ -930,23 +939,35 @@ def check_if_can_change_name_type(dt: DocType, raise_err: bool = True) -> bool:
and autoname_before_save != "autoincrement"
or (not is_autoname_autoincrement and autoname_before_save == "autoincrement")
):
if not frappe.get_all(doctype_name, limit=1):
if frappe.get_meta(dt.name).issingle:
if dt.name == "Customize Form":
frappe.throw(_("Cannot change to/from autoincrement autoname in Customize Form"))
return False
if not frappe.get_all(dt.name, limit=1):
# allow changing the column type if there is no data
return True
if raise_err:
frappe.throw(
_("Can only change to/from Autoincrement naming rule when there is no data in the doctype")
)
frappe.throw(
_("Can only change to/from Autoincrement naming rule when there is no data in the doctype")
)
return False
def change_name_column_type(doctype_name: str, type: str) -> None:
return frappe.db.change_column_type(
doctype_name, "name", type, True if frappe.db.db_type == "mariadb" else False
"""Changes name column type"""
args = (
(doctype_name, "name", type, False, True)
if (frappe.db.db_type == "postgres")
else (doctype_name, "name", type, True)
)
frappe.db.change_column_type(*args)
def validate_links_table_fieldnames(meta):
"""Validate fieldnames in Links table"""

46
frappe/core/doctype/doctype/test_doctype.py
View file

@ -38,6 +38,52 @@ class TestDocType(unittest.TestCase):
doc = new_doctype(name).insert()
doc.delete()
def test_making_sequence_on_change(self):
frappe.delete_doc_if_exists("DocType", self._testMethodName)
dt = new_doctype(self._testMethodName).insert(ignore_permissions=True)
autoname = dt.autoname
# change autoname
dt.autoname = "autoincrement"
dt.save()
# check if name type has been changed
self.assertEqual(
frappe.db.sql(
f"""select data_type FROM information_schema.columns
where column_name = 'name' and table_name = 'tab{self._testMethodName}'"""
)[0][0],
"bigint",
)
if frappe.db.db_type == "mariadb":
table_name = "information_schema.tables"
conditions = f"table_type = 'sequence' and table_name = '{self._testMethodName}_id_seq'"
else:
table_name = "information_schema.sequences"
conditions = f"sequence_name = '{self._testMethodName}_id_seq'"
# check if sequence table is created
self.assertTrue(
frappe.db.sql(
f"""select * from {table_name}
where {conditions}"""
)
)
# change the autoname/naming rule back to original
dt.autoname = autoname
dt.save()
# check if name type has changed
self.assertEqual(
frappe.db.sql(
f"""select data_type FROM information_schema.columns
where column_name = 'name' and table_name = 'tab{self._testMethodName}'"""
)[0][0],
"varchar" if frappe.db.db_type == "mariadb" else "character varying",
)
def test_doctype_unique_constraint_dropped(self):
if frappe.db.exists("DocType", "With_Unique"):
frappe.delete_doc("DocType", "With_Unique")

4
frappe/custom/doctype/customize_form/customize_form.json
View file

@ -287,7 +287,7 @@
"label": "Naming"
},
{
"description": "Naming Options:\n<ol><li><b>field:[fieldname]</b> - By Field</li>\n<li><b>autoincrement</b> - Uses Databases' Auto Increment feature</li><li><b>naming_series:</b> - By Naming Series (field called naming_series must be present</li><li><b>Prompt</b> - Prompt user for a name</li><li><b>[series]</b> - Series by prefix (separated by a dot); for example PRE.#####</li>\n<li><b>format:EXAMPLE-{MM}morewords{fieldname1}-{fieldname2}-{#####}</b> - Replace all braced words (fieldnames, date words (DD, MM, YY), series) with their value. Outside braces, any characters can be used.</li></ol>",
"description": "Naming Options:\n<ol><li><b>field:[fieldname]</b> - By Field</li><li><b>naming_series:</b> - By Naming Series (field called naming_series must be present)</li><li><b>Prompt</b> - Prompt user for a name</li><li><b>[series]</b> - Series by prefix (separated by a dot); for example PRE.#####</li>\n<li><b>format:EXAMPLE-{MM}morewords{fieldname1}-{fieldname2}-{#####}</b> - Replace all braced words (fieldnames, date words (DD, MM, YY), series) with their value. Outside braces, any characters can be used.</li></ol>",
"fieldname": "autoname",
"fieldtype": "Data",
"label": "Auto Name"
@ -319,7 +319,7 @@
"index_web_pages_for_search": 1,
"issingle": 1,
"links": [],
"modified": "2022-04-21 15:36:16.772277",
"modified": "2022-05-12 15:36:16.772277",
"modified_by": "Administrator",
"module": "Custom",
"name": "Customize Form",

11
frappe/custom/doctype/customize_form/customize_form.py
View file

@ -11,9 +11,8 @@ import frappe
import frappe.translate
from frappe import _
from frappe.core.doctype.doctype.doctype import (
change_name_column_type,
check_email_append_to,
check_if_can_change_name_type,
validate_autoincrement_autoname,
validate_fields_for_doctype,
validate_series,
)
@ -163,7 +162,7 @@ class CustomizeForm(Document):
return
validate_series(self, self.autoname, self.doc_type)
can_change_name_type = check_if_can_change_name_type(self)
validate_autoincrement_autoname(self)
self.flags.update_db = False
self.flags.rebuild_doctype_for_global_search = False
self.set_property_setters()
@ -172,12 +171,6 @@ class CustomizeForm(Document):
validate_fields_for_doctype(self.doc_type)
check_email_append_to(self)
if can_change_name_type:
change_name_column_type(
self.doc_type,
"bigint" if self.autoname == "autoincrement" else f"varchar({frappe.db.VARCHAR_LEN})",
)
if self.flags.update_db:
frappe.db.updatedb(self.doc_type)

7
frappe/custom/doctype/customize_form/test_customize_form.py
View file

@ -396,3 +396,10 @@ class TestCustomizeForm(unittest.TestCase):
d.label = ""
d.run_method("save_customization")
self.assertEqual(d.label, "")
def test_change_to_autoincrement_autoname(self):
d = self.get_customize_form("Event")
d.autoname = "autoincrement"
with self.assertRaises(frappe.ValidationError):
d.run_method("save_customization")

9
frappe/database/mariadb/database.py
View file

@ -19,6 +19,15 @@ class MariaDBDatabase(Database):
DataError = pymysql.err.DataError
REGEX_CHARACTER = "regexp"
# NOTE: using a very small cache - as during backup, if the sequence was used in anyform,
# it drops the cache and uses the next non cached value in setval query and
# puts that in the backup file, which will start the counter
# from that value when inserting any new record in the doctype.
# By default the cache is 1000 which will mess up the sequence when
# using the system after a restore.
# issue link: https://jira.mariadb.org/browse/MDEV-21786
SEQUENCE_CACHE = 50
def setup_type_map(self):
self.db_type = "mariadb"
self.type_map = {

9
frappe/database/mariadb/schema.py
View file

@ -40,14 +40,7 @@ class MariaDBTable(DBTable):
not self.meta.issingle and self.meta.autoname == "autoincrement"
) or self.doctype in log_types:
# NOTE: using a very small cache - as during backup, if the sequence was used in anyform,
# it drops the cache and uses the next non cached value in setval func and
# puts that in the backup file, which will start the counter
# from that value when inserting any new record in the doctype.
# By default the cache is 1000 which will mess up the sequence when
# using the system after a restore.
# issue link: https://jira.mariadb.org/browse/MDEV-21786
frappe.db.create_sequence(self.doctype, check_not_exists=True, cache=50)
frappe.db.create_sequence(self.doctype, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
# NOTE: not used nextval func as default as the ability to restore
# database with sequences has bugs in mariadb and gives a scary error.

13
frappe/database/postgres/database.py
View file

@ -31,6 +31,12 @@ class PostgresDatabase(Database):
InterfaceError = psycopg2.InterfaceError
REGEX_CHARACTER = "~"
# NOTE; The sequence cache for postgres is per connection.
# Since we're opening and closing connections for every transaction this results in skipping the cache
# to the next non-cached value hence not using cache in postgres.
# ref: https://stackoverflow.com/questions/21356375/postgres-9-0-4-sequence-skipping-numbers
SEQUENCE_CACHE = 0
def setup_type_map(self):
self.db_type = "postgres"
self.type_map = {
@ -209,18 +215,19 @@ class PostgresDatabase(Database):
)
def change_column_type(
self, doctype: str, column: str, type: str, nullable: bool = False
self, doctype: str, column: str, type: str, nullable: bool = False, use_cast: bool = False
) -> Union[List, Tuple]:
table_name = get_table_name(doctype)
null_constraint = "SET NOT NULL" if not nullable else "DROP NOT NULL"
using_cast = f'using "{column}"::{type}' if use_cast else ""
# postgres allows ddl in transactions but since we've currently made
# things same as mariadb (raising exception on ddl commands if the transaction has any writes),
# hence using sql_ddl here for committing and then moving forward.
return self.sql_ddl(
f"""ALTER TABLE "{table_name}"
ALTER COLUMN "{column}" TYPE {type},
ALTER COLUMN "{column}" {null_constraint}"""
ALTER COLUMN "{column}" TYPE {type} {using_cast},
ALTER COLUMN "{column}" {null_constraint}"""
)
def create_auth_table(self):

6
frappe/database/postgres/schema.py
View file

@ -34,11 +34,7 @@ class PostgresTable(DBTable):
not self.meta.issingle and self.meta.autoname == "autoincrement"
) or self.doctype in log_types:
# The sequence cache is per connection.
# Since we're opening and closing connections for every transaction this results in skipping the cache
# to the next non-cached value hence not using cache in postgres.
# ref: https://stackoverflow.com/questions/21356375/postgres-9-0-4-sequence-skipping-numbers
frappe.db.create_sequence(self.doctype, check_not_exists=True)
frappe.db.create_sequence(self.doctype, check_not_exists=True, cache=frappe.db.SEQUENCE_CACHE)
name_column = "name bigint primary key"
# TODO: set docstatus length

4
frappe/database/sequence.py
View file

@ -5,7 +5,7 @@ def create_sequence(
doctype_name: str,
*,
slug: str = "_id_seq",
temporary=False,
temporary: bool = False,
check_not_exists: bool = False,
cycle: bool = False,
cache: int = 0,
@ -51,7 +51,7 @@ def create_sequence(
else:
query += " cycle"
db.sql(query)
db.sql_ddl(query)
return sequence_name

3
frappe/desk/doctype/event/event.json
View file

@ -277,7 +277,7 @@
"icon": "fa fa-calendar",
"idx": 1,
"links": [],
"modified": "2021-11-18 05:06:24.881742",
"modified": "2022-05-12 05:43:27.935510",
"modified_by": "Administrator",
"module": "Desk",
"name": "Event",
@ -312,6 +312,7 @@
"sender_field": "sender",
"sort_field": "modified",
"sort_order": "DESC",
"states": [],
"subject_field": "subject",
"title_field": "subject",
"track_changes": 1,

6
frappe/model/db_query.py
View file

@ -920,12 +920,12 @@ def cast_name(column: str) -> str:
kwargs = {"string": column, "flags": re.IGNORECASE}
if "cast(" not in column.lower() and "::" not in column:
if re.search(r"locate\(([^,]+),\s*([`\"]?name[`\"]?)\s*\)", **kwargs):
if re.search(r"locate\([^,]+,\s*[`\"]?name[`\"]?\s*\)", **kwargs):
return re.sub(
r"locate\(([^,]+),\s*([`\"]?name[`\"]?)\)", r"locate(\1, cast(\2 as varchar))", **kwargs
r"locate\(([^,]+),\s*([`\"]?name[`\"]?)\s*\)", r"locate(\1, cast(\2 as varchar))", **kwargs
)
elif match := re.search(r"(strpos|ifnull|coalesce)\(\s*([`\"]?name[`\"]?)\s*,", **kwargs):
elif match := re.search(r"(strpos|ifnull|coalesce)\(\s*[`\"]?name[`\"]?\s*,", **kwargs):
func = match.groups()[0]
return re.sub(rf"{func}\(\s*([`\"]?name[`\"]?)\s*,", rf"{func}(cast(\1 as varchar),", **kwargs)

6
frappe/public/js/frappe/form/controls/base_input.js
View file

@ -65,7 +65,11 @@ frappe.ui.form.ControlInput = class ControlInput extends frappe.ui.form.Control
};
var update_input = function() {
me.set_input(me.value);
if (me.doctype && me.docname) {
me.set_input(me.value);
} else {
me.set_input(me.value || null);
}
};
if (me.disp_status != "None") {

55
frappe/tests/test_db.py
View file

@ -551,10 +551,16 @@ class TestDDLCommandsMaria(unittest.TestCase):
)
def test_change_type(self) -> None:
def get_table_description():
return frappe.db.sql(f"DESC `tab{self.test_table_name}`")
# try changing from int to varchar
frappe.db.change_column_type("TestNotes", "id", "varchar(255)")
test_table_description = frappe.db.sql(f"DESC tab{self.test_table_name};")
self.assertGreater(len(test_table_description), 0)
self.assertIn("varchar(255)", test_table_description[0])
self.assertIn("varchar(255)", get_table_description()[0])
# try changing from varchar to bigint
frappe.db.change_column_type("TestNotes", "id", "bigint")
self.assertIn("bigint(20)", get_table_description()[0])
def test_add_index(self) -> None:
index_name = "test_index"
@ -763,21 +769,34 @@ class TestDDLCommandsPost(unittest.TestCase):
self.assertEqual([("id",), ("content",)], frappe.db.describe(self.test_table_name))
def test_change_type(self) -> None:
from psycopg2.errors import DatatypeMismatch
def get_table_description():
return frappe.db.sql(
f"""
SELECT
table_name,
column_name,
data_type
FROM
information_schema.columns
WHERE
table_name = 'tab{self.test_table_name}'"""
)
# try changing from int to varchar
frappe.db.change_column_type(self.test_table_name, "id", "varchar(255)")
check_change = frappe.db.sql(
f"""
SELECT
table_name,
column_name,
data_type
FROM
information_schema.columns
WHERE
table_name = 'tab{self.test_table_name}'
"""
)
self.assertGreater(len(check_change), 0)
self.assertIn("character varying", check_change[0])
self.assertIn("character varying", get_table_description()[0])
# try changing from varchar to int
try:
frappe.db.change_column_type(self.test_table_name, "id", "bigint")
except DatatypeMismatch:
frappe.db.rollback()
# try changing from varchar to int (using cast)
frappe.db.change_column_type(self.test_table_name, "id", "bigint", use_cast=True)
self.assertIn("bigint", get_table_description()[0])
def test_add_index(self) -> None:
index_name = "test_index"
@ -792,7 +811,6 @@ class TestDDLCommandsPost(unittest.TestCase):
)
self.assertEqual(len(indexs_in_table), 1)
@run_only_if(db_type_is.POSTGRES)
def test_modify_query(self):
from frappe.database.postgres.database import modify_query
@ -810,7 +828,6 @@ class TestDDLCommandsPost(unittest.TestCase):
modify_query(query),
)
@run_only_if(db_type_is.POSTGRES)
def test_modify_values(self):
from frappe.database.postgres.database import modify_values