Merge pull request #1328 from Sage-Bionetworks/fix_check_name

Separated check_name into org and schema functions

Author: andrewelamb

synapseclient/models/schema_organization.py

@@ -254,7 +254,7 @@ class SchemaOrganization(SchemaOrganizationProtocol):
 
     def __post_init__(self) -> None:
         if self.name:
-            _check_name(self.name)
+            _check_org_name(self.name)
 
     async def get_async(
         self, *, synapse_client: Optional["Synapse"] = None
@@ -826,9 +826,9 @@ class JSONSchema(JSONSchemaProtocol):
 
     def __post_init__(self) -> None:
         if self.name:
-            _check_name(self.name)
+            _check_schema_name(self.name)
         if self.organization_name:
-            _check_name(self.organization_name)
+            _check_org_name(self.organization_name)
         if self.name and self.organization_name:
             self.uri = f"{self.organization_name}-{self.name}"
         else:
@@ -1313,8 +1313,8 @@ class CreateSchemaRequest(AsynchronousCommunicator):
 
     def __post_init__(self) -> None:
         self.concrete_type = CREATE_SCHEMA_REQUEST
-        _check_name(self.name)
-        _check_name(self.organization_name)
+        _check_schema_name(self.name)
+        _check_org_name(self.organization_name)
         uri = f"{self.organization_name}-{self.name}"
         if self.version:
             self._check_semantic_version(self.version)
@@ -1428,9 +1428,9 @@ def list_json_schema_organizations(
     return all_orgs
 
 
-def _check_name(name: str) -> None:
+def _check_org_name(name: str) -> None:
     """
-    Checks that the input name is a valid Synapse Organization or JSONSchema name
+    Checks that the input name is a valid Synapse Organization
     - Length requirement of 6 ≤ x ≤ 250
     - Names do not contain the string sagebionetworks (case insensitive)
     - May contain periods (each part is separated by periods)
@@ -1449,11 +1449,42 @@ def _check_name(name: str) -> None:
         raise ValueError(f"The name must not contain 'sagebionetworks' : {name}")
     parts = name.split(".")
     for part in parts:
-        if not re.match(r"^([A-Za-z])([A-Za-z]|\d|)*$", part):
+        if len(part) == 0:
+            raise ValueError(f"Organization name sections must not be empty: {name}")
+        if not re.match(r"^([A-Za-z])([A-Za-z0-9])*$", part):
             raise ValueError(
                 (
-                    "Name may be separated by periods, "
-                    "but each part must start with a letter and contain "
-                    f"only letters and numbers: {name}"
+                    f"Un-allowed characters in organization name: {name}"
+                    "Organization name may separated into sections by periods. "
+                    "For example 'my.organization.name'. "
+                    "Each section must start with a letter and contain only letters and numbers."
+                )
+            )
+
+
+def _check_schema_name(name: str) -> None:
+    """
+    Checks that the input name is a valid Synapse JSONSchema name
+    - May contain periods (each part is separated by periods)
+    - Each part must start with a letter
+    - Each part contains only letters and numbers
+
+    Arguments:
+        name: The name of the schema to be checked
+
+    Raises:
+        ValueError: When the name isn't valid
+    """
+    parts = name.split(".")
+    for part in parts:
+        if len(part) == 0:
+            raise ValueError(f"Schema name sections must not be empty: {name}")
+        if not re.match(r"^([A-Za-z])([A-Za-z0-9])*$", part):
+            raise ValueError(
+                (
+                    f"Un-allowed characters in schema name: {name}"
+                    "Schema name may separated into sections by periods. "
+                    "For example 'my.schema.name'. "
+                    "Each section must start with a letter and contain only letters and numbers."
                 )
             )

tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py

@@ -10,7 +10,8 @@
     CreateSchemaRequest,
     JSONSchema,
     SchemaOrganization,
-    _check_name,
+    _check_org_name,
+    _check_schema_name,
 )
 
 ORG_NAME = "mytest.organization"
@@ -97,42 +98,59 @@ def _get_acl_response():
     }
 
 
-class TestCheckName:
-    """Tests for the _check_name validation function."""
+class TestCheckOrgName:
+    """Tests for the _check_org_name validation function."""
 
     def test_valid_name(self) -> None:
         # GIVEN a valid name
         # WHEN I check the name
         # THEN no exception should be raised
-        _check_name("mytest.organization")
+        _check_org_name("mytest.organization")
 
     def test_name_too_short(self) -> None:
         # GIVEN a name that is too short
         # WHEN I check the name
         # THEN it should raise ValueError
         with pytest.raises(ValueError, match="length 6 to 250"):
-            _check_name("abc")
+            _check_org_name("abc")
 
     def test_name_too_long(self) -> None:
         # GIVEN a name that is too long
         # WHEN I check the name
         # THEN it should raise ValueError
         with pytest.raises(ValueError, match="length 6 to 250"):
-            _check_name("a" * 251)
+            _check_org_name("a" * 251)
 
     def test_name_contains_sagebionetworks(self) -> None:
         # GIVEN a name containing 'sagebionetworks'
         # WHEN I check the name
         # THEN it should raise ValueError
         with pytest.raises(ValueError, match="sagebionetworks"):
-            _check_name("my.sagebionetworks.test")
+            _check_org_name("my.sagebionetworks.test")
 
     def test_name_part_starts_with_number(self) -> None:
         # GIVEN a name where a part starts with a number
         # WHEN I check the name
         # THEN it should raise ValueError
         with pytest.raises(ValueError, match="must start with a letter"):
-            _check_name("mytest.1invalid")
+            _check_org_name("mytest.1invalid")
+
+
+class TestCheckSchemaName:
+    """Tests for the _check_schema_name validation function."""
+
+    def test_valid_name(self) -> None:
+        # GIVEN a valid name
+        # WHEN I check the name
+        # THEN no exception should be raised
+        _check_schema_name("mytest.schema")
+
+    def test_name_part_starts_with_number(self) -> None:
+        # GIVEN a name where a part starts with a number
+        # WHEN I check the name
+        # THEN it should raise ValueError
+        with pytest.raises(ValueError, match="must start with a letter"):
+            _check_schema_name("mytest.1invalid")
 
 
 class TestSchemaOrganization: