Conversation
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/server/manage/search_settings.py">
<violation number="1" location="backend/onyx/server/manage/search_settings.py:41">
P2: Return an HTTPException with an explicit status/detail instead of raising NotImplementedError, so clients get a clear, intentional response instead of a 500.</violation>
<violation number="2" location="backend/onyx/server/manage/search_settings.py:137">
P2: Return an HTTPException with an explicit status/detail instead of raising NotImplementedError, so clients receive a clear disabled-endpoint response.
(Based on your team's feedback about FastAPI endpoint error handling.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryDisables secondary index functionality across the backend by hardcoding return values and raising NotImplementedError in key functions. The changes prevent creation of new search settings, index swapping, and secondary index operations during ingestion. Key changes:
The approach is consistent with temporary feature disabling, allowing easy re-enablement by uncommenting code. However, in Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant API
participant SearchSettings
participant SwapIndex
participant Setup
participant DocIndex
Note over Client,DocIndex: Secondary Index Disabled Flow
Client->>API: POST /set-new-search-settings
API->>API: Raise NotImplementedError
API-->>Client: Error: "Setting new search settings is temporarily disabled"
Client->>API: POST /cancel-new-embedding
API->>API: Raise NotImplementedError
API-->>Client: Error: "Cancelling new embedding is temporarily disabled"
Note over Setup,DocIndex: On Startup/Setup
Setup->>SwapIndex: check_and_perform_index_swap()
SwapIndex->>SwapIndex: Log warning
SwapIndex-->>Setup: Return None (no swap)
Setup->>SearchSettings: get_active_search_settings()
SearchSettings->>SearchSettings: get_secondary_search_settings()
SearchSettings->>SearchSettings: Log warning
SearchSettings-->>SearchSettings: Return None
SearchSettings-->>Setup: ActiveSearchSettings(primary, secondary=None)
Setup->>Setup: Hardcode secondary_search_settings = None
Setup->>DocIndex: get_default_document_index(primary, None)
DocIndex-->>Setup: Document index initialized
Note over Client,DocIndex: Ingestion Flow
Client->>API: POST /upsert-document
API->>API: Check active_search_settings.secondary
API->>API: Skip secondary indexing (commented out)
API-->>Client: Success (only primary index updated)
|
backend/onyx/setup.py
Outdated
| secondary_search_settings = active_search_settings.secondary | ||
| # TODO(andrei): Re-enable this bit. | ||
| # secondary_search_settings = active_search_settings.secondary | ||
| secondary_search_settings: SearchSettings | None = None |
There was a problem hiding this comment.
style: Hardcoding secondary_search_settings to None violates the custom instruction to remove the variable entirely when hardcoding to a constant. The variable is still used on lines 94, 141, and 148-149. Consider removing the variable and cleaning up all usage sites:
- Line 94: The condition
if secondary_search_settings and DISABLE_INDEX_UPDATE_ON_SWAP:will always beFalse, so remove the entire block - Line 141: Pass
Nonedirectly toget_default_document_index() - Lines 148-149: Simplify to just
None
| secondary_search_settings: SearchSettings | None = None | |
| # TODO(andrei): Re-enable this bit. | |
| # secondary_search_settings = active_search_settings.secondary |
Context Used: Rule from dashboard - When hardcoding a boolean variable to a constant value, remove the variable entirely and clean up al... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/setup.py
Line: 82:82
Comment:
**style:** Hardcoding `secondary_search_settings` to `None` violates the custom instruction to remove the variable entirely when hardcoding to a constant. The variable is still used on lines 94, 141, and 148-149. Consider removing the variable and cleaning up all usage sites:
- Line 94: The condition `if secondary_search_settings and DISABLE_INDEX_UPDATE_ON_SWAP:` will always be `False`, so remove the entire block
- Line 141: Pass `None` directly to `get_default_document_index()`
- Lines 148-149: Simplify to just `None`
```suggestion
# TODO(andrei): Re-enable this bit.
# secondary_search_settings = active_search_settings.secondary
```
**Context Used:** Rule from `dashboard` - When hardcoding a boolean variable to a constant value, remove the variable entirely and clean up al... ([source](https://app.greptile.com/review/custom-context?memory=17793125-da1f-469f-ae44-062cf6bd177e))
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
that's too much work
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/db/swap_index.py">
<violation number="1">
P1: Search settings are marked swapped before the Vespa index swap succeeds. If ensure_indices_exist fails, the function returns None but the DB status is already PAST/PRESENT, leaving the system in an inconsistent state. Update statuses only after the swap succeeds (or roll back on failure).</violation>
</file>
<file name="backend/onyx/db/search_settings.py">
<violation number="1">
P2: get_secondary_search_settings now returns the latest FUTURE settings, which re-enables secondary index handling via get_active_search_settings. If secondary indices are meant to be disabled, this should return None (and optionally log a warning) instead of querying the DB.</violation>
</file>
<file name="backend/onyx/server/onyx_api/ingestion.py">
<violation number="1">
P2: This block re-enables secondary indexing for ingestion, which contradicts the stated goal of disabling secondary indices. If a secondary search setting exists, ingestion will still write to it. Consider skipping secondary indexing entirely while the feature is disabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…secondary-indices-backend
Description
We'll re-enable this after the OpenSearch feature work is done.
NOTE: I made some changes since the last review, turns out you cannot just disable all of
get_secondary_search_settingsand related interactions because by default when we create a fresh instance of Onyx, if the user has notuser_has_overridden_embedding_modelwe automatically create a secondary index for them. This logic is inbackend/alembic/versions/dbaa756c2ccf_embedding_models.py.How Has This Been Tested?
I trust CI.
Additional Options
Summary by cubic
Temporarily disable secondary indices and index swapping across the backend while we finish the OpenSearch work. All flows now use only the primary search settings.
Written for commit 610c011. Summary will update on new commits.