api: Fix encoding of strings in realm endpoint.

* Don't require strings to be unnecessarily JSON-encoded.
* Use check_capped_string rather than custom code for length checks.
* Update frontend to pass the right parameters.

With a much simplified populate_data_for_request design suggested by
Anders; we only support a handful of data types, all of which are
correctly encoded automatically by jQuery.

Fixes part of #18035.

frontend_tests/node_tests/settings_org.js

@@ -247,11 +247,11 @@ function test_submit_settings_form(override, submit_form) {
     assert(patched);
 
     let expected_value = {
-        bot_creation_policy: "1",
+        bot_creation_policy: 1,
-        invite_to_stream_policy: "1",
+        invite_to_stream_policy: 1,
-        email_address_visibility: "1",
+        email_address_visibility: 1,
         add_emoji_by_admins_only: false,
-        create_stream_policy: "2",
+        create_stream_policy: 2,
     };
     assert.deepEqual(data, expected_value);
 
@@ -281,7 +281,7 @@ function test_submit_settings_form(override, submit_form) {
     assert(patched);
 
     expected_value = {
-        default_language: '"en"',
+        default_language: "en",
         default_twenty_four_hour_time: "true",
     };
     assert.deepEqual(data, expected_value);

static/js/settings_org.js

@@ -795,7 +795,8 @@ export function build_page() {
             }
         } else if (subsection === "other_settings") {
             const code_block_language_value = default_code_language_widget.value();
-            data.default_code_block_language = JSON.stringify(code_block_language_value);
+            // No need to JSON-encode, since this value is already a string.
+            data.default_code_block_language = code_block_language_value;
         } else if (subsection === "other_permissions") {
             const add_emoji_permission = $("#id_realm_add_emoji_by_admins_only").val();
 
@@ -866,7 +867,7 @@ export function build_page() {
                 const input_value = get_input_element_value(input_elem);
                 if (input_value !== undefined) {
                     const property_name = input_elem.attr("id").replace("id_realm_", "");
-                    data[property_name] = JSON.stringify(input_value);
+                    data[property_name] = input_value;
                 }
             }
         }

templates/zerver/api/changelog.md

@@ -10,6 +10,12 @@ below features are supported.
 
 ## Changes in Zulip 4.0
 
+**Feature level 52**
+
+* `PATCH /realm`: Removed unnecessary JSON-encoding of string
+  parameters `name`, `description`, `default_language`, and
+  `default_code_block_language`.
+
 **Feature level 51**
 
 * [`POST /register`](/api/register-queue): Added a new boolean field

version.py

@@ -30,7 +30,7 @@ DESKTOP_WARNING_VERSION = "5.2.0"
 #
 # Changes should be accompanied by documentation explaining what the
 # new level means in templates/zerver/api/changelog.md.
-API_FEATURE_LEVEL = 51
+API_FEATURE_LEVEL = 52
 
 # Bump the minor PROVISION_VERSION to indicate that folks should provision
 # only when going from an old version of the code to a newer version. Bump

zerver/models.py

@@ -184,6 +184,7 @@ def clear_supported_auth_backends_cache() -> None:
 
 class Realm(models.Model):
     MAX_REALM_NAME_LENGTH = 40
+    MAX_REALM_DESCRIPTION_LENGTH = 1000
     MAX_REALM_SUBDOMAIN_LENGTH = 40
     MAX_REALM_REDIRECT_URL_LENGTH = 128
 

zerver/tests/test_realm.py

@@ -1,6 +1,6 @@
 import datetime
 import re
-from typing import Any, Dict, List, Mapping
+from typing import Any, Dict, List, Mapping, Union
 from unittest import mock
 
 import orjson
@@ -103,7 +103,7 @@ class RealmTest(ZulipTestCase):
     def test_update_realm_description(self) -> None:
         self.login("iago")
         new_description = "zulip dev group"
-        data = dict(description=orjson.dumps(new_description).decode())
+        data = dict(description=new_description)
         events: List[Mapping[str, Any]] = []
         with tornado_redirected_to_list(events):
             result = self.client_patch("/json/realm", data)
@@ -124,25 +124,25 @@ class RealmTest(ZulipTestCase):
 
     def test_realm_description_length(self) -> None:
         new_description = "A" * 1001
-        data = dict(description=orjson.dumps(new_description).decode())
+        data = dict(description=new_description)
 
         # create an admin user
         self.login("iago")
 
         result = self.client_patch("/json/realm", data)
-        self.assert_json_error(result, "Organization description is too long.")
+        self.assert_json_error(result, "description is too long (limit: 1000 characters)")
         realm = get_realm("zulip")
         self.assertNotEqual(realm.description, new_description)
 
     def test_realm_name_length(self) -> None:
         new_name = "A" * (Realm.MAX_REALM_NAME_LENGTH + 1)
-        data = dict(name=orjson.dumps(new_name).decode())
+        data = dict(name=new_name)
 
         # create an admin user
         self.login("iago")
 
         result = self.client_patch("/json/realm", data)
-        self.assert_json_error(result, "Organization name is too long.")
+        self.assert_json_error(result, "name is too long (limit: 40 characters)")
         realm = get_realm("zulip")
         self.assertNotEqual(realm.name, new_name)
 
@@ -151,7 +151,7 @@ class RealmTest(ZulipTestCase):
 
         self.login("othello")
 
-        req = dict(name=orjson.dumps(new_name).decode())
+        req = dict(name=new_name)
         result = self.client_patch("/json/realm", req)
         self.assert_json_error(result, "Must be an organization administrator")
 
@@ -419,7 +419,7 @@ class RealmTest(ZulipTestCase):
         # we need an admin user.
         self.login("iago")
 
-        req = dict(default_language=orjson.dumps(new_lang).decode())
+        req = dict(default_language=new_lang)
         result = self.client_patch("/json/realm", req)
         self.assert_json_success(result)
         realm = get_realm("zulip")
@@ -429,7 +429,7 @@ class RealmTest(ZulipTestCase):
         # as the default realm language, correct validation error is
         # raised and the invalid language is not saved in db
         invalid_lang = "invalid_lang"
-        req = dict(default_language=orjson.dumps(invalid_lang).decode())
+        req = dict(default_language=invalid_lang)
         result = self.client_patch("/json/realm", req)
         self.assert_json_error(result, f"Invalid language '{invalid_lang}'")
         realm = get_realm("zulip")
@@ -820,8 +820,10 @@ class RealmAPITest(ZulipTestCase):
         setattr(realm, attr, value)
         realm.save(update_fields=[attr])
 
-    def update_with_api(self, name: str, value: int) -> Realm:
+    def update_with_api(self, name: str, value: Union[int, str]) -> Realm:
-        result = self.client_patch("/json/realm", {name: orjson.dumps(value).decode()})
+        if not isinstance(value, str):
+            value = orjson.dumps(value).decode()
+        result = self.client_patch("/json/realm", {name: value})
         self.assert_json_success(result)
         return get_realm("zulip")  # refresh data
 

zerver/views/realm.py

@@ -26,10 +26,10 @@ from zerver.lib.retention import parse_message_retention_days
 from zerver.lib.streams import access_stream_by_id
 from zerver.lib.validator import (
     check_bool,
+    check_capped_string,
     check_dict,
     check_int,
     check_int_in,
-    check_string,
     check_string_or_int,
     to_non_negative_int,
 )
@@ -41,8 +41,12 @@ from zerver.models import Realm, UserProfile
 def update_realm(
     request: HttpRequest,
     user_profile: UserProfile,
-    name: Optional[str] = REQ(json_validator=check_string, default=None),
+    name: Optional[str] = REQ(
-    description: Optional[str] = REQ(json_validator=check_string, default=None),
+        str_validator=check_capped_string(Realm.MAX_REALM_NAME_LENGTH), default=None
+    ),
+    description: Optional[str] = REQ(
+        str_validator=check_capped_string(Realm.MAX_REALM_DESCRIPTION_LENGTH), default=None
+    ),
     emails_restricted_to_domains: Optional[bool] = REQ(json_validator=check_bool, default=None),
     disallow_disposable_email_addresses: Optional[bool] = REQ(
         json_validator=check_bool, default=None
@@ -68,7 +72,7 @@ def update_realm(
         converter=to_non_negative_int, default=None
     ),
     allow_edit_history: Optional[bool] = REQ(json_validator=check_bool, default=None),
-    default_language: Optional[str] = REQ(json_validator=check_string, default=None),
+    default_language: Optional[str] = REQ(default=None),
     waiting_period_threshold: Optional[int] = REQ(converter=to_non_negative_int, default=None),
     authentication_methods: Optional[Dict[str, Any]] = REQ(
         json_validator=check_dict([]), default=None
@@ -106,7 +110,7 @@ def update_realm(
     ),
     default_twenty_four_hour_time: Optional[bool] = REQ(json_validator=check_bool, default=None),
     video_chat_provider: Optional[int] = REQ(json_validator=check_int, default=None),
-    default_code_block_language: Optional[str] = REQ(json_validator=check_string, default=None),
+    default_code_block_language: Optional[str] = REQ(default=None),
     digest_weekday: Optional[int] = REQ(
         json_validator=check_int_in(Realm.DIGEST_WEEKDAY_VALUES), default=None
     ),
@@ -117,10 +121,6 @@ def update_realm(
     # the entire request can succeed or fail atomically.
     if default_language is not None and default_language not in get_available_language_codes():
         raise JsonableError(_("Invalid language '{}'").format(default_language))
-    if description is not None and len(description) > 1000:
-        return json_error(_("Organization description is too long."))
-    if name is not None and len(name) > Realm.MAX_REALM_NAME_LENGTH:
-        return json_error(_("Organization name is too long."))
     if authentication_methods is not None:
         if not user_profile.is_realm_owner:
             raise OrganizationOwnerRequired()