fix(#56): hide documents via query_enabled instead of Marqo purge - #145
fix(#56): hide documents via query_enabled instead of Marqo purge#145nexus69420 wants to merge 5 commits into
Conversation
…o purge Include off and Delete flip the search flag so operators can restore without reingest.
KDwevedi
left a comment
There was a problem hiding this comment.
Please address these blockers before merge:
-
MarqoStore.set_query_enabled()discards the response fromupdate_documents()and always reports every requested ID as updated. Marqo can return a successful HTTP response witherrors: trueand per-item failures; in that case the route proceeds to flip SQLite even though search visibility did not change. Please inspect the response, count only confirmed successes, and surface any failed items so the SQLite mutation is not performed. -
apply_document_query_enabled()mutates recorded indexes sequentially. If an early index succeeds and a later index fails, the helper raises before SQLite changes, but the earlier index remains flipped. This is especially unsafe on re-enable because a historical index can become searchable while the document remains disabled in SQLite. Please make failure handling restore already-mutated indexes (or otherwise preserve a consistent per-index/SQLite state) and add a regression where the later sorted index fails.
There is also an acceptance/scope mismatch to resolve explicitly: this PR says it closes #56 while intentionally dropping the required 409 for indexes without query_enabled and leaving the duplicate REVIEW/ADMIN privilege surface unchanged. Please either implement those criteria or update #56 and get agreement on the revised contract. The coordinated index recreation and chat/voice query_enabled:true rollout should remain a merge/deploy prerequisite.
Count only confirmed item successes, restore earlier indexes when a later one fails, and 409 when the index schema has no query_enabled field. SQLite is not flipped unless every target index succeeds.
|
Addressed the review blockers.
Privilege: chunk Chat/voice |
KDwevedi
left a comment
There was a problem hiding this comment.
The substantive review blockers are addressed: per-item Marqo failures now fail closed, earlier indexes are compensated on a later failure, the missing-schema path returns 409, and the privilege contract is documented.
One branch-specific suite regression remains. The full suite now fails test_disable_document_hides_in_own_tenant_index_not_legacy: its fake tenant index does not advertise query_enabled, so the route correctly returns the new 409 instead of exercising the tenant-index isolation assertion. Please update that non-legacy fake/schema fixture to include the new filterable field while keeping the legacy fake without it, then rerun the full suite.
Current full result: 879 passed, 2 failed; the other failure is the existing taxonomy-console assertion that also fails on main.
… fake Disable/include now 409s when the index schema has no query_enabled. The tenant-isolation fake only advertised instance, so hide-in-own-index never reached the isolation assertion. Legacy fakes still omit the field.
|
The isolation fake now advertises
Also merged |
Non-legacy hide now goes through the same update_documents item payload as production. A tenant index without query_enabled 409s and does not delete or disable.
Summary
query_enabledfilter field before more indexes are created #56. Hide/show is aquery_enabledflag flip, not a Marqo purge (except chunk hard Delete). Search ANDsquery_enabled:true. Excluded chunks are still ingested so Include on does not need reingest.query_enabled:falseand exclude all SQLite chunks. Records stay in the index. They differ on the list: Delete also setsis_disabled, hides the card, and can cancel Temporal. Restore only unhides the card; Include on brings search back. Chunk Delete remains the only hard content delete.query_enabledfield and full reingest. Chat/voice (outside this repo) must filterquery_enabled:true. Intentionally dropped Chunk soft-disable, done properly: addquery_enabledfilter field before more indexes are created #56's 409-on-missing-field (recreate instead). Privilege split (Review chunk vs Admin doc) left as-is.purge_artifactsfrom P1 Data lifecycle: prevent orphan artifacts/rows on soft-delete and hard-delete #135 is unchanged.Test plan
pytest tests/test_document_lifecycle.py tests/test_marqo_doc_scope.py tests/test_search_service.py tests/test_vector_store.py tests/test_tenant_isolation.py tests/test_db.py tests/test_activities.py(182 passed locally; the remaining failure is untrackedscripts/_h100_reconcile_v2_marqo.pyleaking a Marqo client, not this PR)query_enabled:falseis_excludedby unchangedoriginal_text; reingest usesinclude_excluded=Trueindexed(notremoved)