fix: improve index attempts API #5287
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| user: User, | ||
| get_editable: bool = True, | ||
| ) -> bool: | ||
| stmt = select(ConnectorCredentialPair.id) |
There was a problem hiding this comment.
fetch less info than before (likely trivial improvement)
| select(IndexAttempt) | ||
| .join(ConnectorCredentialPair) | ||
| .where(ConnectorCredentialPair.connector_id == connector_id) | ||
| stmt = select(IndexAttempt).where( |
There was a problem hiding this comment.
avoid unnecessary join
| select(IndexAttempt) | ||
| .join(ConnectorCredentialPair) | ||
| .where(ConnectorCredentialPair.connector_id == connector_id) | ||
| stmt = select(IndexAttempt).where( |
There was a problem hiding this comment.
avoid unnecessary join
| stmt = stmt.offset(page * page_size).limit(page_size) | ||
| stmt = stmt.options( | ||
| contains_eager(IndexAttempt.connector_credential_pair), | ||
| joinedload(IndexAttempt.error_rows), |
There was a problem hiding this comment.
don't need this stuff
|
|
||
| // need to always have the most recent index attempts around | ||
| // so just kick off a separate fetch | ||
| const { |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR improves the index attempts API by refactoring the pagination system to work directly with connector-credential pair IDs (cc_pair_id) rather than going through connector IDs. The changes span both backend and frontend components to create a more direct and efficient data flow.
On the backend, the core improvement involves updating database functions in backend/onyx/db/index_attempt.py to use cc_pair_id directly instead of requiring joins with the ConnectorCredentialPair table. The count_index_attempts_for_connector function was renamed to count_index_attempts_for_cc_pair and get_paginated_index_attempts_for_cc_pair_id was updated to filter directly on the connector_credential_pair_id field. This eliminates unnecessary table joins and improves query performance.
The API endpoints in backend/onyx/server/documents/cc_pair.py were simplified by introducing a new verify_user_has_access_to_cc_pair function that provides lightweight permission checking without fetching full objects. This replaces the previous approach of using get_connector_credential_pair_from_id_for_user which was overkill for simple access verification.
On the frontend, the changes remove duplicate data fetching in web/src/app/admin/connector/[ccPairId]/page.tsx where two separate usePaginatedFetch hooks were making redundant API calls for the same index attempts data. The component structure was simplified to use a single pagination hook, and the IndexingAttemptsTable component was renamed to IndexAttemptsTable for naming consistency across the codebase.
These changes align with the repository's principles of avoiding duplicate code, preferring simplicity where there's no clear gain from complexity, and maintaining consistency. The refactoring creates a more direct data flow that reduces database queries and API calls while maintaining all existing functionality.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it primarily optimizes existing functionality without changing core behavior
- Score reflects straightforward refactoring that improves performance and code organization while maintaining existing functionality
- Pay close attention to the database query changes in
backend/onyx/db/index_attempt.pyto ensure the simplified queries return the same results
Context used:
Rule - When implementing pagination parameters in backend APIs that will be consumed by the frontend's usePaginatedFetch hook, the parameter must be named page_num instead of page. (link)
5 files reviewed, 1 comment
| stmt = select(ConnectorCredentialPair.id) | ||
| stmt = _add_user_filters(stmt, user, get_editable) | ||
| stmt = stmt.where(ConnectorCredentialPair.id == cc_pair_id) | ||
| result = db_session.execute(stmt) | ||
| return result.scalars().first() is not None |
There was a problem hiding this comment.
style: Consider using .scalar() instead of .scalars().first() since we're selecting a single column and only need one result
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| get_editable: bool = True, | ||
| ) -> bool: | ||
| stmt = select(ConnectorCredentialPair.id) | ||
| stmt = _add_user_filters(stmt, user, get_editable) |
There was a problem hiding this comment.
Edit access granted to non-curator group members via unguarded get_editable call.
Prompt for AI agents
Address the following comment on backend/onyx/db/connector_credential_pair.py at line 285:
<comment>Edit access granted to non-curator group members via unguarded get_editable call.</comment>
<file context>
@@ -275,6 +275,19 @@ def get_connector_credential_pair_from_id_for_user(
+ get_editable: bool = True,
+) -> bool:
+ stmt = select(ConnectorCredentialPair.id)
+ stmt = _add_user_filters(stmt, user, get_editable)
+ stmt = stmt.where(ConnectorCredentialPair.id == cc_pair_id)
+ result = db_session.execute(stmt)
</file context>
| def verify_user_has_access_to_cc_pair( | ||
| cc_pair_id: int, | ||
| db_session: Session, | ||
| user: User, |
There was a problem hiding this comment.
Type annotation excludes None; inconsistent with helpers that accept Optional[User] and may prevent use in anonymous/disabled-auth flows.
Prompt for AI agents
Address the following comment on backend/onyx/db/connector_credential_pair.py at line 281:
<comment>Type annotation excludes None; inconsistent with helpers that accept Optional[User] and may prevent use in anonymous/disabled-auth flows.</comment>
<file context>
@@ -275,6 +275,19 @@ def get_connector_credential_pair_from_id_for_user(
+def verify_user_has_access_to_cc_pair(
+ cc_pair_id: int,
+ db_session: Session,
+ user: User,
+ get_editable: bool = True,
+) -> bool:
</file context>
| user: User, | |
| user: User | None, |
* Improve index attempts API * Fix import
Description
^
How Has This Been Tested?
Tested locally
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Shifted the index attempts API to use cc_pair_id instead of connector_id, added a direct access check, and simplified the frontend. This fixes mixed counts/pagination when a connector has multiple CC pairs and removes redundant requests.
Bug Fixes
Refactors