Merge branch 'develop' into update-black

Author: Lingling Peng

synapseclient/extensions/curator/file_based_metadata_task.py

@@ -24,7 +24,7 @@
 from synapseclient.operations import FileOptions, get
 
 TYPE_DICT = {
-    "string": ColumnType.STRING,
+    "string": ColumnType.MEDIUMTEXT,
     "number": ColumnType.DOUBLE,
     "integer": ColumnType.INTEGER,
     "boolean": ColumnType.BOOLEAN,
@@ -199,48 +199,67 @@ def _create_columns_from_json_schema(json_schema: dict[str, Any]) -> list[Column
         raise ValueError(
             "The 'properties' field in the JSON Schema must be a dictionary."
         )
-    columns = []
-    for name, prop_schema in properties.items():
-        column_type = _get_column_type_from_js_property(prop_schema)
-        maximum_size = None
-        if column_type == "STRING":
-            maximum_size = 100
-        if column_type in LIST_TYPE_DICT.values():
-            maximum_size = 5
-
-        column = Column(
-            name=name,
-            column_type=column_type,
-            maximum_size=maximum_size,
-            default_value=None,
-        )
-        columns.append(column)
+    columns = [
+        _create_synapse_column_from_js_property(prop_schema, name)
+        for name, prop_schema in properties.items()
+    ]
     return columns
 
 
+def _create_synapse_column_from_js_property(
+    js_property: dict[str, Any], name: str
+) -> Column:
+    """
+    Creates a Synapse Column based on a JSON Schema property.
+
+    Args:
+        js_property: A JSON Schema property in dict form.
+        name: The name of the column.
+
+    Returns:
+        A Synapse Column based on the JSON Schema property.
+    """
+    column_type = _get_column_type_from_js_property(js_property)
+    return Column(name=name, column_type=column_type)
+
+
 def _get_column_type_from_js_property(js_property: dict[str, Any]) -> ColumnType:
     """
     Gets the Synapse column type from a JSON Schema property.
     The JSON Schema should be valid but that should not be assumed.
-    If the type can not be determined ColumnType.STRING will be returned.
+    If the type can not be determined ColumnType.MEDIUMTEXT will be returned.
 
     Args:
         js_property: A JSON Schema property in dict form.
 
     Returns:
         A Synapse ColumnType based on the JSON Schema type
     """
-    # Enums are always strings in Synapse tables
+    # Enums are set as MediumText columns
     if "enum" in js_property:
-        return ColumnType.STRING
+        return ColumnType.MEDIUMTEXT
     if "type" in js_property:
-        if js_property["type"] == "array":
+        js_type = js_property["type"]
+        # Synapse columns cannot be more than one type
+        # If the JSONSchema type is a list of types, check if it's a nullable single type
+        if isinstance(js_type, list):
+            types = [t for t in js_type if t != "null"]
+            if len(types) == 1:
+                js_type = types[0]
+            # If there are multiple non-null types, we cannot determine a single column type, so default to MediumText
+            else:
+                return ColumnType.MEDIUMTEXT
+        if js_type == "array":
             return _get_list_column_type_from_js_property(js_property)
-        return TYPE_DICT.get(js_property["type"], ColumnType.STRING)
+        # If there is only one JSONSChema type, return the corresponding Synapse column type,
+        #  defaulting to MediumText if there is no match
+        return TYPE_DICT.get(js_type, ColumnType.MEDIUMTEXT)
     # A oneOf list usually indicates that the type could be one or more different things
+    # Curator extension does not create the types of JSON Schemas where this is the case
+    #  but if it is present we will attempt to determine the type based on the items in the oneOf list.
     if "oneOf" in js_property and isinstance(js_property["oneOf"], list):
         return _get_column_type_from_js_one_of_list(js_property["oneOf"])
-    return ColumnType.STRING
+    return ColumnType.MEDIUMTEXT
 
 
 def _get_column_type_from_js_one_of_list(js_one_of_list: list[Any]) -> ColumnType:
@@ -258,15 +277,15 @@ def _get_column_type_from_js_one_of_list(js_one_of_list: list[Any]) -> ColumnTyp
     items = [item for item in js_one_of_list if isinstance(item, dict)]
     # Enums are always strings in Synapse tables
     if [item for item in items if "enum" in item]:
-        return ColumnType.STRING
+        return ColumnType.MEDIUMTEXT
     # For Synapse ColumnType we can ignore null types in JSON Schemas
     type_items = [item for item in items if "type" in item if item["type"] != "null"]
     if len(type_items) == 1:
         type_item = type_items[0]
         if type_item["type"] == "array":
             return _get_list_column_type_from_js_property(type_item)
-        return TYPE_DICT.get(type_item["type"], ColumnType.STRING)
-    return ColumnType.STRING
+        return TYPE_DICT.get(type_item["type"], ColumnType.MEDIUMTEXT)
+    return ColumnType.MEDIUMTEXT
 
 
 def _get_list_column_type_from_js_property(js_property: dict[str, Any]) -> ColumnType:

tests/unit/synapseclient/extensions/unit_test_curator.py

@@ -12,6 +12,7 @@
 import shutil
 import tempfile
 import unittest
+from typing import Any
 from unittest.mock import Mock, mock_open, patch
 
 import pandas as pd
@@ -26,6 +27,7 @@
 )
 from synapseclient.extensions.curator.file_based_metadata_task import (
     _create_columns_from_json_schema,
+    _create_synapse_column_from_js_property,
     _get_column_type_from_js_one_of_list,
     _get_column_type_from_js_property,
     _get_list_column_type_from_js_property,
@@ -50,7 +52,7 @@
     SchemaRegistryColumnConfig,
     get_latest_schema_uri,
 )
-from synapseclient.models import ColumnType
+from synapseclient.models import Column, ColumnType
 from synapseclient.models.curation import (
     FileBasedMetadataTaskProperties,
     RecordBasedMetadataTaskProperties,
@@ -1670,50 +1672,6 @@ def test_create_columns_from_json_schema_success(self):
         assert all(hasattr(col, "name") for col in columns)
         assert all(hasattr(col, "column_type") for col in columns)
 
-    def test_get_column_type_from_js_property_enum(self):
-        """Test getting column type for enum property."""
-        # GIVEN a JSON schema property with an enum
-        js_property = {"enum": ["option1", "option2", "option3"]}
-
-        # WHEN I get the column type
-        result = _get_column_type_from_js_property(js_property)
-
-        # THEN it should return STRING type
-        assert result == ColumnType.STRING
-
-    def test_get_column_type_from_js_property_array(self):
-        """Test getting column type for array property."""
-        # GIVEN a JSON schema property with array type
-        js_property = {"type": "array", "items": {"type": "string"}}
-
-        # WHEN I get the column type
-        result = _get_column_type_from_js_property(js_property)
-
-        # THEN it should return a list type
-        assert result == ColumnType.STRING_LIST
-
-    def test_get_column_type_from_js_property_one_of(self):
-        """Test getting column type for oneOf property."""
-        # GIVEN a JSON schema property with oneOf
-        js_property = {"oneOf": [{"type": "string"}, {"type": "null"}]}
-
-        # WHEN I get the column type
-        result = _get_column_type_from_js_property(js_property)
-
-        # THEN it should return STRING type
-        assert result == ColumnType.STRING
-
-    def test_get_column_type_from_js_property_fallback(self):
-        """Test getting column type fallback to STRING."""
-        # GIVEN a JSON schema property without recognizable type
-        js_property = {"description": "some property"}
-
-        # WHEN I get the column type
-        result = _get_column_type_from_js_property(js_property)
-
-        # THEN it should return STRING type as fallback
-        assert result == ColumnType.STRING
-
     def test_get_column_type_from_js_one_of_list_with_enum(self):
         """Test getting column type from oneOf list containing enum."""
         # GIVEN a oneOf list with an enum
@@ -1722,8 +1680,8 @@ def test_get_column_type_from_js_one_of_list_with_enum(self):
         # WHEN I get the column type
         result = _get_column_type_from_js_one_of_list(js_one_of_list)
 
-        # THEN it should return STRING type
-        assert result == ColumnType.STRING
+        # THEN it should return MEDIUMTEXT type
+        assert result == ColumnType.MEDIUMTEXT
 
     def test_get_column_type_from_js_one_of_list_single_type(self):
         """Test getting column type from oneOf list with single non-null type."""
@@ -1758,8 +1716,8 @@ def test_get_column_type_from_js_one_of_list_fallback(self):
         # WHEN I get the column type
         result = _get_column_type_from_js_one_of_list(js_one_of_list)
 
-        # THEN it should return STRING type as fallback
-        assert result == ColumnType.STRING
+        # THEN it should return MEDIUMTEXT type as fallback
+        assert result == ColumnType.MEDIUMTEXT
 
     def test_get_list_column_type_from_js_property_with_enum(self):
         """Test getting list column type for property with enum items."""
@@ -1795,6 +1753,157 @@ def test_get_list_column_type_from_js_property_fallback(self):
         assert result == ColumnType.STRING_LIST
 
 
+@pytest.mark.parametrize(
+    "json_schema, expected_columns",
+    [
+        (
+            {
+                "properties": {
+                    "string_col": {"type": "string"},
+                }
+            },
+            [
+                Column(name="string_col", column_type=ColumnType.MEDIUMTEXT),
+            ],
+        ),
+        (
+            {
+                "properties": {
+                    "string_col": {"type": "string"},
+                    "int_col": {"type": "integer"},
+                    "bool_col": {"type": "boolean"},
+                    "number_col": {"type": "number"},
+                }
+            },
+            [
+                Column(name="string_col", column_type=ColumnType.MEDIUMTEXT),
+                Column(name="int_col", column_type=ColumnType.INTEGER),
+                Column(name="bool_col", column_type=ColumnType.BOOLEAN),
+                Column(name="number_col", column_type=ColumnType.DOUBLE),
+            ],
+        ),
+    ],
+    ids=["one column", "three columns"],
+)
+def test_create_columns_from_json_schema(
+    json_schema: dict[str, Any], expected_columns: list[Column]
+):
+    """Test successful column creation from JSON schema."""
+    assert _create_columns_from_json_schema(json_schema) == expected_columns
+
+
+@pytest.mark.parametrize(
+    "json_schema",
+    [{}, {"properties": []}],
+    ids=["empty schema", "properties is not a dict"],
+)
+def test_create_columns_from_json_schema_exceptions(json_schema: dict[str, Any]):
+    """Test exceptions when creating columns from invalid JSON schema."""
+    with pytest.raises(ValueError):
+        _create_columns_from_json_schema(json_schema)
+
+
+@pytest.mark.parametrize(
+    "json_schema_property, property_name, expected_column_type",
+    [
+        (
+            {"type": "array", "items": {"type": "string"}},
+            "string_list_col",
+            ColumnType.STRING_LIST,
+        ),
+        (
+            {"type": "array", "items": {"type": "integer"}},
+            "int_list_col",
+            ColumnType.INTEGER_LIST,
+        ),
+        (
+            {"type": "array", "items": {"type": "boolean"}},
+            "bool_list_col",
+            ColumnType.BOOLEAN_LIST,
+        ),
+        (
+            {"type": "number"},
+            "number_col",
+            ColumnType.DOUBLE,
+        ),
+        (
+            {"type": "integer"},
+            "integer_col",
+            ColumnType.INTEGER,
+        ),
+        (
+            {"type": "boolean"},
+            "boolean_col",
+            ColumnType.BOOLEAN,
+        ),
+        (
+            {"type": "string"},
+            "string_col",
+            ColumnType.MEDIUMTEXT,
+        ),
+    ],
+    ids=[
+        "string_list",
+        "integer_list",
+        "boolean_list",
+        "number",
+        "integer",
+        "boolean",
+        "string",
+    ],
+)
+def test_create_synapse_column_from_js_property(
+    json_schema_property: dict[str, Any],
+    property_name: str,
+    expected_column_type: ColumnType,
+):
+    """Test successful column creation from JSON schema property."""
+    result = _create_synapse_column_from_js_property(
+        json_schema_property, property_name
+    )
+    assert isinstance(result, Column)
+    assert result.name == property_name
+    assert result.column_type == expected_column_type
+
+
+@pytest.mark.parametrize(
+    "json_schema_property, expected_column_type",
+    [
+        ({"enum": ["a", "b", "c"]}, ColumnType.MEDIUMTEXT),
+        ({"type": "string"}, ColumnType.MEDIUMTEXT),
+        ({"type": "integer"}, ColumnType.INTEGER),
+        ({"type": "number"}, ColumnType.DOUBLE),
+        ({"type": "boolean"}, ColumnType.BOOLEAN),
+        ({"type": ["integer", "null"]}, ColumnType.INTEGER),
+        ({"type": ["integer", "string"]}, ColumnType.MEDIUMTEXT),
+        ({"type": "array", "items": {"type": "integer"}}, ColumnType.INTEGER_LIST),
+        ({"oneOf": [{"type": "integer"}, {"type": "null"}]}, ColumnType.INTEGER),
+        ({"type": "unknown"}, ColumnType.MEDIUMTEXT),
+        ({}, ColumnType.MEDIUMTEXT),
+    ],
+    ids=[
+        "enum_property",
+        "type_string",
+        "type_integer",
+        "type_number",
+        "type_boolean",
+        "type_list_nullable",
+        "type_list_multiple_types",
+        "type_array",
+        "one_of_list",
+        "unknown_type",
+        "empty_property",
+    ],
+)
+def test_get_column_type_from_js_property(
+    json_schema_property: dict[str, Any], expected_column_type: ColumnType
+):
+    """Test getting column type from JSON schema property."""
+    assert (
+        _get_column_type_from_js_property(json_schema_property) == expected_column_type
+    )
+
+
 class TestGetLatestSchemaUri(unittest.TestCase):
     """Test cases for get_latest_schema_uri function."""