[SYNPY-1869] Review fixes for SearchIndex entity and supporting functionality#1433
Open
BryanFauble wants to merge 2 commits into
Open
[SYNPY-1869] Review fixes for SearchIndex entity and supporting functionality#1433BryanFauble wants to merge 2 commits into
BryanFauble wants to merge 2 commits into
Conversation
- Fix store_async crash when upserting onto an existing SearchIndex: a
freshly-constructed instance has no _last_persistent_instance, which
store_entity dereferenced unconditionally for the version_label check.
- Guard force_version in store_entity for models that do not define it.
- Add SearchIndex to the get_id() type union in services/search.py.
- Flatten SearchIndex.to_synapse_request() to return the entity body
directly instead of a single-key {"entity": ...} wrapper the only
caller immediately unwrapped.
- Add reference docs for SearchIndex and the search configuration
resources, resolving the dangling mkdocstrings cross-references to
SearchQuery, SearchIndexQuery, search_dsl.Query, TextAnalyzer, and
SynonymSet.
- Rewrite point-in-time comments as durable descriptions of what the
code is.
- Add unit coverage for the upsert-onto-existing-index path.
Contributor
There was a problem hiding this comment.
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
SearchIndexrouting coverage instore_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
SearchQueryand 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.
Comment on lines
48
to
52
| if ( | ||
| resource.version_label | ||
| and resource.version_label | ||
| != resource._last_persistent_instance.version_label | ||
| and last_persistent_instance | ||
| and resource.version_label != last_persistent_instance.version_label | ||
| ): |
Comment on lines
251
to
255
| async def store_async( | ||
| self, | ||
| dry_run: bool = False, | ||
| *, | ||
| job_timeout: int = 600, | ||
| synapse_client: Optional[Synapse] = None, |
Comment on lines
23
to
28
| def store( | ||
| self, | ||
| dry_run: bool = False, | ||
| *, | ||
| job_timeout: int = 600, | ||
| synapse_client: Optional[Synapse] = None, | ||
| ) -> "Self": |
The SearchIndex and SearchIndexQuery class docstrings opened their ```python fence directly under `Example:`, with no ` ` description line. griffe only parses an Examples section when the title line is followed by body text, so both blocks fell through as literal markdown and the fence leaked onto the rendered page instead of becoming a collapsible example admonition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
Review pass over the SearchIndex work on
synpy-1869-search-index. Four classes of issue:store_asynccrashes when upserting onto an existing index.SearchIndex.store_asyncmoved offTableStoreMixinto its own implementation that callsservices/storable_entity.store_entity(). That function dereferencesresource._last_persistent_instance.version_labelunconditionally for models that have aversion_number. A freshly-constructedSearchIndex(name=..., parent_id=..., defining_sql=...)that resolves to an already-existing entity viaget_id()reachesstore_entity()with_last_persistent_instancestillNone, becausemerge_dataclass_entities()copies field values but does not set it. Reproduced asAttributeError: 'NoneType' object has no attribute 'version_label'.store_entity()also readresource.force_version, whichSearchIndexdoes not define —AttributeError: 'SearchIndex' object has no attribute 'force_version'.SearchIndexmissing from theget_id()type union inmodels/services/search.py, thoughstore_asyncnow calls it.Dangling docs cross-references. The docstrings add mkdocstrings refs to
synapseclient.models.SearchQuery,SearchIndexQuery,search_dsl.Query,TextAnalyzer, andSynonymSet, but no reference page rendersSearchIndexor any of the search-management resources, so every one of those refs failed to resolve at build time.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 anentitywrapper".Solution:
services/storable_entity.py— hoist_last_persistent_instanceto a local and guard it before theversion_labelcomparison; guardforce_versionwithgetattr(..., False). Fixed in the shared function rather than at theSearchIndexcall site: every model routed throughstore_entity()without a persistent instance hits the same crash, so one guard covers all callers instead of one per model.models/services/search.py— addSearchIndexto theget_id()union andTYPE_CHECKINGimports.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 asto_synapse_request()["entity"]. The wrapper was inherited from theTableStoreMixinbundle2 shape, which this model no longer uses.SearchIndexand for the search configuration resources (SearchConfiguration,TextAnalyzer,SynonymSet,ColumnAnalyzerOverride,SearchConfigBinding), wired intomkdocs.ymlnav alongside the other model pages. The org-scoped resources have no delete endpoint, so their pages liststore/get/listonly."Left untyped becausefromis a Python keyword"→"Untyped becausefromis a Python keyword and cannot be aTypedDictfield";"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_dslTypedDictshapes (Aggregation,SourceFilter,Highlight,FieldCollapse,Rescore,AnalyzerRef,ScalarValue) are not re-exported fromsynapseclient.models, leavingmodels/__init__.pyunchanged from the base branch. Exporting seven of the eight DSL types whileQuerystays behind — because the name collides with the existing SQL table-queryQuerymodel — is a half-API that needs a comment in__all__to explain itself. They remain importable asfrom 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.test_store_async_upserts_onto_existing_index, covering the crash in (1). It patchesput_entityrather thanstore_entityso the realstore_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 theforce_versionguard restored (AttributeError: 'NoneType' object has no attribute 'version_label'), then passes with both.test_to_synapse_requestandtest_store_async_posts_entityfor the unwrapped request body.mkdocs build— everysearch-related autoref warning is gone. Remaining warnings in the build are pre-existing and unrelated to this branch.pre-commit run— all hooks pass.test_search_index_async.pyis unchanged by this PR beyond what the base branch already had.