[SYNPY-1671] Adding store & delete factory method

[BryanFauble]

Problem:

• The synapseclient library requires users to instantiate entity classes and call methods on those instances for common operations like storing and deleting, which creates unnecessary friction.
• Following the factory-style pattern established with the get/get_async implementation, additional methods need to be exposed as standalone functions for a more streamlined developer experience.
• This PR addresses a subset of the methods identified in the Jira ticket: store and delete.

Solution:

• Implemented new factory functions following the pattern established by get/get_async:
  • delete / delete_async: Unified interface for deleting any Synapse entity type (File, Folder, Project, Table, Dataset, Team, etc.) with support for:
    • Deletion by entity object or string Synapse ID (e.g., "syn123456" or "syn123456.4")
    • Version-specific deletion with clear precedence rules (explicit version parameter > entity's version_number attribute > ID string version)
    • Safety validation requiring version_only=True when deleting specific versions
    • Appropriate warnings for entity types that don't support version-specific deletion
  • store / store_async: Unified interface for storing any Synapse entity type with type-specific option classes:
    • StoreFileOptions: Controls for synapse_store, content_type, merge_existing_annotations, associate_activity_to_new_version
    • StoreContainerOptions: Controls for failure_strategy (LOG_EXCEPTION vs RAISE_EXCEPTION)
    • StoreTableOptions: Controls for dry_run and job_timeout
    • StoreJSONSchemaOptions: Required options for JSONSchema entities including schema_body, version, dry_run
    • StoreGridOptions: Controls for attach_to_previous_session and timeout
• Updated synapseclient/operations/__init__.py to expose all new functions and option classes at the package level.

Testing:

• Added comprehensive integration test suites covering both synchronous and asynchronous variants:
  • test_delete_operations_async.py / test_delete_operations.py: Tests for file deletion by ID string and object, version-specific deletion with various precedence scenarios, error handling for invalid IDs and missing version numbers, warning logging for unsupported version deletion on entity types like Project/Folder
  • test_factory_operations_store_async.py / test_factory_operations_store.py: Tests for storing all supported entity types (Project, Folder, File, Table, Dataset, EntityView, Team, Evaluation, CurationTask, JSONSchema, Grid, etc.), option class functionality, update workflows, and error handling for unsupported types

[Copilot]

Pull request overview

This PR introduces factory-style methods store, store_async, delete, and delete_async to simplify common operations on Synapse entities. The implementation follows the pattern established by the existing get/get_async methods and provides a unified interface for storing and deleting various entity types with type-specific configuration options.

Key Changes:

• New store/store_async functions supporting 15+ entity types with 5 specialized option classes (StoreFileOptions, StoreContainerOptions, StoreTableOptions, StoreJSONSchemaOptions, StoreGridOptions)
• New delete/delete_async functions with version-specific deletion support and clear precedence rules for version parameters
• Comprehensive integration test suites covering both synchronous and asynchronous variants

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
synapseclient/operations/store_operations.py	Implements store/store_async factory methods with entity-specific handlers and option classes
synapseclient/operations/delete_operations.py	Implements delete/delete_async factory methods with version handling and validation
synapseclient/operations/init.py	Exports new functions and option classes at package level
tests/integration/synapseclient/operations/synchronous/test_factory_operations_store.py	Integration tests for synchronous store operations covering all supported entity types
tests/integration/synapseclient/operations/synchronous/test_delete_operations.py	Integration tests for synchronous delete operations including version-specific deletion
tests/integration/synapseclient/operations/async/test_factory_operations_store_async.py	Async variants of store operation integration tests
tests/integration/synapseclient/operations/async/test_delete_operations_async.py	Async variants of delete operation integration tests

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The test description says "Grid deletion is fire-and-forget, no need to verify" but this is misleading - the delete operation is still being called and could potentially fail silently. Either verify the deletion succeeded or document why verification is not necessary for Grid entities specifically.

Suggested change

	# Grid deletion is fire-and-forget, no need to verify
	# For Grid entities, this test only checks that delete() can be called without raising.

[Copilot]

The error message includes a hardcoded suggestion with 'delete()' but does not specify the async variant. When this error is raised from delete_async(), the suggestion should say delete_async() instead. Consider making the function name dynamic or providing context-appropriate suggestions.

Suggested change

	f"Deleting a specific version requires version_only=True. "
	f"Use delete('{entity}', version_only=True) to delete version {final_version}."
	"Deleting a specific version requires version_only=True. "
	f"Pass version_only=True when calling this function to delete version {final_version}."

[Copilot]

The type hint for version parameter allows Union[int, str], but the comparison at line 493 version != entity_version doesn't account for type coercion (e.g., version=2 as string "2" vs entity_version=2 as int). This could cause false positive warnings. Consider normalizing both to the same type before comparison, or document that version should always be an int.

Suggested change

	# Emit warning only when there's an actual version conflict (both are set and different)
	if (
	version_only
	and version is not None
	and entity_version is not None
	and version != entity_version
	# Normalize versions for comparison to avoid false conflicts between str/int
	normalized_version = (
	int(version) if isinstance(version, str) and version.isdigit() else version
	)
	normalized_entity_version = (
	int(entity_version)
	if isinstance(entity_version, str) and entity_version.isdigit()
	else entity_version
	)
	# Emit warning only when there's an actual version conflict (both are set and different)
	if (
	version_only
	and normalized_version is not None
	and normalized_entity_version is not None
	and normalized_version != normalized_entity_version

[Copilot]

The version parameter type is Union[int, str] but this is assigned directly to entity.version_number at line 516 without type conversion. If final_version_for_entity is a string, this could cause issues if version_number expects an int. Consider converting to int or clarifying the expected type.

Suggested change

	# Set the entity's version_number to the final version so delete_async uses it
	entity.version_number = final_version_for_entity
	# Normalize final_version_for_entity to an int before assigning
	if isinstance(final_version_for_entity, str):
	try:
	final_version_for_entity_int = int(final_version_for_entity)
	except ValueError as exc:
	raise ValueError(
	f"Invalid version value '{final_version_for_entity}'; an integer is required."
	) from exc
	else:
	final_version_for_entity_int = final_version_for_entity
	# Set the entity's version_number to the final version so delete_async uses it
	entity.version_number = final_version_for_entity_int

[andrewelamb]

I think I've seen this logic elsewhere. It might make sense to have a SynapseID object that does this check. Then this function could take in a str or SynapseID type.

class SynapseID(id: str):


      if not is_synapse_id_str(id):
            raise ValueError(
                f"Invalid Synapse ID: {entity}. "
                "Expected a valid Synapse ID string (e.g., 'syn123456' or 'syn123456.4')."
            )

    self.id, self.version = get_synid_and_version(id)


Then it could do:

if isinstance(entity, str):
entity = SynapseID(entity)

[BryanFauble]

I think that it needs to be a little easier to verify the Synapse ID format, but putting it into a class doesn't seem like the right thing. Rather it probably makes sense to be a utility function.

[andrewelamb]

LGTM!

In general, to fix incomplete URL substring sanitization, the URL must be parsed and its components (especially hostname) checked explicitly, instead of using substring checks on the raw URL string. For host checks, use a proper URL parser and compare the hostname (or carefully checked suffix) to an allowlist of expected domains.

For this specific test file, we should stop asserting "synapse.org" in url.lower() and instead parse the URL and assert that its hostname matches the expected Synapse web domain. We should do this wherever that substring check appears (lines 203, 216, 234). Python’s standard library urllib.parse is already appropriate and needs no external dependency. Concretely:

• Add from urllib.parse import urlparse at the top of tests/integration/synapseclient/operations/async/test_utility_operations_async.py.
• In test_onweb_async_project_by_id, replace assert "synapse.org" in url.lower() with parsing the URL (parsed = urlparse(url)) and asserting on parsed.hostname, e.g. assert parsed.hostname and parsed.hostname.lower().endswith("synapse.org"). This allows subdomains like www.synapse.org while rejecting hosts that merely contain synapse.org somewhere else.
• Apply the same pattern in test_onweb_async_project_by_object and test_onweb_async_with_subpage.

This preserves the existing intent (“URL is a Synapse URL”) while making the checks precise and avoiding unsafe substring usage.

In general, the way to fix this class of issue is to stop treating the URL as an opaque string for domain validation and instead parse it with a URL parser, then inspect the hostname (and possibly scheme and path) explicitly. For Synapse URLs, we likely expect hostnames such as www.synapse.org or other subdomains of synapse.org; using urllib.parse.urlparse to obtain parsed.hostname and asserting that it either equals synapse.org or ends with .synapse.org is both precise and robust against substring tricks.

For this specific test file, we should:

• Import urlparse from urllib.parse.
• Replace the three assertions assert "synapse.org" in url.lower() with hostname-based assertions.
• Because we do not know all valid Synapse hosts from this snippet, the safest non-breaking change is to assert that the parsed hostname ends with synapse.org (allowing www.synapse.org, foo.synapse.org, etc.), and that parsing succeeded.

Concretely:

• At the top of tests/integration/synapseclient/operations/async/test_utility_operations_async.py, add from urllib.parse import urlparse alongside existing imports.
• For each of the three tests that currently contain assert "synapse.org" in url.lower() (lines 203, 216, 234), first parse the URL: parsed = urlparse(url), then:
  • Assert parsed.hostname is not None.
  • Assert parsed.hostname.lower().endswith("synapse.org").

This changes the tests from substring checks to host-based checks while preserving their intent of verifying that onweb_async returns a proper Synapse URL.

In general, to avoid incomplete URL sanitization, you should parse the URL with a standard library (such as Python’s urllib.parse.urlparse) and validate the hostname component rather than searching for a domain substring in the raw URL string. This ensures that domain checks apply to the actual host part of the URL and are not accidentally satisfied by occurrences in the path, query, or other components.

For this test file, the best fix is to replace the substring assertion assert "synapse.org" in url.lower() with a hostname-based check using urllib.parse.urlparse. Since we cannot assume or modify other files, we will update only this test module. Specifically:

• Add an import for urlparse from urllib.parse at the top of tests/integration/synapseclient/operations/async/test_utility_operations_async.py.
• In the three tests that currently assert "synapse.org" in url.lower() (lines 203, 216, and 234), parse the URL and assert that the parsed hostname equals "synapse.org" or ends with ".synapse.org". This is stricter and aligns with the recommended pattern.
• Keep the rest of the assertions unchanged to preserve existing functionality.

Concretely:

• At the top of the file, add from urllib.parse import urlparse.
• In each test (test_onweb_async_project_by_id, test_onweb_async_project_by_object, and test_onweb_async_with_subpage), insert something like:

  parsed = urlparse(url)
  assert parsed.hostname is not None
  assert parsed.hostname == "synapse.org" or parsed.hostname.endswith(".synapse.org")

  and remove the old substring assertion.

No additional helper methods are required; we only need the standard-library import and minor assertion changes.

In general, the issue is that using a substring check like "synapse.org" in url.lower() does not actually verify that the URL’s host is synapse.org (or a valid subdomain), only that this text appears somewhere in the string. The robust fix is to parse the URL with urllib.parse.urlparse, then assert on the hostname component (for example, that it equals or ends with synapse.org). This matches the recommendation in the background: always parse the URL and check the host value instead of using string containment.

For this specific test file, we should update the tests that currently assert "synapse.org" in url.lower() (lines 190 and 203, and potentially 219) to parse the URL and check its hostname. We can do this without altering the behavior of onweb; we are only verifying its output more strictly. Concretely:

• Add an import for urlparse from urllib.parse at the top of tests/integration/synapseclient/operations/synchronous/test_utility_operations.py.
• In test_onweb_project_by_id, replace assert "synapse.org" in url.lower() with code that parses url and asserts that parsed.hostname is either exactly synapse.org or ends with .synapse.org. Using an endswith check supports any subdomains that onweb might legitimately generate (e.g., www.synapse.org).
• Do the same replacement in test_onweb_project_by_object and test_onweb_with_subpage for their "synapse.org" in url.lower() checks, to keep tests consistent and to resolve any similar CodeQL warnings.

No new methods are needed; only the new import and slight in-test logic changes are required.

In general, to avoid incomplete URL substring sanitization, parse the URL and validate the hostname (and optionally scheme), instead of checking whether a domain string appears anywhere in the URL. In Python, urllib.parse.urlparse is the standard way to do this. You then compare parsed.hostname or enforce an allow list of hostnames or suffixes.

For this specific test, we should replace the assertion assert "synapse.org" in url.lower() with logic that parses url, extracts the hostname, normalizes it to lowercase, and asserts that the hostname is either exactly synapse.org or ends with .synapse.org. That matches the intent (“a valid Synapse URL”) without relying on substring checks. To implement this, we need to import urllib.parse.urlparse at the top of tests/integration/synapseclient/operations/synchronous/test_utility_operations.py and update the assertions in the three test_onweb_* methods (lines 190, 203, and 219) to use urlparse(url).hostname. No other behavior of the tests should change: they should still check that the returned URL is non-None, a string, and contains the appropriate IDs and labels.

Concretely:

• Add from urllib.parse import urlparse alongside the existing imports at the top of the file.
• In test_onweb_project_by_id, replace assert "synapse.org" in url.lower() with a parsed-hostname check.
• In test_onweb_project_by_object, replace assert "synapse.org" in url.lower() similarly.
• In test_onweb_with_subpage, replace assert "synapse.org" in url.lower() similarly.
  This preserves all existing semantics while enforcing a more precise and secure notion of “Synapse URL”.

In general, the fix is to stop treating the URL as an arbitrary string and instead parse it, then check its hostname (and optionally scheme) explicitly. Rather than verifying that "synapse.org" appears anywhere in the URL, we should use urllib.parse.urlparse to extract netloc or hostname and assert that it matches or safely ends with the expected domain (for example, equals "synapse.org" or ends with ".synapse.org"). This aligns with the recommendation in the background material.

For this specific test file, we only need to adjust the assertions in the test_onweb_* tests so they verify the parsed hostname instead of (or in addition to) using substring membership. The minimal, behavior‑preserving change is:

• Import urlparse from urllib.parse at the top of the file.
• In test_onweb_project_by_id, test_onweb_project_by_object, and test_onweb_with_subpage, replace the assertion assert "synapse.org" in url.lower() with code that:
  • parses the URL (parsed = urlparse(url)),
  • obtains the hostname (host = parsed.hostname),
  • and asserts host == "synapse.org" or host.endswith(".synapse.org") as appropriate.

Given Synapse typically uses a single domain host like www.synapse.org or synapse.org, a robust check is to assert that host is exactly "synapse.org" or ends with ".synapse.org". This prevents "evil-synapse.org" from passing. To avoid changing tested functionality too much, we won’t enforce an exact value for subdomains, only that they are under synapse.org. We can implement a small helper expression inline in each test:

parsed = urlparse(url)
host = parsed.hostname
assert host is not None
assert host == "synapse.org" or host.endswith(".synapse.org")

This keeps the rest of the tests unchanged and introduces only one new standard-library import.

---