[SYNPY-1869] Review fixes for SearchIndex entity and supporting functionality

[BryanFauble]

Problem:

Review pass over the SearchIndex work on synpy-1869-search-index. Four classes of issue:

1. store_async crashes when upserting onto an existing index. SearchIndex.store_async moved off TableStoreMixin to its own implementation that calls services/storable_entity.store_entity(). That function dereferences resource._last_persistent_instance.version_label unconditionally for models that have a version_number. A freshly-constructed SearchIndex(name=..., parent_id=..., defining_sql=...) that resolves to an already-existing entity via get_id() reaches store_entity() with _last_persistent_instance still None, because merge_dataclass_entities() copies field values but does not set it. Reproduced as AttributeError: 'NoneType' object has no attribute 'version_label'. store_entity() also read resource.force_version, which SearchIndex does not define — AttributeError: 'SearchIndex' object has no attribute 'force_version'.

2. SearchIndex missing from the get_id() type union in models/services/search.py, though store_async now calls it.

3. Dangling docs cross-references. The docstrings add mkdocstrings refs to synapseclient.models.SearchQuery, SearchIndexQuery, search_dsl.Query, TextAnalyzer, and SynonymSet, but no reference page renders SearchIndex or any of the search-management resources, so every one of those refs failed to resolve at build time.

4. Point-in-time comments. Several comments and docstrings narrated the change rather than describing the code: "existing code that passes raw dicts keeps working unchanged", "Left untyped because...", "is deliberately not re-exported here", "States added by Synapse after this client was released", "the entity body is sent to Synapse without an entity wrapper".

5. SearchIndex no longer needs table/versioning semantics.

6. SearchIndexStatus and SearchIndexState are removed along with it

7. added ForwardCompatibleStrEnum, a string enum base that accepts undeclared values instead of raising

Solution:

• services/storable_entity.py — hoist _last_persistent_instance to a local and guard it before the version_label comparison; guard force_version with getattr(..., False). Fixed in the shared function rather than at the SearchIndex call site: every model routed through store_entity() without a persistent instance hits the same crash, so one guard covers all callers instead of one per model.
• models/services/search.py — add SearchIndex to the get_id() union and TYPE_CHECKING imports.
• models/search_index.py — to_synapse_request() returns the entity body directly instead of a single-key {"entity": ...} wrapper that its only caller immediately unwrapped as to_synapse_request()["entity"]. The wrapper was inherited from the TableStoreMixin bundle2 shape, which this model no longer uses.
• Docs — add sync + async reference pages for SearchIndex and for the search configuration resources (SearchConfiguration, TextAnalyzer, SynonymSet, ColumnAnalyzerOverride, SearchConfigBinding), wired into mkdocs.yml nav alongside the other model pages. The org-scoped resources have no delete endpoint, so their pages list store / get / list only.
• Comments — rewritten as durable descriptions of what the code is. "Left untyped because from is a Python keyword" → "Untyped because fromis a Python keyword and cannot be aTypedDict field"; "States added by Synapse after this client was released are preserved" → "A state Synapse returns that is not declared here is preserved as-is"; and so on.

One deliberate narrowing of the reviewed diff: the seven search_dsl TypedDict shapes (Aggregation, SourceFilter, Highlight, FieldCollapse, Rescore, AnalyzerRef, ScalarValue) are not re-exported from synapseclient.models, leaving models/__init__.py unchanged from the base branch. Exporting seven of the eight DSL types while Query stays behind — because the name collides with the existing SQL table-query Query model — is a half-API that needs a comment in __all__ to explain itself. They remain importable as from synapseclient.models.search_dsl import Query, Aggregation, ..., which is one consistent import path for the whole DSL. Say the word if you'd rather have them on the top-level namespace and I'll put them back.

Testing:

