diff --git a/docs/guides/extensions/curator/metadata_curation.md b/docs/guides/extensions/curator/metadata_curation.md index e0d4d8ca3..fec0915cb 100644 --- a/docs/guides/extensions/curator/metadata_curation.md +++ b/docs/guides/extensions/curator/metadata_curation.md @@ -132,6 +132,55 @@ print(f"Created CurationTask: {task.task_id}") - Automatic schema binding to the folder for validation - Optional wiki attached to the folder +### Controlling the order of the columns + +Both [create_record_based_metadata_task][synapseclient.extensions.curator.create_record_based_metadata_task] and [create_file_based_metadata_task][synapseclient.extensions.curator.create_file_based_metadata_task] accept an optional `column_order` that controls the left-to-right order of the columns contributors see in the grid. JSON Schema property order is not reliably preserved by downstream applications, so pass the order explicitly here instead of relying on the schema. + +You only need to name the columns that need intentional placement. Every column you leave out stays visible and is appended after the ones you named, in its existing relative order. This keeps the configuration short as properties are added to the schema over time. + +A few columns are always pinned to the front and cannot be moved: + +- Record-based tasks pin the `upsert_keys`, because they identify each row. +- File-based tasks pin `name` and `id`. + +Naming a pinned column in `column_order` has no effect — it stays in its pinned position and is not duplicated. A name that does not match any available column raises a `ValueError`. + +```python +record_set, curation_task = create_record_based_metadata_task( + synapse_client=syn, + folder_id="syn987654321", + record_set_name="AnimalMetadata_Records", + record_set_description="Centralized metadata for animal study data", + curation_task_name="AnimalMetadata_Curation", + upsert_keys=["StudyKey"], + column_order=["diagnosis", "specimenType", "assay"], + instructions="Complete all required fields according to the schema.", + schema_uri=schema_uri, + create_grid=False, +) + +# Resulting column order: +# StudyKey, diagnosis, specimenType, assay, +``` + +```python +entity_view, task = create_file_based_metadata_task( + synapse_client=syn, + folder_id="syn987654321", + curation_task_name="FileMetadata_Curation", + instructions="Annotate each file with metadata according to the schema requirements.", + entity_view_name="Animal Study Files View", + schema_uri=schema_uri, + column_order=["patientId", "sampleId", "assay", "fileFormat"], + return_entities=True, +) + +# Resulting column order: +# name, id, patientId, sampleId, assay, fileFormat, +``` + +For file-based tasks the available columns include the Synapse managed columns such as `createdBy` and `modifiedOn` in addition to the JSON Schema properties, so you may place those wherever you like. They are no longer pinned to the front — only `name` and `id` are — which lets contributor-relevant metadata appear before system metadata. + ### Controlling who can access the grid session Both [create_record_based_metadata_task][synapseclient.extensions.curator.create_record_based_metadata_task] and [create_file_based_metadata_task][synapseclient.extensions.curator.create_file_based_metadata_task] accept an optional `authorization_mode` that tells clients how to scope access when a grid session is created for the task: diff --git a/synapseclient/extensions/curator/file_based_metadata_task.py b/synapseclient/extensions/curator/file_based_metadata_task.py index ee261ac44..ca64c47e2 100644 --- a/synapseclient/extensions/curator/file_based_metadata_task.py +++ b/synapseclient/extensions/curator/file_based_metadata_task.py @@ -5,12 +5,17 @@ in Synapse, including EntityView creation, CurationTask setup, and Wiki attachment. """ +from collections import OrderedDict from typing import Any, Optional, Tuple, Union from synapseclient import Synapse # type: ignore from synapseclient import Wiki # type: ignore from synapseclient.core.exceptions import SynapseHTTPError # type: ignore -from synapseclient.extensions.curator.utils import project_id_from_entity_id +from synapseclient.extensions.curator.utils import ( + project_id_from_entity_id, + resolve_column_order_list, + validate_column_order_list, +) from synapseclient.models import ( # type: ignore AuthorizationMode, Column, @@ -42,6 +47,7 @@ def _create_json_schema_entity_view( synapse_entity_id: str, entity_view_name: str = "JSON Schema view", view_type_mask: Union[int, ViewTypeMask] = ViewTypeMask.FILE, + column_order: list[str] | None = None, syn: Optional[Synapse] = None, ) -> EntityView: """ @@ -55,17 +61,29 @@ def _create_json_schema_entity_view( ViewTypeMask.FILE. Additional types can be added using bitwise OR (e.g., ViewTypeMask.FILE | ViewTypeMask.DOCKER). Accepts either a ViewTypeMask enum member or its raw integer value. + column_order: Optional list of column names to place immediately after the + pinned name and id columns, in the order given. Remaining columns keep + their existing relative order. syn: A Synapse object thats been logged in Returns: The created EntityView object + + Raises: + ValueError: If synapse_entity_id is not a Folder or a Project, or if + column_order is malformed or names a column that is not present on the + created EntityView. """ entity = get( file_options=FileOptions(download_file=False), synapse_id=synapse_entity_id, synapse_client=syn, ) - assert isinstance(entity, (Folder, Project)) + if not isinstance(entity, (Folder, Project)): + raise ValueError( + f"A JSON Schema can only be read from a Folder or a Project, but " + f"{synapse_entity_id} is a {type(entity).__name__}." + ) jsb = entity.get_schema(synapse_client=syn) version_info = jsb.json_schema_version_info schema = JSONSchema(version_info.schema_name, version_info.organization_name) @@ -78,10 +96,27 @@ def _create_json_schema_entity_view( view_type_mask=view_type_mask, columns=columns, ).store(synapse_client=syn) - # This reorder is so that these show up in the front of the EntityView in Synapse. - view.reorder_column(name="name", index=0) - view.reorder_column(name="id", index=1) - view.reorder_column(name="createdBy", index=2) + + try: + available_columns = list(view.columns.keys()) + ordered_columns = resolve_column_order_list( + available_columns=available_columns, + pinned_columns=["name", "id"], + requested_columns=column_order, + ) + view.columns = OrderedDict( + (column, view.columns[column]) for column in ordered_columns + ) + except ValueError: + try: + view.delete(synapse_client=syn) + except Exception: + Synapse.get_client(synapse_client=syn).logger.exception( + f"Could not delete the created EntityView {view.id}. Delete it " + "yourself, either from the Synapse web UI, or with the Python " + f"client: EntityView(id='{view.id}').delete()" + ) + raise view.store(synapse_client=syn) return view @@ -332,6 +367,7 @@ def create_file_based_metadata_task( # parameter and change the return type to Tuple[EntityView, CurationTask]. return_entities: bool = False, *, + column_order: list[str] | None = None, synapse_client: Optional[Synapse] = None, ) -> Union[Tuple[str, int], Tuple[EntityView, CurationTask]]: """ @@ -385,6 +421,32 @@ def create_file_based_metadata_task( ) ``` + Example: Controlling the column order of the EntityView + Pass column_order to place specific columns immediately after the pinned name + and id columns. You only need to name the columns you care about; every other + column, including Synapse managed columns such as createdBy, is appended + afterwards in its existing order. + + ```python + import synapseclient + from synapseclient.extensions.curator import create_file_based_metadata_task + + syn = synapseclient.Synapse() + syn.login() + + entity_view, curation_task = create_file_based_metadata_task( + synapse_client=syn, + folder_id="syn12345678", + curation_task_name="BiospecimenMetadataTemplate", + instructions="Please curate this metadata according to the schema requirements", + column_order=["patientId", "sampleId", "assay", "fileFormat"], + return_entities=True, + ) + + # Resulting column order: + # name, id, patientId, sampleId, assay, fileFormat, + ``` + Arguments: folder_id: The Synapse Folder ID to create the file view for. curation_task_name: Name for the CurationTask (used as data_type field). @@ -423,6 +485,14 @@ def create_file_based_metadata_task( objects instead of their ID strings. Defaults to False for backwards compatibility. The entity-returning shape will become the default in v5.0.0, at which point this parameter will be removed. + column_order: Optional list of column names placed immediately after the + pinned name and id columns, in the order given. Columns that are not + named keep their existing relative order and are appended afterwards, so + you only need to list the columns that need intentional placement. The + name and id columns always remain the two leftmost columns, so naming + either of them here has no effect. Every name must match a column on the + created EntityView, which includes the JSON Schema properties as well as + the Synapse managed columns such as createdBy and modifiedOn. synapse_client: If not passed in and caching was not disabled by `Synapse.allow_client_caching(False)` this will use the last created instance from the Synapse class constructor. @@ -437,7 +507,8 @@ def create_file_based_metadata_task( - The created CurationTask object Raises: - ValueError: If required parameters are missing. + ValueError: If required parameters are missing, or if column_order is + malformed or names a column that is not on the created EntityView. SynapseError: If there are issues with Synapse operations. """ # Validate required parameters @@ -447,6 +518,7 @@ def create_file_based_metadata_task( raise ValueError("curation_task_name is required") if not instructions: raise ValueError("instructions is required") + validate_column_order_list(column_order) synapse_client = Synapse.get_client(synapse_client=synapse_client) @@ -478,6 +550,7 @@ def create_file_based_metadata_task( synapse_entity_id=folder_id, entity_view_name=entity_view_name, view_type_mask=view_type_mask, + column_order=column_order, ) entity_view_id = entity_view.id except Exception as e: diff --git a/synapseclient/extensions/curator/record_based_metadata_task.py b/synapseclient/extensions/curator/record_based_metadata_task.py index b85689409..486c65528 100644 --- a/synapseclient/extensions/curator/record_based_metadata_task.py +++ b/synapseclient/extensions/curator/record_based_metadata_task.py @@ -12,7 +12,11 @@ from synapseclient import Synapse from synapseclient.core.typing_utils import DataFrame as DATA_FRAME_TYPE from synapseclient.core.utils import test_import_pandas -from synapseclient.extensions.curator.utils import project_id_from_entity_id +from synapseclient.extensions.curator.utils import ( + project_id_from_entity_id, + resolve_column_order_list, + validate_column_order_list, +) from synapseclient.models import ( AuthorizationMode, CurationTask, @@ -78,33 +82,6 @@ def extract_schema_properties_from_dict(schema_data: Dict[str, Any]) -> DATA_FRA return df -def _reorder_columns_with_upsert_keys_first( - df: DATA_FRAME_TYPE, upsert_keys: list[str] -) -> DATA_FRAME_TYPE: - """ - Reorder a DataFrame's columns so the upsert key columns appear first. - - The upsert keys serve as the row identifiers in the Grid curation UI, so they - should always be the leftmost columns of the CSV template. The relative order of - the upsert keys is preserved as given, followed by the remaining columns in their - original order. Callers are expected to validate that every upsert key is present - among the columns before calling this function. - - Args: - df: DataFrame whose columns should be reordered. - upsert_keys: List of column names to move to the front, in the desired order. - - Returns: - DataFrame with the upsert key columns moved to the front. - """ - upsert_key_set = set(upsert_keys) - remaining_columns = [ - column for column in df.columns.tolist() if column not in upsert_key_set - ] - - return df[upsert_keys + remaining_columns] - - def extract_schema_properties_from_web( syn: Synapse, schema_uri: str ) -> DATA_FRAME_TYPE: @@ -141,6 +118,7 @@ def create_record_based_metadata_task( assignee_principal_id: Optional[Union[str, int]] = None, authorization_mode: Optional[Union[AuthorizationMode, str]] = None, *, + column_order: list[str] | None = None, synapse_client: Optional[Synapse] = None, project_id: Optional[str] = None, # Deprecated, will be removed in v5.0.0 create_grid: bool = True, # Deprecated, will be removed in v5.0.0 @@ -191,6 +169,36 @@ def create_record_based_metadata_task( ) ``` + Example: Controlling the column order of the RecordSet + Pass column_order to place specific columns immediately after the upsert keys. + You only need to name the columns you care about; every other schema property + is appended afterwards in its existing order. Upsert keys always stay leftmost, + so naming one in column_order does not duplicate or move it. + + ```python + import synapseclient + from synapseclient.extensions.curator import create_record_based_metadata_task + + syn = synapseclient.Synapse() + syn.login() + + record_set, curation_task = create_record_based_metadata_task( + synapse_client=syn, + folder_id="syn87654321", + record_set_name="BiospecimenMetadata_RecordSet", + record_set_description="RecordSet for biospecimen metadata curation", + curation_task_name="BiospecimenMetadataTemplate", + upsert_keys=["patientId", "specimenID"], + column_order=["diagnosis", "assay"], + instructions="Please curate this metadata according to the schema requirements", + schema_uri="schema-org-schema.name.schema-v1.0.0", + create_grid=False, + ) + + # Resulting column order: + # patientId, specimenID, diagnosis, assay, + ``` + Arguments: folder_id: The Synapse ID of the folder to upload RecordSet to. record_set_name: Name for the RecordSet entity that will be created. @@ -225,6 +233,12 @@ def create_record_based_metadata_task( for the current user. Changing this value after the task already exists resets the task's active session, so a new grid session must be opened before curation can continue. + column_order: Optional list of column names placed immediately after the + upsert keys, in the order given. Columns that are not named keep their + existing relative order and are appended afterwards, so you only need to + list the columns that need intentional placement. Naming an upsert key + here has no effect, it stays in its leading position. Every name must + match a property defined by the schema. synapse_client: If not passed in and caching was not disabled by `Synapse.allow_client_caching(False)` this will use the last created instance from the Synapse class constructor. @@ -239,7 +253,8 @@ def create_record_based_metadata_task( Raises: ValueError: If required parameters are missing, if schema_uri is not provided, - or if any upsert_keys are not found among the schema properties. + if any upsert_keys are not found among the schema properties, or if + column_order is malformed or names a column that is not a schema property. SynapseError: If there are issues with Synapse operations. """ # Validate required parameters @@ -257,6 +272,7 @@ def create_record_based_metadata_task( raise ValueError("instructions is required") if not schema_uri: raise ValueError("schema_uri is required") + validate_column_order_list(column_order) if project_id: synapse_client.logger.warning( @@ -289,9 +305,14 @@ def create_record_based_metadata_task( f"{missing_upsert_keys}. Upsert keys identify each row and must correspond " "to columns defined in the schema." ) - template_df = _reorder_columns_with_upsert_keys_first( - df=template_df, upsert_keys=upsert_keys - ) + + template_df = template_df[ + resolve_column_order_list( + available_columns=template_df.columns.tolist(), + pinned_columns=upsert_keys, + requested_columns=column_order, + ) + ] synapse_client.logger.info( f"Extracted schema properties and created template: {template_df.columns.tolist()}" diff --git a/synapseclient/extensions/curator/utils.py b/synapseclient/extensions/curator/utils.py index b63f9bfac..51928bacd 100644 --- a/synapseclient/extensions/curator/utils.py +++ b/synapseclient/extensions/curator/utils.py @@ -30,3 +30,112 @@ def project_id_from_entity_id(entity_id: str, synapse_client: Synapse) -> str: if iterations > MAX_HIERARCHY_DEPTH: raise ValueError("Could not find project ID in folder hierarchy") return current_obj.id + + +def validate_column_order_list(column_order_list: list[str] | None) -> list[str]: + """ + Validate the shape of a caller supplied column order value. + + This checks everything that can be checked without knowing which columns are + actually available: that the value is a list, that every entry is a non-empty + string, and that no entry is repeated. Use this to fail fast before any entities + are created in Synapse. The check that every requested column actually exists is + performed later by resolve_column_order_list. + + Arguments: + column_order_list: The caller supplied column order, or None. + + Returns: + The column order as a list. An empty list is returned when + column_order_list is None. + + Raises: + ValueError: If column_order_list is not a list, contains a non-string or + empty value, or contains duplicate values. + """ + if column_order_list is None: + return [] + + if not isinstance(column_order_list, list): + raise ValueError( + "column_order must be a list of column names, but received " + f"{type(column_order_list).__name__}." + ) + + invalid_values = [ + value for value in column_order_list if not isinstance(value, str) or not value + ] + if invalid_values: + raise ValueError( + "column_order must contain only non-empty strings. The following values " + f"are not valid column names: {invalid_values}." + ) + + seen = set() + duplicates = [] + for value in column_order_list: + if value in seen and value not in duplicates: + duplicates.append(value) + seen.add(value) + if duplicates: + raise ValueError( + f"column_order contains duplicate values: {duplicates}. Each column may " + "only be listed once." + ) + + return list(column_order_list) + + +def resolve_column_order_list( + available_columns: list[str], + pinned_columns: list[str], + requested_columns: list[str] | None = None, +) -> list[str]: + """ + Return pinned, requested, and remaining columns in final display order. + + The resulting order is: + + 1. The pinned columns, in the order given. + 2. The requested columns that were not already pinned, in the order given. + 3. The remaining available columns, in their existing relative order. + + A column never appears more than once, so including a pinned column in + requested_columns leaves it in its pinned position. + + Callers are responsible for making sure every pinned column is present among the + available columns. + + Arguments: + available_columns: Every column that may appear in the final order. + pinned_columns: Columns that must lead the final order, in the desired order. + requested_columns: Optional caller supplied ordering applied after the pinned + columns. + + Returns: + The full list of column names in their final display order. + + Raises: + ValueError: If requested_columns fails validate_column_order_list, or if + it names a column that is not among available_columns. + """ + requested_columns = validate_column_order_list(requested_columns) + + available_set = set(available_columns) + unknown_columns = [ + column for column in requested_columns if column not in available_set + ] + if unknown_columns: + raise ValueError( + "The following column_order values were not found among the available " + f"columns: {unknown_columns}." + ) + + ordered_columns: list[str] = [] + placed = set() + for column in list(pinned_columns) + requested_columns + list(available_columns): + if column not in placed: + ordered_columns.append(column) + placed.add(column) + + return ordered_columns diff --git a/tests/unit/synapseclient/extensions/unit_test_curator.py b/tests/unit/synapseclient/extensions/unit_test_curator.py index 1c2656a8c..fbce627f9 100644 --- a/tests/unit/synapseclient/extensions/unit_test_curator.py +++ b/tests/unit/synapseclient/extensions/unit_test_curator.py @@ -12,8 +12,9 @@ import shutil import tempfile import unittest -from typing import Any -from unittest.mock import Mock, call, mock_open, patch +from collections import OrderedDict +from typing import Any, Callable +from unittest.mock import Mock, mock_open, patch import pandas as pd import pytest @@ -37,7 +38,6 @@ update_wiki_with_entity_view, ) from synapseclient.extensions.curator.record_based_metadata_task import ( - _reorder_columns_with_upsert_keys_first, create_dataframe_from_titles, extract_property_titles, extract_schema_properties_from_dict, @@ -53,6 +53,10 @@ SchemaRegistryColumnConfig, get_latest_schema_uri, ) +from synapseclient.extensions.curator.utils import ( + resolve_column_order_list, + validate_column_order_list, +) from synapseclient.models import Column, ColumnType, ViewTypeMask from synapseclient.models.curation import ( AuthorizationMode, @@ -153,6 +157,7 @@ def test_create_file_based_metadata_task_success_with_schema( synapse_entity_id=self.folder_id, entity_view_name=self.entity_view_name, view_type_mask=ViewTypeMask.FILE, + column_order=None, ) mock_create_wiki.assert_called_once_with( syn=self.mock_syn, entity_view_id="syn87654321", owner_id=self.folder_id @@ -274,6 +279,32 @@ def test_create_file_based_metadata_task_missing_instructions( synapse_client=self.mock_syn, ) + @patch( + "synapseclient.extensions.curator.file_based_metadata_task._create_json_schema_entity_view" + ) + @patch( + "synapseclient.extensions.curator.file_based_metadata_task.Synapse.get_client" + ) + def test_create_file_based_metadata_task_invalid_column_order( + self, mock_get_client, mock_create_entity_view + ): + """A malformed column_order is rejected before any Synapse work happens.""" + # GIVEN a column_order that is not a list + mock_get_client.return_value = self.mock_syn + + # WHEN I create the file-based metadata task + # THEN a ValueError is raised before the entity view is created + with pytest.raises(ValueError, match="must be a list"): + create_file_based_metadata_task( + folder_id=self.folder_id, + curation_task_name=self.curation_task_name, + instructions=self.instructions, + column_order="patientId", + synapse_client=self.mock_syn, + ) + + mock_create_entity_view.assert_not_called() + @patch( "synapseclient.extensions.curator.file_based_metadata_task.Synapse.get_client" ) @@ -502,6 +533,7 @@ def test_create_file_based_metadata_task_forwards_all_params_to_curation_task( synapse_entity_id=self.folder_id, entity_view_name=self.entity_view_name, view_type_mask=ViewTypeMask.FILE, + column_order=None, ) @patch( @@ -680,6 +712,102 @@ def test_create_record_based_metadata_task_success( "A Grid object will no longer be created by this function starting in v5.0.0." ) + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.project_id_from_entity_id" + ) + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.Synapse.get_client" + ) + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.extract_schema_properties_from_web" + ) + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.tempfile.NamedTemporaryFile" + ) + @patch("synapseclient.extensions.curator.record_based_metadata_task.RecordSet") + @patch("synapseclient.extensions.curator.record_based_metadata_task.CurationTask") + @patch("builtins.open") + def test_create_record_based_metadata_task_applies_column_order( + self, + mock_open, + mock_curation_task_cls, + mock_record_set_cls, + mock_temp_file, + mock_extract_schema, + mock_get_client, + mock_get_project_id_from_entity_id, + ): + """Test that column_order orders the CSV template used for the RecordSet.""" + # GIVEN a schema whose properties are not in the desired order + mock_get_client.return_value = self.mock_syn + mock_get_project_id_from_entity_id.return_value = self.project_id + mock_extract_schema.return_value = pd.DataFrame( + columns=["age", "assay", "patientId", "diagnosis", "specimenID"] + ) + + mock_temp = Mock() + mock_temp.name = "/tmp/test.csv" + mock_temp_file.return_value = mock_temp + + mock_record_set = Mock() + mock_record_set.id = "syn87654321" + mock_record_set_cls.return_value.store.return_value = mock_record_set + mock_curation_task_cls.return_value.store.return_value = Mock(task_id="task123") + + # WHEN I create the task with multiple upsert keys and a partial column order + create_record_based_metadata_task( + folder_id=self.folder_id, + record_set_name=self.record_set_name, + record_set_description=self.record_set_description, + curation_task_name=self.curation_task_name, + upsert_keys=["patientId", "specimenID"], + instructions=self.instructions, + schema_uri=self.schema_uri, + column_order=["diagnosis", "patientId"], + create_grid=False, + synapse_client=self.mock_syn, + ) + + # THEN the CSV template written for the RecordSet leads with the upsert keys, + # follows with the requested columns without duplicating the upsert key, and + # keeps the unlisted properties in their relative order + written_csv = "".join( + write_call.args[0] + for write_call in mock_open.return_value.__enter__.return_value.write.call_args_list + ) + assert written_csv.splitlines()[0] == "patientId,specimenID,diagnosis,age,assay" + + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.project_id_from_entity_id" + ) + @patch( + "synapseclient.extensions.curator.record_based_metadata_task.extract_schema_properties_from_web" + ) + def test_create_record_based_metadata_task_invalid_column_order( + self, + mock_extract_schema, + mock_get_project_id_from_entity_id, + ): + """A malformed column_order is rejected before any Synapse work happens.""" + # GIVEN a column_order containing a duplicate + # WHEN I create the record-based metadata task + # THEN a ValueError is raised before the schema is fetched + with pytest.raises(ValueError, match="duplicate"): + create_record_based_metadata_task( + folder_id=self.folder_id, + record_set_name=self.record_set_name, + record_set_description=self.record_set_description, + curation_task_name=self.curation_task_name, + upsert_keys=self.upsert_keys, + instructions=self.instructions, + schema_uri=self.schema_uri, + column_order=["assay", "assay"], + synapse_client=self.mock_syn, + ) + + mock_get_project_id_from_entity_id.assert_not_called() + mock_extract_schema.assert_not_called() + @patch( "synapseclient.extensions.curator.record_based_metadata_task.project_id_from_entity_id" ) @@ -1702,7 +1830,7 @@ def test_query_schema_registry_multiple_filters( ) -class TestRecordBasedHelperFunctions(unittest.TestCase): +class TestRecordBasedHelperFunctions: """Test cases for helper functions in record_based_metadata_task module.""" def test_extract_property_titles_success(self): @@ -1740,9 +1868,9 @@ def test_create_dataframe_from_titles_success(self): result = create_dataframe_from_titles(titles) - self.assertIsInstance(result, pd.DataFrame) - self.assertEqual(list(result.columns), titles) - self.assertEqual(len(result), 0) # Empty DataFrame + assert isinstance(result, pd.DataFrame) + assert list(result.columns) == titles + assert len(result) == 0 # Empty DataFrame def test_create_dataframe_from_titles_empty(self): """Test DataFrame creation with empty titles.""" @@ -1750,8 +1878,8 @@ def test_create_dataframe_from_titles_empty(self): result = create_dataframe_from_titles(titles) - self.assertIsInstance(result, pd.DataFrame) - self.assertEqual(len(result.columns), 0) + assert isinstance(result, pd.DataFrame) + assert len(result.columns) == 0 def test_extract_schema_properties_from_dict_success(self): """Test successful schema property extraction from dictionary.""" @@ -1761,9 +1889,9 @@ def test_extract_schema_properties_from_dict_success(self): result = extract_schema_properties_from_dict(schema_data) - self.assertIsInstance(result, pd.DataFrame) + assert isinstance(result, pd.DataFrame) expected_columns = ["specimenID", "age"] - self.assertEqual(list(result.columns), expected_columns) + assert list(result.columns) == expected_columns @patch("synapseclient.extensions.curator.record_based_metadata_task.JSONSchema") def test_extract_schema_properties_from_web_success(self, mock_schema_cls): @@ -1781,82 +1909,149 @@ def test_extract_schema_properties_from_web_success(self, mock_schema_cls): result = extract_schema_properties_from_web(mock_syn, schema_uri) - self.assertIsInstance(result, pd.DataFrame) + assert isinstance(result, pd.DataFrame) expected_columns = ["specimenID", "age"] - self.assertEqual(list(result.columns), expected_columns) + assert list(result.columns) == expected_columns mock_schema.get.assert_called_once() mock_schema.get_body.assert_called_once() - def test_reorder_columns_with_upsert_keys_first(self): - """Test reordering a DataFrame's columns to put upsert keys first.""" - # GIVEN starting columns, upsert keys, and the expected resulting order - cases = [ + +class TestValidateColumnOrderList: + """Test cases for validate_column_order_list in curator.utils.""" + + @pytest.mark.parametrize( + "column_order,expected", + [ + (None, []), + ([], []), + (["a", "b"], ["a", "b"]), ( - "moves keys to front", - ["age", "diagnosis", "specimenID"], - ["specimenID"], - ["specimenID", "age", "diagnosis"], + ["specimenID", "patientId", "assay"], + ["specimenID", "patientId", "assay"], + ), + ], + ids=["none", "empty list", "valid list", "order is preserved"], + ) + def test_accepts_valid_input(self, column_order, expected): + """None becomes an empty list and a valid list is returned in order.""" + assert validate_column_order_list(column_order) == expected + + @pytest.mark.parametrize( + "column_order,expected_message", + [ + (["patientId", 5], "non-empty strings"), + (["patientId", None], "non-empty strings"), + (["patientId", ""], "non-empty strings"), + (["patientId", "assay", "patientId"], "duplicate"), + ], + ids=[ + "non-string entry", + "none entry", + "empty string entry", + "duplicate entry", + ], + ) + def test_rejects_invalid_input(self, column_order, expected_message): + """Non-lists, non-string entries, and duplicates are rejected.""" + with pytest.raises(ValueError, match=expected_message): + validate_column_order_list(column_order) + + def test_error_names_every_invalid_entry(self): + """The error message lists all offending values, not just the first.""" + with pytest.raises(ValueError, match=r"5.*''"): + validate_column_order_list(["patientId", 5, ""]) + + def test_error_names_every_duplicate_once(self): + """A value repeated several times is reported a single time.""" + with pytest.raises(ValueError, match=r"duplicate values: \['patientId'\]"): + validate_column_order_list(["patientId", "patientId", "patientId"]) + + +class TestResolveColumnOrderList: + """Test cases for resolve_column_order_list in curator.utils.""" + + @pytest.mark.parametrize( + "available,pinned,requested,expected", + [ + ( + ["assay", "name", "id", "createdBy"], + ["name", "id"], + None, + ["name", "id", "assay", "createdBy"], + ), + ( + ["assay", "name", "id", "createdBy", "patientId"], + ["name", "id"], + ["patientId", "createdBy"], + ["name", "id", "patientId", "createdBy", "assay"], + ), + ( + ["assay", "name", "id"], + ["name", "id"], + ["id", "assay"], + ["name", "id", "assay"], + ), + ( + ["a", "b", "c"], + [], + ["c"], + ["c", "a", "b"], ), ( - "preserves provided key order", ["age", "individualID", "diagnosis", "specimenID"], ["specimenID", "individualID"], + None, ["specimenID", "individualID", "age", "diagnosis"], ), ( - "no upsert keys preserves original order", ["age", "specimenID"], [], + None, ["age", "specimenID"], ), - ] - - for name, columns, upsert_keys, expected in cases: - with self.subTest(name): - # WHEN I reorder the columns with the upsert keys first - df = pd.DataFrame(columns=columns) - result = _reorder_columns_with_upsert_keys_first(df, upsert_keys) - - # THEN the upsert keys lead in the given order, others keep their order - self.assertEqual(list(result.columns), expected) - - def test_reorder_columns_with_upsert_keys_first_missing_key_raises(self): - """Callers must validate keys; a missing upsert key raises KeyError.""" - # GIVEN a DataFrame whose columns do not contain every upsert key - df = pd.DataFrame(columns=["age", "specimenID"]) - - # WHEN I reorder with an upsert key absent from the columns - # THEN a KeyError is raised rather than silently dropping the key - with self.assertRaises(KeyError): - _reorder_columns_with_upsert_keys_first(df, ["specimenID", "notAColumn"]) + ( + ["age", "diagnosis", "specimenID"], + ["specimenID"], + ["diagnosis", "age"], + ["specimenID", "diagnosis", "age"], + ), + ], + ids=[ + "no request keeps the pinned columns first", + "requested columns follow the pinned columns", + "a pinned column in the request is not duplicated", + "no pinned columns means the request leads", + "pinned columns lead in the order given, not their available order", + "no pinned columns and no request preserves the original order", + "a full request order is honored exactly", + ], + ) + def test_resolves_expected_order(self, available, pinned, requested, expected): + """Pinned, requested, and remaining columns are concatenated without repeats.""" + assert resolve_column_order_list(available, pinned, requested) == expected + + def test_unknown_column_raises(self): + """Requesting a column that is not available raises a clear ValueError.""" + with pytest.raises( + ValueError, match=r"not found among the available columns: \['x', 'y'\]" + ): + resolve_column_order_list(["a", "b"], ["a"], ["x", "b", "y"]) -class TestFileBasedHelperFunctions(unittest.TestCase): +class TestFileBasedHelperFunctions: """Test cases for helper functions in file_based_metadata_task module.""" - def setUp(self): + @pytest.fixture(autouse=True, scope="function") + def init_mock_syn(self): """Set up test fixtures.""" self.mock_syn = Mock(spec=Synapse) self.mock_syn.logger = Mock() - @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") - @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") - @patch("synapseclient.extensions.curator.file_based_metadata_task.get") - @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") - def test_create_json_schema_entity_view_success( - self, - mock_json_schema_cls, - mock_get, - mock_entity_view_cls, - mock_isinstance, - ): - """Test successful creation of JSON schema entity view.""" - # GIVEN a valid synapse entity with a JSON schema - entity_id = "syn12345678" - entity_view_name = "Test View" - - mock_entity = Mock() - mock_entity.get_schema.return_value = JSONSchemaBinding( + @pytest.fixture + def schema_bound_entity(self) -> Mock: + """A mock Folder/Project that reports a bound JSON schema.""" + entity = Mock() + entity.get_schema.return_value = JSONSchemaBinding( object_id=1, object_type="", created_on="", @@ -1875,18 +2070,69 @@ def test_create_json_schema_entity_view_success( created_by="", ), ) - mock_get.return_value = mock_entity - mock_isinstance.return_value = True + return entity + + @pytest.fixture + def make_json_schema(self) -> Callable[[dict], Mock]: + """Factory for a mock JSONSchema whose body exposes the given properties.""" + + def _make(properties: dict) -> Mock: + schema = Mock() + schema.get_body.return_value = {"properties": properties} + return schema + + return _make + + @pytest.fixture + def make_stored_entity_view(self) -> Callable[[list], Mock]: + """ + Factory for a mock EntityView whose store() returns itself with the given + columns. + + The column names stand in for the state of the view after Synapse has appended + its default columns, which is when the final column order is calculated. + """ + + def _make(column_names: list) -> Mock: + view = Mock() + view.id = "syn87654321" + view.columns = OrderedDict( + (name, Column(name=name, column_type=ColumnType.MEDIUMTEXT)) + for name in column_names + ) + view.store.return_value = view + return view - mock_json_schema = Mock() - mock_json_schema.get_body.return_value = { - "properties": {"name": {"type": "string"}, "age": {"type": "integer"}} - } - mock_json_schema_cls.return_value = mock_json_schema + return _make + + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") + def test_create_json_schema_entity_view_success( + self, + mock_json_schema_cls, + mock_get, + mock_entity_view_cls, + mock_isinstance, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, + ): + """Test successful creation of JSON schema entity view.""" + # GIVEN a valid synapse entity with a JSON schema + entity_id = "syn12345678" + entity_view_name = "Test View" - mock_view = Mock() - mock_view.id = "syn87654321" - mock_view.store.return_value = mock_view + mock_get.return_value = schema_bound_entity + mock_isinstance.return_value = True + mock_json_schema_cls.return_value = make_json_schema( + {"name": {"type": "string"}, "age": {"type": "integer"}} + ) + + mock_view = make_stored_entity_view( + ["age", "name", "createdBy", "id", "modifiedOn"] + ) mock_entity_view_cls.return_value = mock_view # WHEN I create the JSON schema entity view @@ -1899,14 +2145,205 @@ def test_create_json_schema_entity_view_success( # THEN the created EntityView object should be returned assert result is mock_view assert result.id == "syn87654321" - # AND the columns are reordered so that "name", "id", and "createdBy" - # appear first, in that order. - assert mock_view.reorder_column.call_args_list == [ - call(name="name", index=0), - call(name="id", index=1), - call(name="createdBy", index=2), + # AND "name" and "id" are pinned to the front while every other column, + # including "createdBy", keeps its existing relative order. + assert list(result.columns.keys()) == [ + "name", + "id", + "age", + "createdBy", + "modifiedOn", ] + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") + def test_create_json_schema_entity_view_column_order( + self, + mock_json_schema_cls, + mock_get, + mock_entity_view_cls, + mock_isinstance, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, + ): + """A requested column_order is applied after the pinned name and id columns.""" + # GIVEN a stored view whose columns are not in the requested order + mock_get.return_value = schema_bound_entity + mock_isinstance.return_value = True + mock_json_schema_cls.return_value = make_json_schema( + {"assay": {"type": "string"}, "patientId": {"type": "string"}} + ) + mock_view = make_stored_entity_view( + ["assay", "patientId", "name", "createdBy", "id", "fileFormat"] + ) + mock_entity_view_cls.return_value = mock_view + + # WHEN I create the view with an explicit partial column order + result = _create_json_schema_entity_view( + syn=self.mock_syn, + synapse_entity_id="syn12345678", + column_order=["patientId", "fileFormat", "name"], + ) + + # THEN name and id lead, the requested columns follow in the order given, + # and the unlisted columns are appended in their existing relative order + assert list(result.columns.keys()) == [ + "name", + "id", + "patientId", + "fileFormat", + "assay", + "createdBy", + ] + + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") + def test_create_json_schema_entity_view_unknown_column_order_raises( + self, + mock_json_schema_cls, + mock_get, + mock_entity_view_cls, + mock_isinstance, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, + ): + """A column_order naming a column the view does not have raises ValueError.""" + # GIVEN a stored view without the requested column + mock_get.return_value = schema_bound_entity + mock_isinstance.return_value = True + mock_json_schema_cls.return_value = make_json_schema( + {"age": {"type": "integer"}} + ) + mock_view = make_stored_entity_view(["age", "name", "id"]) + mock_entity_view_cls.return_value = mock_view + + # WHEN I create the view requesting a column that does not exist + # THEN a ValueError naming the unknown column is raised + with pytest.raises(ValueError, match=r"\['invalidColumn'\]"): + _create_json_schema_entity_view( + syn=self.mock_syn, + synapse_entity_id="syn12345678", + column_order=["invalidColumn"], + ) + + # AND the EntityView that was already created is deleted rather than left + # behind as an orphan + mock_view.delete.assert_called_once_with(synapse_client=self.mock_syn) + + @patch( + "synapseclient.extensions.curator.file_based_metadata_task.Synapse.get_client" + ) + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") + def test_create_json_schema_entity_view_reports_failed_cleanup( + self, + mock_json_schema_cls, + mock_get, + mock_entity_view_cls, + mock_isinstance, + mock_get_client, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, + ): + """When the cleanup delete fails the original error still propagates.""" + # GIVEN a bad column_order and a view that cannot be deleted + mock_get.return_value = schema_bound_entity + mock_isinstance.return_value = True + mock_get_client.return_value = self.mock_syn + mock_json_schema_cls.return_value = make_json_schema( + {"age": {"type": "integer"}} + ) + mock_view = make_stored_entity_view(["age", "name", "id"]) + mock_view.delete.side_effect = SynapseHTTPError("403 Forbidden") + mock_entity_view_cls.return_value = mock_view + + # WHEN I create the view requesting a column that does not exist + # THEN the ValueError explaining the problem is what propagates, not the + # delete failure + with pytest.raises(ValueError, match=r"\['invalidColumn'\]"): + _create_json_schema_entity_view( + syn=self.mock_syn, + synapse_entity_id="syn12345678", + column_order=["invalidColumn"], + ) + + # AND the Synapse ID needing manual cleanup is logged, with instructions + # on how to delete it + logged_message = self.mock_syn.logger.exception.call_args.args[0] + assert "syn87654321" in logged_message + assert "EntityView(id='syn87654321').delete()" in logged_message + + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + @patch("synapseclient.extensions.curator.file_based_metadata_task.JSONSchema") + def test_create_json_schema_entity_view_order_store_failure_keeps_view( + self, + mock_json_schema_cls, + mock_get, + mock_entity_view_cls, + mock_isinstance, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, + ): + """A failure persisting the column order leaves the EntityView in place.""" + # GIVEN a valid column_order and a store that fails when the order is persisted + mock_get.return_value = schema_bound_entity + mock_isinstance.return_value = True + mock_json_schema_cls.return_value = make_json_schema( + {"age": {"type": "integer"}} + ) + mock_view = make_stored_entity_view(["age", "name", "id"]) + mock_view.store.side_effect = [ + mock_view, + SynapseHTTPError("503 Service Unavailable"), + ] + mock_entity_view_cls.return_value = mock_view + + # WHEN I create the view + # THEN the transient error propagates unchanged + with pytest.raises(SynapseHTTPError, match="503"): + _create_json_schema_entity_view( + syn=self.mock_syn, + synapse_entity_id="syn12345678", + column_order=["age"], + ) + + # AND the view is left alone so that the call can be retried, rather than + # being deleted as it is for a bad column_order + mock_view.delete.assert_not_called() + + @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") + @patch("synapseclient.extensions.curator.file_based_metadata_task.get") + def test_create_json_schema_entity_view_non_container_entity_raises( + self, + mock_get, + mock_entity_view_cls, + ): + """An entity that is not a Folder or Project is rejected with a ValueError.""" + # GIVEN an entity ID that does not resolve to a Folder or a Project + mock_get.return_value = Mock() + + # WHEN I create the JSON schema entity view for it + # THEN a ValueError naming the entity is raised and no view is created + with pytest.raises(ValueError, match="only be read from a Folder or a Project"): + _create_json_schema_entity_view( + syn=self.mock_syn, + synapse_entity_id="syn12345678", + ) + + mock_entity_view_cls.assert_not_called() + @patch("synapseclient.extensions.curator.file_based_metadata_task.isinstance") @patch("synapseclient.extensions.curator.file_based_metadata_task.EntityView") @patch("synapseclient.extensions.curator.file_based_metadata_task.get") @@ -1917,6 +2354,9 @@ def test_create_json_schema_entity_view_with_file_and_folder_view_type_mask( mock_get, mock_entity_view_cls, mock_isinstance, + schema_bound_entity, + make_json_schema, + make_stored_entity_view, ): """Test that a combined FILE|FOLDER view_type_mask is forwarded to EntityView.""" # GIVEN a valid synapse entity with a JSON schema @@ -1924,38 +2364,13 @@ def test_create_json_schema_entity_view_with_file_and_folder_view_type_mask( entity_view_name = "Test View" combined_mask = ViewTypeMask.FILE | ViewTypeMask.FOLDER - mock_entity = Mock() - mock_entity.get_schema.return_value = JSONSchemaBinding( - object_id=1, - object_type="", - created_on="", - created_by="", - enable_derived_annotations=True, - json_schema_version_info=JSONSchemaVersionInfo( - organization_id="", - organization_name="org.name", - schema_id="", - id="", - schema_name="schema.name", - version_id="", - semantic_version="0.0.1", - json_sha256_hex="", - created_on="", - created_by="", - ), - ) - mock_get.return_value = mock_entity + mock_get.return_value = schema_bound_entity mock_isinstance.return_value = True + mock_json_schema_cls.return_value = make_json_schema( + {"name": {"type": "string"}, "age": {"type": "integer"}} + ) - mock_json_schema = Mock() - mock_json_schema.get_body.return_value = { - "properties": {"name": {"type": "string"}, "age": {"type": "integer"}} - } - mock_json_schema_cls.return_value = mock_json_schema - - mock_view = Mock() - mock_view.id = "syn87654321" - mock_view.store.return_value = mock_view + mock_view = make_stored_entity_view(["name", "id", "age"]) mock_entity_view_cls.return_value = mock_view # WHEN I create the JSON schema entity view with both file and folder types