From 71a859e05f8725cf74805b7a58e1095ddf417eb4 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 24 Feb 2026 10:08:33 -0800 Subject: [PATCH 1/3] separated check_name into org and schema functions --- synapseclient/models/schema_organization.py | 41 +++++++++++++++---- .../unit_test_schema_organization_async.py | 35 ++++++++++++---- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/synapseclient/models/schema_organization.py b/synapseclient/models/schema_organization.py index a6fc10566..b30ad839a 100644 --- a/synapseclient/models/schema_organization.py +++ b/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) @@ -1452,7 +1452,32 @@ def _check_name(name: str) -> None: if not re.match(r"^([A-Za-z])([A-Za-z]|\d|)*$", part): raise ValueError( ( - "Name may be separated by periods, " + "Organization name may be separated by periods, " + "but each part must start with a letter and contain " + f"only letters and numbers: {name}" + ) + ) + + +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 not re.match(r"^([A-Za-z])([A-Za-z]|\d|)*$", part): + raise ValueError( + ( + "Schema name may be separated by periods, " "but each part must start with a letter and contain " f"only letters and numbers: {name}" ) diff --git a/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py b/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py index 494e064ba..67833f240 100644 --- a/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py +++ b/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py @@ -4,13 +4,15 @@ import pytest +from build.lib.synapseclient.models.schema_organization import _check_name from synapseclient import Synapse from synapseclient.models.mixins.json_schema import JSONSchemaVersionInfo from synapseclient.models.schema_organization import ( CreateSchemaRequest, JSONSchema, SchemaOrganization, - _check_name, + _check_org_name, + _check_schema_name, ) ORG_NAME = "mytest.organization" @@ -97,42 +99,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: From ac2de8c35570471f24a3c77544222076cbab4793 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 24 Feb 2026 10:10:57 -0800 Subject: [PATCH 2/3] remove old import --- .../models/async/unit_test_schema_organization_async.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py b/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py index 67833f240..24f9cd7fe 100644 --- a/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py +++ b/tests/unit/synapseclient/models/async/unit_test_schema_organization_async.py @@ -4,7 +4,6 @@ import pytest -from build.lib.synapseclient.models.schema_organization import _check_name from synapseclient import Synapse from synapseclient.models.mixins.json_schema import JSONSchemaVersionInfo from synapseclient.models.schema_organization import ( From ae178b6140dcde04d7444148fc0ef934d7471110 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Wed, 25 Feb 2026 08:38:27 -0800 Subject: [PATCH 3/3] improved regex and error messages --- synapseclient/models/schema_organization.py | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/synapseclient/models/schema_organization.py b/synapseclient/models/schema_organization.py index b30ad839a..91922a4d5 100644 --- a/synapseclient/models/schema_organization.py +++ b/synapseclient/models/schema_organization.py @@ -1449,12 +1449,15 @@ def _check_org_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( ( - "Organization 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." ) ) @@ -1474,11 +1477,14 @@ def _check_schema_name(name: str) -> None: """ 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"Schema name sections must not be empty: {name}") + if not re.match(r"^([A-Za-z])([A-Za-z0-9])*$", part): raise ValueError( ( - "Schema 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 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." ) )