• uv run pytest tests/unit — 3003 passed, 2 skipped.
• Added test_store_async_upserts_onto_existing_index, covering the crash in (1). It patches put_entity rather than store_entity so the real store_entity() runs and the guard is actually exercised. Confirmed the test fails without the fix (AttributeError: 'SearchIndex' object has no attribute 'force_version') and again with only the force_version guard restored (AttributeError: 'NoneType' object has no attribute 'version_label'), then passes with both.
• Updated test_to_synapse_request and test_store_async_posts_entity for the unwrapped request body.
• mkdocs build — every search-related autoref warning is gone. Remaining warnings in the build are pre-existing and unrelated to this branch.
• pre-commit run — all hooks pass.
• Integration tests not run (require Synapse credentials); test_search_index_async.py is unchanged by this PR beyond what the base branch already had.

[Copilot]

Pull request overview

This PR refines the experimental SearchIndex model and related search-management types in the Synapse Python Client, focusing on making store_async upserts safe, improving routing through the operations layer, and restoring/searching docs coverage for the search APIs.

Changes:

• Hardened shared entity-storage logic and updated SearchIndex.store_async() to support upsert onto existing indices without crashing.
• Added SearchIndex routing coverage in store_async / get_async / delete_async, plus unit/integration test updates for the new request shape and query dispatch.
• Expanded search documentation and introduced typed OpenSearch query DSL shapes used by SearchQuery and related request models.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/unit/synapseclient/operations/unit_test_store_operations.py	Adds unit coverage ensuring store_async routes SearchIndex correctly (incl. dry-run).
tests/unit/synapseclient/operations/unit_test_factory_operations.py	Adds unit coverage for routing get_async to SearchIndex based on concrete type.
tests/unit/synapseclient/operations/unit_test_delete_operations.py	Adds unit coverage ensuring delete_async routes SearchIndex correctly.
tests/unit/synapseclient/models/async/unit_test_search_management_async.py	Adds coverage for forward-compatible handling of unknown SearchIndexState values.
tests/unit/synapseclient/models/async/unit_test_search_index_async.py	Updates/extends unit tests for SearchIndex store/upsert behavior and query dispatch.
tests/integration/synapseclient/models/async/test_search_index_async.py	Adjusts integration test to use SearchIndex.query_async() rather than constructing SearchIndexQuery directly.
synapseclient/operations/store_operations.py	Adds SearchIndex to supported store routing and help text.
synapseclient/operations/factory_operations.py	Adds SearchIndex to entity-type dispatch in get_async.
synapseclient/operations/delete_operations.py	Adds SearchIndex to supported delete routing and help/doc text.
synapseclient/models/services/storable_entity.py	Adds guards around _last_persistent_instance and force_version in shared store logic.
synapseclient/models/services/search.py	Adds SearchIndex to the get_id() accepted type union.
synapseclient/models/search_management.py	Improves typing/docs for search management models and adopts forward-compatible enum behavior.
synapseclient/models/search_index.py	Switches SearchIndex to a custom store_async, unwrapped request body, plus query/autocomplete typing and tracing.
synapseclient/models/search_dsl.py	Introduces typed OpenSearch Query DSL TypedDict shapes for IDE/type-checking.
synapseclient/models/protocols/search_index_protocol.py	Updates sync protocol typing/docs to include query and new parameter types.
synapseclient/models/organization.py	Adjusts deprecation metadata versions for renamed organization APIs.
synapseclient/models/mixins/table_components.py	Removes SearchIndex from read-only schema list as it no longer uses the table schema/store bundle.
synapseclient/models/mixins/enum_coercion.py	Adds ForwardCompatibleStrEnum to preserve unknown backend enum values without raising.
synapseclient/models/mixins/init.py	Re-exports ForwardCompatibleStrEnum.
mkdocs.yml	Adds navigation entries for new search reference pages (sync + async).
docs/reference/experimental/sync/search_management.md	Adds sync reference page for search configuration resources.
docs/reference/experimental/sync/search_index.md	Adds sync reference page for SearchIndex plus supporting types and DSL module docs.
docs/reference/experimental/async/search_management.md	Adds async reference page for search configuration resources.
docs/reference/experimental/async/search_index.md	Adds async reference page for SearchIndex and supporting types.

---

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

[linglp]

these two lines are causing problems in the integration tests. I deleted them.

[linglp]

after deleting version and version related fields in search index, I have to put a fix here

[linglp]

Move SearchIndex in the block here because it doesn't have version support

[linglp]

@BryanFauble Do we need to add these to the doc?

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (6)

synapseclient/operations/delete_operations.py:321

• Same docstring issue as the sync delete(): SearchIndex is listed as supporting version-specific deletion, but delete_async routes SearchIndex through the non-versioned deletion path. This section should not include SearchIndex.

        - Table, Dataset, DatasetCollection, EntityView, MaterializedView,
          SearchIndex, SubmissionView, VirtualTable (use version_only=True)

synapseclient/models/mixins/init.py:33

• EnumCoercionMixin is not included in synapseclient.models.mixins.all, so even if re-imported it won’t be exported consistently. Add it back to all if you intend to keep it as part of the public mixins surface.

__all__ = [
    "AccessControllable",
    "ProjectSettingsMixin",
    "StorableContainer",
    "StorageLocationConfigurable",

synapseclient/models/services/storable_entity.py:24

• store_entity is annotated as returning bool and its docstring says it returns whether a re-read is required, but the function actually returns the stored/updated entity dict from Synapse. This incorrect return type can confuse callers and type checkers.

async def store_entity(
    resource: Union["File", "Folder", "Project", "Link", "SearchIndex"],
    entity: Dict[str, Union[str, bool, int, float]],
    *,
    synapse_client: Optional[Synapse] = None,
) -> bool:

synapseclient/models/mixins/init.py:5

• EnumCoercionMixin is no longer re-exported from synapseclient.models.mixins, which is a backwards-incompatible API change for any downstream code doing from synapseclient.models.mixins import EnumCoercionMixin. If this removal isn’t intentional, re-add the import.

This issue also appears on line 29 of the same file.

from synapseclient.models.mixins.access_control import AccessControllable
from synapseclient.models.mixins.asynchronous_job import AsynchronousCommunicator
from synapseclient.models.mixins.form import (

synapseclient/models/search_index.py:299

• When upserting onto an existing SearchIndex, merge_dataclass_entities pulls server fields onto self but _last_persistent_instance stays unset. That makes has_changed always True (causing unnecessary PUTs) and makes dry_run diffs compare against an empty SearchIndex instead of the persisted state. Consider setting _last_persistent_instance from the fetched existing entity after merging.

            merge_dataclass_entities(
                source=existing_index, destination=self, logger=client.logger
            )

synapseclient/operations/delete_operations.py:90

• The docstring lists SearchIndex as supporting version-specific deletion, but the implementation treats SearchIndex as not supporting version deletion (it warns and deletes the entire entity). This should be removed to avoid misleading users.

This issue also appears on line 320 of the same file.

        - Table, Dataset, DatasetCollection, EntityView, MaterializedView,
          SearchIndex, SubmissionView, VirtualTable (use version_only=True)

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

synapseclient/operations/delete_operations.py:321

• The delete_async() docstring lists SearchIndex as supporting version-specific deletion, but the implementation treats SearchIndex as not supporting versions (it warns and deletes the whole entity). This is misleading for callers using version_only=True.

        - File, RecordSet (use version_only=True)
        - Table, Dataset, DatasetCollection, EntityView, MaterializedView,
          SearchIndex, SubmissionView, VirtualTable (use version_only=True)

synapseclient/operations/delete_operations.py:90

• The delete() docstring lists SearchIndex as supporting version-specific deletion, but the implementation treats SearchIndex as not supporting versions (it warns and deletes the whole entity). This is misleading for callers using version_only=True.

This issue also appears on line 319 of the same file.

        - File, RecordSet (use version_only=True)
        - Table, Dataset, DatasetCollection, EntityView, MaterializedView,
          SearchIndex, SubmissionView, VirtualTable (use version_only=True)

synapseclient/models/mixins/init.py:29

• EnumCoercionMixin is no longer imported/re-exported from synapseclient.models.mixins, which is a breaking change for callers using from synapseclient.models.mixins import EnumCoercionMixin. If the intent is not to break that import path, re-add the re-export here.

    ProjectSettingsMixin,
    StorageLocationConfigurable,
)

__all__ = [

[BryanFauble]

LGTM! Thank you @linglp for pushing this forward.