From 31524a3effd710abf8cc146c4b24c013e283fac3 Mon Sep 17 00:00:00 2001 From: pablonyx Date: Wed, 19 Feb 2025 10:46:06 -0800 Subject: [PATCH] add connector validation (#4016) --- ...f4507_remove_inactive_ccpair_status_on_.py | 29 +++++++++ .../background/celery/tasks/indexing/tasks.py | 25 +++++++- .../background/celery/tasks/pruning/tasks.py | 1 + .../onyx/background/indexing/run_indexing.py | 28 +++++++- backend/onyx/connectors/dropbox/connector.py | 27 ++++++++ backend/onyx/connectors/factory.py | 40 ++++++++++++ backend/onyx/connectors/github/connector.py | 47 +++++++++++++- backend/onyx/connectors/interfaces.py | 52 ++++++++++++++- backend/onyx/connectors/notion/connector.py | 63 ++++++++++++++++++ .../onyx/connectors/onyx_jira/connector.py | 37 +++++++++++ backend/onyx/db/credentials.py | 5 ++ backend/onyx/db/enums.py | 1 + backend/onyx/server/documents/cc_pair.py | 23 +++++++ backend/onyx/server/documents/credential.py | 11 ++++ .../common_utils/managers/connector.py | 3 +- .../configuration/search/UpgradingPage.tsx | 13 ++-- .../connector/[ccPairId]/ReIndexButton.tsx | 29 +++++---- web/src/app/admin/connector/[ccPairId]/lib.ts | 21 ++++++ .../app/admin/connector/[ccPairId]/page.tsx | 16 ++++- .../app/admin/connector/[ccPairId]/types.ts | 1 + .../[connector]/AddConnectorPage.tsx | 2 +- .../status/CCPairIndexingStatusTable.tsx | 13 ++++ web/src/components/HoverPopup.tsx | 64 ++++++------------- web/src/components/Status.tsx | 23 +++++-- .../credentials/CredentialSection.tsx | 24 +++++-- web/src/components/ui/badge.tsx | 28 +++++++- web/src/components/ui/button.tsx | 5 +- web/src/lib/ccPair.ts | 20 ++++++ web/src/lib/types.ts | 1 + 29 files changed, 559 insertions(+), 93 deletions(-) create mode 100644 backend/alembic/versions/acaab4ef4507_remove_inactive_ccpair_status_on_.py diff --git a/backend/alembic/versions/acaab4ef4507_remove_inactive_ccpair_status_on_.py b/backend/alembic/versions/acaab4ef4507_remove_inactive_ccpair_status_on_.py new file mode 100644 index 000000000000..4980ef92e173 --- /dev/null +++ b/backend/alembic/versions/acaab4ef4507_remove_inactive_ccpair_status_on_.py @@ -0,0 +1,29 @@ +"""remove inactive ccpair status on downgrade + +Revision ID: acaab4ef4507 +Revises: b7a7eee5aa15 +Create Date: 2025-02-16 18:21:41.330212 + +""" +from alembic import op +from onyx.db.models import ConnectorCredentialPair +from onyx.db.enums import ConnectorCredentialPairStatus +from sqlalchemy import update + +# revision identifiers, used by Alembic. +revision = "acaab4ef4507" +down_revision = "b7a7eee5aa15" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + pass + + +def downgrade() -> None: + op.execute( + update(ConnectorCredentialPair) + .where(ConnectorCredentialPair.status == ConnectorCredentialPairStatus.INVALID) + .values(status=ConnectorCredentialPairStatus.ACTIVE) + ) diff --git a/backend/onyx/background/celery/tasks/indexing/tasks.py b/backend/onyx/background/celery/tasks/indexing/tasks.py index 14db20753dcf..eb4284ff1eb0 100644 --- a/backend/onyx/background/celery/tasks/indexing/tasks.py +++ b/backend/onyx/background/celery/tasks/indexing/tasks.py @@ -48,6 +48,7 @@ from onyx.configs.constants import OnyxCeleryTask from onyx.configs.constants import OnyxRedisConstants from onyx.configs.constants import OnyxRedisLocks from onyx.configs.constants import OnyxRedisSignals +from onyx.connectors.interfaces import ConnectorValidationError from onyx.db.connector import mark_ccpair_with_indexing_trigger from onyx.db.connector_credential_pair import fetch_connector_credential_pairs from onyx.db.connector_credential_pair import get_connector_credential_pair_from_id @@ -107,6 +108,9 @@ class IndexingWatchdogTerminalStatus(str, Enum): "index_attempt_mismatch" # expected index attempt metadata not found in db ) + CONNECTOR_VALIDATION_ERROR = ( + "connector_validation_error" # the connector validation failed + ) CONNECTOR_EXCEPTIONED = "connector_exceptioned" # the connector itself exceptioned WATCHDOG_EXCEPTIONED = "watchdog_exceptioned" # the watchdog exceptioned @@ -127,6 +131,7 @@ class IndexingWatchdogTerminalStatus(str, Enum): _ENUM_TO_CODE: dict[IndexingWatchdogTerminalStatus, int] = { IndexingWatchdogTerminalStatus.PROCESS_SIGNAL_SIGKILL: -9, IndexingWatchdogTerminalStatus.OUT_OF_MEMORY: 137, + IndexingWatchdogTerminalStatus.CONNECTOR_VALIDATION_ERROR: 247, IndexingWatchdogTerminalStatus.BLOCKED_BY_DELETION: 248, IndexingWatchdogTerminalStatus.BLOCKED_BY_STOP_SIGNAL: 249, IndexingWatchdogTerminalStatus.FENCE_NOT_FOUND: 250, @@ -144,6 +149,7 @@ class IndexingWatchdogTerminalStatus(str, Enum): _CODE_TO_ENUM: dict[int, IndexingWatchdogTerminalStatus] = { -9: IndexingWatchdogTerminalStatus.PROCESS_SIGNAL_SIGKILL, 137: IndexingWatchdogTerminalStatus.OUT_OF_MEMORY, + 247: IndexingWatchdogTerminalStatus.CONNECTOR_VALIDATION_ERROR, 248: IndexingWatchdogTerminalStatus.BLOCKED_BY_DELETION, 249: IndexingWatchdogTerminalStatus.BLOCKED_BY_STOP_SIGNAL, 250: IndexingWatchdogTerminalStatus.FENCE_NOT_FOUND, @@ -796,6 +802,15 @@ def connector_indexing_task( # get back the total number of indexed docs and return it n_final_progress = redis_connector_index.get_progress() redis_connector_index.set_generator_complete(HTTPStatus.OK.value) + except ConnectorValidationError: + raise SimpleJobException( + f"Indexing task failed: attempt={index_attempt_id} " + f"tenant={tenant_id} " + f"cc_pair={cc_pair_id} " + f"search_settings={search_settings_id}", + code=IndexingWatchdogTerminalStatus.CONNECTOR_VALIDATION_ERROR.code, + ) + except Exception as e: logger.exception( f"Indexing spawned task failed: attempt={index_attempt_id} " @@ -803,8 +818,8 @@ def connector_indexing_task( f"cc_pair={cc_pair_id} " f"search_settings={search_settings_id}" ) - raise e + finally: if lock.owned(): lock.release() @@ -1064,9 +1079,13 @@ def connector_indexing_proxy_task( ) ) continue - except Exception: + except Exception as e: result.status = IndexingWatchdogTerminalStatus.WATCHDOG_EXCEPTIONED - result.exception_str = traceback.format_exc() + if isinstance(e, ConnectorValidationError): + # No need to expose full stack trace for validation errors + result.exception_str = str(e) + else: + result.exception_str = traceback.format_exc() # handle exit and reporting elapsed = time.monotonic() - start diff --git a/backend/onyx/background/celery/tasks/pruning/tasks.py b/backend/onyx/background/celery/tasks/pruning/tasks.py index ca592fe26907..2cb6e26baeb8 100644 --- a/backend/onyx/background/celery/tasks/pruning/tasks.py +++ b/backend/onyx/background/celery/tasks/pruning/tasks.py @@ -431,6 +431,7 @@ def connector_pruning_generator_task( f"cc_pair={cc_pair_id} " f"connector_source={cc_pair.connector.source}" ) + runnable_connector = instantiate_connector( db_session, cc_pair.connector.source, diff --git a/backend/onyx/background/indexing/run_indexing.py b/backend/onyx/background/indexing/run_indexing.py index 1c7d36333cb7..41823754a942 100644 --- a/backend/onyx/background/indexing/run_indexing.py +++ b/backend/onyx/background/indexing/run_indexing.py @@ -21,6 +21,7 @@ from onyx.configs.constants import DocumentSource from onyx.configs.constants import MilestoneRecordType from onyx.connectors.connector_runner import ConnectorRunner from onyx.connectors.factory import instantiate_connector +from onyx.connectors.interfaces import ConnectorValidationError from onyx.connectors.models import ConnectorCheckpoint from onyx.connectors.models import ConnectorFailure from onyx.connectors.models import Document @@ -86,6 +87,11 @@ def _get_connector_runner( credential=attempt.connector_credential_pair.credential, tenant_id=tenant_id, ) + + # validate the connector settings + + runnable_connector.validate_connector_settings() + except Exception as e: logger.exception(f"Unable to instantiate connector due to {e}") @@ -567,8 +573,28 @@ def _run_indexing( "Connector run exceptioned after elapsed time: " f"{time.monotonic() - start_time} seconds" ) + if isinstance(e, ConnectorValidationError): + # On validation errors during indexing, we want to cancel the indexing attempt + # and mark the CCPair as invalid. This prevents the connector from being + # used in the future until the credentials are updated. + with get_session_with_current_tenant() as db_session_temp: + mark_attempt_canceled( + index_attempt_id, + db_session_temp, + reason=str(e), + ) - if isinstance(e, ConnectorStopSignal): + if ctx.is_primary: + update_connector_credential_pair( + db_session=db_session_temp, + connector_id=ctx.connector_id, + credential_id=ctx.credential_id, + status=ConnectorCredentialPairStatus.INVALID, + ) + memory_tracer.stop() + raise e + + elif isinstance(e, ConnectorStopSignal): with get_session_with_current_tenant() as db_session_temp: mark_attempt_canceled( index_attempt_id, diff --git a/backend/onyx/connectors/dropbox/connector.py b/backend/onyx/connectors/dropbox/connector.py index 1a206068ab76..b26be1477cb5 100644 --- a/backend/onyx/connectors/dropbox/connector.py +++ b/backend/onyx/connectors/dropbox/connector.py @@ -4,12 +4,16 @@ from typing import Any from dropbox import Dropbox # type: ignore from dropbox.exceptions import ApiError # type:ignore +from dropbox.exceptions import AuthError # type:ignore from dropbox.files import FileMetadata # type:ignore from dropbox.files import FolderMetadata # type:ignore from onyx.configs.app_configs import INDEX_BATCH_SIZE from onyx.configs.constants import DocumentSource +from onyx.connectors.interfaces import ConnectorValidationError +from onyx.connectors.interfaces import CredentialInvalidError from onyx.connectors.interfaces import GenerateDocumentsOutput +from onyx.connectors.interfaces import InsufficientPermissionsError from onyx.connectors.interfaces import LoadConnector from onyx.connectors.interfaces import PollConnector from onyx.connectors.interfaces import SecondsSinceUnixEpoch @@ -141,6 +145,29 @@ class DropboxConnector(LoadConnector, PollConnector): return None + def validate_connector_settings(self) -> None: + if self.dropbox_client is None: + raise ConnectorMissingCredentialError("Dropbox credentials not loaded.") + + try: + self.dropbox_client.files_list_folder(path="", limit=1) + except AuthError as e: + logger.exception("Failed to validate Dropbox credentials") + raise CredentialInvalidError(f"Dropbox credential is invalid: {e.error}") + except ApiError as e: + if ( + e.error is not None + and "insufficient_permissions" in str(e.error).lower() + ): + raise InsufficientPermissionsError( + "Your Dropbox token does not have sufficient permissions." + ) + raise ConnectorValidationError( + f"Unexpected Dropbox error during validation: {e.user_message_text or e}" + ) + except Exception as e: + raise Exception(f"Unexpected error during Dropbox settings validation: {e}") + if __name__ == "__main__": import os diff --git a/backend/onyx/connectors/factory.py b/backend/onyx/connectors/factory.py index 28a67e6a5e60..ddf68fb2dea7 100644 --- a/backend/onyx/connectors/factory.py +++ b/backend/onyx/connectors/factory.py @@ -31,6 +31,7 @@ from onyx.connectors.guru.connector import GuruConnector from onyx.connectors.hubspot.connector import HubSpotConnector from onyx.connectors.interfaces import BaseConnector from onyx.connectors.interfaces import CheckpointConnector +from onyx.connectors.interfaces import ConnectorValidationError from onyx.connectors.interfaces import EventConnector from onyx.connectors.interfaces import LoadConnector from onyx.connectors.interfaces import PollConnector @@ -52,8 +53,11 @@ from onyx.connectors.wikipedia.connector import WikipediaConnector from onyx.connectors.xenforo.connector import XenforoConnector from onyx.connectors.zendesk.connector import ZendeskConnector from onyx.connectors.zulip.connector import ZulipConnector +from onyx.db.connector import fetch_connector_by_id from onyx.db.credentials import backend_update_credential_json +from onyx.db.credentials import fetch_credential_by_id_for_user from onyx.db.models import Credential +from onyx.db.models import User class ConnectorMissingException(Exception): @@ -174,3 +178,39 @@ def instantiate_connector( backend_update_credential_json(credential, new_credentials, db_session) return connector + + +def validate_ccpair_for_user( + connector_id: int, + credential_id: int, + db_session: Session, + user: User | None, + tenant_id: str | None, +) -> None: + # Validate the connector settings + connector = fetch_connector_by_id(connector_id, db_session) + credential = fetch_credential_by_id_for_user( + credential_id, + user, + db_session, + get_editable=False, + ) + if not credential: + raise ValueError("Credential not found") + if not connector: + raise ValueError("Connector not found") + + try: + runnable_connector = instantiate_connector( + db_session=db_session, + source=connector.source, + input_type=connector.input_type, + connector_specific_config=connector.connector_specific_config, + credential=credential, + tenant_id=tenant_id, + ) + except Exception as e: + error_msg = f"Unexpected error creating connector: {e}" + raise ConnectorValidationError(error_msg) + + runnable_connector.validate_connector_settings() diff --git a/backend/onyx/connectors/github/connector.py b/backend/onyx/connectors/github/connector.py index d466e9de14f0..14092598267b 100644 --- a/backend/onyx/connectors/github/connector.py +++ b/backend/onyx/connectors/github/connector.py @@ -9,6 +9,7 @@ from typing import cast from github import Github from github import RateLimitExceededException from github import Repository +from github.GithubException import GithubException from github.Issue import Issue from github.PaginatedList import PaginatedList from github.PullRequest import PullRequest @@ -16,7 +17,10 @@ from github.PullRequest import PullRequest from onyx.configs.app_configs import GITHUB_CONNECTOR_BASE_URL from onyx.configs.app_configs import INDEX_BATCH_SIZE from onyx.configs.constants import DocumentSource +from onyx.connectors.interfaces import ConnectorValidationError +from onyx.connectors.interfaces import CredentialExpiredError from onyx.connectors.interfaces import GenerateDocumentsOutput +from onyx.connectors.interfaces import InsufficientPermissionsError from onyx.connectors.interfaces import LoadConnector from onyx.connectors.interfaces import PollConnector from onyx.connectors.interfaces import SecondsSinceUnixEpoch @@ -26,7 +30,6 @@ from onyx.connectors.models import Section from onyx.utils.batching import batch_generator from onyx.utils.logger import setup_logger - logger = setup_logger() @@ -226,6 +229,48 @@ class GithubConnector(LoadConnector, PollConnector): return self._fetch_from_github(adjusted_start_datetime, end_datetime) + def validate_connector_settings(self) -> None: + if self.github_client is None: + raise ConnectorMissingCredentialError("GitHub credentials not loaded.") + + if not self.repo_owner or not self.repo_name: + raise ConnectorValidationError( + "Invalid connector settings: 'repo_owner' and 'repo_name' must be provided." + ) + + try: + test_repo = self.github_client.get_repo( + f"{self.repo_owner}/{self.repo_name}" + ) + test_repo.get_contents("") + + except RateLimitExceededException: + raise ConnectorValidationError( + "Validation failed due to GitHub rate-limits being exceeded. Please try again later." + ) + + except GithubException as e: + if e.status == 401: + raise CredentialExpiredError( + "GitHub credential appears to be invalid or expired (HTTP 401)." + ) + elif e.status == 403: + raise InsufficientPermissionsError( + "Your GitHub token does not have sufficient permissions for this repository (HTTP 403)." + ) + elif e.status == 404: + raise ConnectorValidationError( + f"GitHub repository not found with name: {self.repo_owner}/{self.repo_name}" + ) + else: + raise ConnectorValidationError( + f"Unexpected GitHub error (status={e.status}): {e.data}" + ) + except Exception as exc: + raise Exception( + f"Unexpected error during GitHub settings validation: {exc}" + ) + if __name__ == "__main__": import os diff --git a/backend/onyx/connectors/interfaces.py b/backend/onyx/connectors/interfaces.py index e92b077ce328..e49065a9bd8f 100644 --- a/backend/onyx/connectors/interfaces.py +++ b/backend/onyx/connectors/interfaces.py @@ -12,7 +12,6 @@ from onyx.connectors.models import Document from onyx.connectors.models import SlimDocument from onyx.indexing.indexing_heartbeat import IndexingHeartbeatInterface - SecondsSinceUnixEpoch = float GenerateDocumentsOutput = Iterator[list[Document]] @@ -45,6 +44,14 @@ class BaseConnector(abc.ABC): raise RuntimeError(custom_parser_req_msg) return metadata_lines + def validate_connector_settings(self) -> None: + """ + Override this if your connector needs to validate credentials or settings. + Raise an exception if invalid, otherwise do nothing. + + Default is a no-op (always successful). + """ + # Large set update or reindex, generally pulling a complete state or from a savestate file class LoadConnector(BaseConnector): @@ -139,3 +146,46 @@ class CheckpointConnector(BaseConnector): ``` """ raise NotImplementedError + + +class ConnectorValidationError(Exception): + """General exception for connector validation errors.""" + + def __init__(self, message: str): + self.message = message + super().__init__(self.message) + + +class UnexpectedError(Exception): + """Raised when an unexpected error occurs during connector validation. + + Unexpected errors don't necessarily mean the credential is invalid, + but rather that there was an error during the validation process + or we encountered a currently unhandled error case. + """ + + def __init__(self, message: str = "Unexpected error during connector validation"): + super().__init__(message) + + +class CredentialInvalidError(ConnectorValidationError): + """Raised when a connector's credential is invalid.""" + + def __init__(self, message: str = "Credential is invalid"): + super().__init__(message) + + +class CredentialExpiredError(ConnectorValidationError): + """Raised when a connector's credential is expired.""" + + def __init__(self, message: str = "Credential has expired"): + super().__init__(message) + + +class InsufficientPermissionsError(ConnectorValidationError): + """Raised when the credential does not have sufficient API permissions.""" + + def __init__( + self, message: str = "Insufficient permissions for the requested operation" + ): + super().__init__(message) diff --git a/backend/onyx/connectors/notion/connector.py b/backend/onyx/connectors/notion/connector.py index ed753486f4e0..70f1f001a97f 100644 --- a/backend/onyx/connectors/notion/connector.py +++ b/backend/onyx/connectors/notion/connector.py @@ -7,6 +7,7 @@ from datetime import timezone from typing import Any from typing import Optional +import requests from retry import retry from onyx.configs.app_configs import INDEX_BATCH_SIZE @@ -15,10 +16,14 @@ from onyx.configs.constants import DocumentSource from onyx.connectors.cross_connector_utils.rate_limit_wrapper import ( rl_requests, ) +from onyx.connectors.interfaces import ConnectorValidationError +from onyx.connectors.interfaces import CredentialExpiredError from onyx.connectors.interfaces import GenerateDocumentsOutput +from onyx.connectors.interfaces import InsufficientPermissionsError from onyx.connectors.interfaces import LoadConnector from onyx.connectors.interfaces import PollConnector from onyx.connectors.interfaces import SecondsSinceUnixEpoch +from onyx.connectors.models import ConnectorMissingCredentialError from onyx.connectors.models import Document from onyx.connectors.models import Section from onyx.utils.batching import batch_generator @@ -616,6 +621,64 @@ class NotionConnector(LoadConnector, PollConnector): else: break + def validate_connector_settings(self) -> None: + if not self.headers.get("Authorization"): + raise ConnectorMissingCredentialError("Notion credentials not loaded.") + + try: + # We'll do a minimal search call (page_size=1) to confirm accessibility + if self.root_page_id: + # If root_page_id is set, fetch the specific page + res = rl_requests.get( + f"https://api.notion.com/v1/pages/{self.root_page_id}", + headers=self.headers, + timeout=_NOTION_CALL_TIMEOUT, + ) + else: + # If root_page_id is not set, perform a minimal search + test_query = { + "filter": {"property": "object", "value": "page"}, + "page_size": 1, + } + res = rl_requests.post( + "https://api.notion.com/v1/search", + headers=self.headers, + json=test_query, + timeout=_NOTION_CALL_TIMEOUT, + ) + res.raise_for_status() + + except requests.exceptions.HTTPError as http_err: + status_code = http_err.response.status_code if http_err.response else None + + if status_code == 401: + raise CredentialExpiredError( + "Notion credential appears to be invalid or expired (HTTP 401)." + ) + elif status_code == 403: + raise InsufficientPermissionsError( + "Your Notion token does not have sufficient permissions (HTTP 403)." + ) + elif status_code == 404: + # Typically means resource not found or not shared. Could be root_page_id is invalid. + raise ConnectorValidationError( + "Notion resource not found or not shared with the integration (HTTP 404)." + ) + elif status_code == 429: + raise ConnectorValidationError( + "Validation failed due to Notion rate-limits being exceeded (HTTP 429). " + "Please try again later." + ) + else: + raise Exception( + f"Unexpected Notion HTTP error (status={status_code}): {http_err}" + ) from http_err + + except Exception as exc: + raise Exception( + f"Unexpected error during Notion settings validation: {exc}" + ) + if __name__ == "__main__": import os diff --git a/backend/onyx/connectors/onyx_jira/connector.py b/backend/onyx/connectors/onyx_jira/connector.py index 082170471660..98c56808738d 100644 --- a/backend/onyx/connectors/onyx_jira/connector.py +++ b/backend/onyx/connectors/onyx_jira/connector.py @@ -12,8 +12,11 @@ from onyx.configs.app_configs import JIRA_CONNECTOR_LABELS_TO_SKIP from onyx.configs.app_configs import JIRA_CONNECTOR_MAX_TICKET_SIZE from onyx.configs.constants import DocumentSource from onyx.connectors.cross_connector_utils.miscellaneous_utils import time_str_to_utc +from onyx.connectors.interfaces import ConnectorValidationError +from onyx.connectors.interfaces import CredentialExpiredError from onyx.connectors.interfaces import GenerateDocumentsOutput from onyx.connectors.interfaces import GenerateSlimDocumentOutput +from onyx.connectors.interfaces import InsufficientPermissionsError from onyx.connectors.interfaces import LoadConnector from onyx.connectors.interfaces import PollConnector from onyx.connectors.interfaces import SecondsSinceUnixEpoch @@ -272,6 +275,40 @@ class JiraConnector(LoadConnector, PollConnector, SlimConnector): yield slim_doc_batch + def validate_connector_settings(self) -> None: + if self._jira_client is None: + raise ConnectorMissingCredentialError("Jira") + + if not self._jira_project: + raise ConnectorValidationError( + "Invalid connector settings: 'jira_project' must be provided." + ) + + try: + self.jira_client.project(self._jira_project) + + except Exception as e: + status_code = getattr(e, "status_code", None) + + if status_code == 401: + raise CredentialExpiredError( + "Jira credential appears to be expired or invalid (HTTP 401)." + ) + elif status_code == 403: + raise InsufficientPermissionsError( + "Your Jira token does not have sufficient permissions for this project (HTTP 403)." + ) + elif status_code == 404: + raise ConnectorValidationError( + f"Jira project not found with key: {self._jira_project}" + ) + elif status_code == 429: + raise ConnectorValidationError( + "Validation failed due to Jira rate-limits being exceeded. Please try again later." + ) + else: + raise Exception(f"Unexpected Jira error during validation: {e}") + if __name__ == "__main__": import os diff --git a/backend/onyx/db/credentials.py b/backend/onyx/db/credentials.py index ca82ceb1624b..1e4c7673cce3 100644 --- a/backend/onyx/db/credentials.py +++ b/backend/onyx/db/credentials.py @@ -14,6 +14,7 @@ from onyx.configs.constants import DocumentSource from onyx.connectors.google_utils.shared_constants import ( DB_CREDENTIALS_DICT_SERVICE_ACCOUNT_KEY, ) +from onyx.db.enums import ConnectorCredentialPairStatus from onyx.db.models import ConnectorCredentialPair from onyx.db.models import Credential from onyx.db.models import Credential__UserGroup @@ -245,6 +246,10 @@ def swap_credentials_connector( existing_pair.credential_id = new_credential_id existing_pair.credential = new_credential + # Update ccpair status if it's in INVALID state + if existing_pair.status == ConnectorCredentialPairStatus.INVALID: + existing_pair.status = ConnectorCredentialPairStatus.ACTIVE + # Commit the changes db_session.commit() diff --git a/backend/onyx/db/enums.py b/backend/onyx/db/enums.py index 7a7943a014c5..329ee35fcd68 100644 --- a/backend/onyx/db/enums.py +++ b/backend/onyx/db/enums.py @@ -73,6 +73,7 @@ class ConnectorCredentialPairStatus(str, PyEnum): ACTIVE = "ACTIVE" PAUSED = "PAUSED" DELETING = "DELETING" + INVALID = "INVALID" def is_active(self) -> bool: return self == ConnectorCredentialPairStatus.ACTIVE diff --git a/backend/onyx/server/documents/cc_pair.py b/backend/onyx/server/documents/cc_pair.py index 9a61367be98c..dfabf531f2f6 100644 --- a/backend/onyx/server/documents/cc_pair.py +++ b/backend/onyx/server/documents/cc_pair.py @@ -25,6 +25,9 @@ from onyx.background.celery.versioned_apps.primary import app as primary_app from onyx.background.indexing.models import IndexAttemptErrorPydantic from onyx.configs.constants import OnyxCeleryPriority from onyx.configs.constants import OnyxCeleryTask +from onyx.connectors.factory import validate_ccpair_for_user +from onyx.connectors.interfaces import ConnectorValidationError +from onyx.db.connector import delete_connector from onyx.db.connector_credential_pair import add_credential_to_connector from onyx.db.connector_credential_pair import ( get_connector_credential_pair_from_id_for_user, @@ -617,6 +620,10 @@ def associate_credential_to_connector( ) try: + validate_ccpair_for_user( + connector_id, credential_id, db_session, user, tenant_id + ) + response = add_credential_to_connector( db_session=db_session, user=user, @@ -641,10 +648,26 @@ def associate_credential_to_connector( ) return response + + except ConnectorValidationError as e: + # If validation fails, delete the connector and commit the changes + # Ensures we don't leave invalid connectors in the database + # NOTE: consensus is that it makes sense to unify connector and ccpair creation flows + # which would rid us of needing to handle cases like these + delete_connector(db_session, connector_id) + db_session.commit() + + raise HTTPException( + status_code=400, detail="Connector validation error: " + str(e) + ) + except IntegrityError as e: logger.error(f"IntegrityError: {e}") raise HTTPException(status_code=400, detail="Name must be unique") + except Exception: + raise HTTPException(status_code=500, detail="Unexpected error") + @router.delete("/connector/{connector_id}/credential/{credential_id}") def dissociate_credential_from_connector( diff --git a/backend/onyx/server/documents/credential.py b/backend/onyx/server/documents/credential.py index 61a84792e99a..54769632d955 100644 --- a/backend/onyx/server/documents/credential.py +++ b/backend/onyx/server/documents/credential.py @@ -7,6 +7,7 @@ from sqlalchemy.orm import Session from onyx.auth.users import current_admin_user from onyx.auth.users import current_curator_or_admin_user from onyx.auth.users import current_user +from onyx.connectors.factory import validate_ccpair_for_user from onyx.db.credentials import alter_credential from onyx.db.credentials import cleanup_gmail_credentials from onyx.db.credentials import create_credential @@ -17,6 +18,7 @@ from onyx.db.credentials import fetch_credentials_by_source_for_user from onyx.db.credentials import fetch_credentials_for_user from onyx.db.credentials import swap_credentials_connector from onyx.db.credentials import update_credential +from onyx.db.engine import get_current_tenant_id from onyx.db.engine import get_session from onyx.db.models import DocumentSource from onyx.db.models import User @@ -98,7 +100,16 @@ def swap_credentials_for_connector( credential_swap_req: CredentialSwapRequest, user: User | None = Depends(current_user), db_session: Session = Depends(get_session), + tenant_id: str | None = Depends(get_current_tenant_id), ) -> StatusResponse: + validate_ccpair_for_user( + credential_swap_req.connector_id, + credential_swap_req.new_credential_id, + db_session, + user, + tenant_id, + ) + connector_credential_pair = swap_credentials_connector( new_credential_id=credential_swap_req.new_credential_id, connector_id=credential_swap_req.connector_id, diff --git a/backend/tests/integration/common_utils/managers/connector.py b/backend/tests/integration/common_utils/managers/connector.py index 2bcda485107a..04dd37c2adfa 100644 --- a/backend/tests/integration/common_utils/managers/connector.py +++ b/backend/tests/integration/common_utils/managers/connector.py @@ -30,7 +30,8 @@ class ConnectorManager: name=name, source=source, input_type=input_type, - connector_specific_config=connector_specific_config or {}, + connector_specific_config=connector_specific_config + or {"file_locations": []}, access_type=access_type, groups=groups or [], ) diff --git a/web/src/app/admin/configuration/search/UpgradingPage.tsx b/web/src/app/admin/configuration/search/UpgradingPage.tsx index 98653c4aa42b..87290bb462eb 100644 --- a/web/src/app/admin/configuration/search/UpgradingPage.tsx +++ b/web/src/app/admin/configuration/search/UpgradingPage.tsx @@ -67,12 +67,13 @@ export default function UpgradingPage({ }; const statusOrder: Record = useMemo( () => ({ - failed: 0, - canceled: 1, - completed_with_errors: 2, - not_started: 3, - in_progress: 4, - success: 5, + invalid: 0, + failed: 1, + canceled: 2, + completed_with_errors: 3, + not_started: 4, + in_progress: 5, + success: 6, }), [] ); diff --git a/web/src/app/admin/connector/[ccPairId]/ReIndexButton.tsx b/web/src/app/admin/connector/[ccPairId]/ReIndexButton.tsx index bd647adcd8d1..4848d425bdd3 100644 --- a/web/src/app/admin/connector/[ccPairId]/ReIndexButton.tsx +++ b/web/src/app/admin/connector/[ccPairId]/ReIndexButton.tsx @@ -4,9 +4,14 @@ import { PopupSpec, usePopup } from "@/components/admin/connectors/Popup"; import { Button } from "@/components/ui/button"; import Text from "@/components/ui/text"; import { triggerIndexing } from "./lib"; +import { mutate } from "swr"; +import { buildCCPairInfoUrl, getTooltipMessage } from "./lib"; import { useState } from "react"; import { Modal } from "@/components/Modal"; import { Separator } from "@/components/ui/separator"; +import { ConnectorCredentialPairStatus } from "./types"; +import { CCPairStatus } from "@/components/Status"; +import { getCCPairStatusMessage } from "@/lib/ccPair"; function ReIndexPopup({ connectorId, @@ -83,16 +88,16 @@ export function ReIndexButton({ ccPairId, connectorId, credentialId, - isDisabled, isIndexing, - isDeleting, + isDisabled, + ccPairStatus, }: { ccPairId: number; connectorId: number; credentialId: number; - isDisabled: boolean; isIndexing: boolean; - isDeleting: boolean; + isDisabled: boolean; + ccPairStatus: ConnectorCredentialPairStatus; }) { const { popup, setPopup } = usePopup(); const [reIndexPopupVisible, setReIndexPopupVisible] = useState(false); @@ -115,18 +120,14 @@ export function ReIndexButton({ onClick={() => { setReIndexPopupVisible(true); }} - disabled={isDisabled || isDeleting} - tooltip={ - isDeleting - ? "Cannot index while connector is deleting" - : isIndexing - ? "Indexing is already in progress" - : isDisabled - ? "Connector must be re-enabled before indexing" - : undefined + disabled={ + isDisabled || + ccPairStatus == ConnectorCredentialPairStatus.DELETING || + ccPairStatus == ConnectorCredentialPairStatus.PAUSED } + tooltip={getCCPairStatusMessage(isDisabled, isIndexing, ccPairStatus)} > - Index + Re-Index ); diff --git a/web/src/app/admin/connector/[ccPairId]/lib.ts b/web/src/app/admin/connector/[ccPairId]/lib.ts index 89372eaf7a2d..3f392be62bf9 100644 --- a/web/src/app/admin/connector/[ccPairId]/lib.ts +++ b/web/src/app/admin/connector/[ccPairId]/lib.ts @@ -40,3 +40,24 @@ export async function triggerIndexing( } mutate(buildCCPairInfoUrl(ccPairId)); } + +export function getTooltipMessage( + isInvalid: boolean, + isDeleting: boolean, + isIndexing: boolean, + isDisabled: boolean +): string | undefined { + if (isInvalid) { + return "Connector is in an invalid state. Please update the credentials or configuration before re-indexing."; + } + if (isDeleting) { + return "Cannot index while connector is deleting"; + } + if (isIndexing) { + return "Indexing is already in progress"; + } + if (isDisabled) { + return "Connector must be re-enabled before indexing"; + } + return undefined; +} diff --git a/web/src/app/admin/connector/[ccPairId]/page.tsx b/web/src/app/admin/connector/[ccPairId]/page.tsx index fb84d31d017f..b21949f129df 100644 --- a/web/src/app/admin/connector/[ccPairId]/page.tsx +++ b/web/src/app/admin/connector/[ccPairId]/page.tsx @@ -43,6 +43,7 @@ import IndexAttemptErrorsModal from "./IndexAttemptErrorsModal"; import usePaginatedFetch from "@/hooks/usePaginatedFetch"; import { IndexAttemptSnapshot } from "@/lib/types"; import { Spinner } from "@/components/Spinner"; +import { Callout } from "@/components/ui/callout"; // synchronize these validations with the SQLAlchemy connector class until we have a // centralized schema for both frontend and backend @@ -363,6 +364,7 @@ function Main({ ccPairId }: { ccPairId: number }) {
{!isDeleting && } @@ -379,8 +380,7 @@ function Main({ ccPairId }: { ccPairId: number }) {
Creator:{" "} @@ -424,6 +424,16 @@ function Main({ ccPairId }: { ccPairId: number }) { /> )} + + {ccPair.status === ConnectorCredentialPairStatus.INVALID && ( +
+ + This connector is in an invalid state. Please update your + credentials or create a new connector before re-indexing. + +
+ )} + ); + } else if ( + ccPairsIndexingStatus.cc_pair_status === + ConnectorCredentialPairStatus.INVALID + ) { + return ( + + Invalid + + ); } // ACTIVE case diff --git a/web/src/components/HoverPopup.tsx b/web/src/components/HoverPopup.tsx index 6fac81cd2bdc..c704865ba3b2 100644 --- a/web/src/components/HoverPopup.tsx +++ b/web/src/components/HoverPopup.tsx @@ -8,58 +8,32 @@ interface HoverPopupProps { style?: "basic" | "dark"; } +import { + Tooltip, + TooltipContent, + TooltipProvider, + TooltipTrigger, +} from "@/components/ui/tooltip"; + export const HoverPopup = ({ mainContent, popupContent, classNameModifications, direction = "bottom", - style = "basic", }: HoverPopupProps) => { - const [hovered, setHovered] = useState(false); - - let popupDirectionClass; - let popupStyle = {}; - switch (direction) { - case "left": - popupDirectionClass = "top-0 left-0 transform"; - popupStyle = { transform: "translateX(calc(-100% - 5px))" }; - break; - case "left-top": - popupDirectionClass = "bottom-0 left-0"; - popupStyle = { transform: "translate(calc(-100% - 5px), 0)" }; - break; - case "bottom": - popupDirectionClass = "top-0 left-0 mt-6 pt-2"; - break; - case "top": - popupDirectionClass = "top-0 left-0 translate-y-[-100%] pb-2"; - break; - } - return ( -
{ - setHovered(true); - }} - onMouseLeave={() => setHovered(false)} - > - {hovered && ( -
+ + +
{mainContent}
+ + -
- {popupContent} -
-
- )} -
{mainContent}
-
+ {popupContent} + + + ); }; diff --git a/web/src/components/Status.tsx b/web/src/components/Status.tsx index ed57bf2e7d4d..9ac38534aa2b 100644 --- a/web/src/components/Status.tsx +++ b/web/src/components/Status.tsx @@ -10,6 +10,7 @@ import { FiPauseCircle, } from "react-icons/fi"; import { HoverPopup } from "./HoverPopup"; +import { ConnectorCredentialPairStatus } from "@/app/admin/connector/[ccPairId]/types"; export function IndexAttemptStatus({ status, @@ -70,6 +71,12 @@ export function IndexAttemptStatus({ Canceled ); + } else if (status === "invalid") { + badge = ( + + Invalid + + ); } else { badge = ( @@ -83,29 +90,33 @@ export function IndexAttemptStatus({ export function CCPairStatus({ status, - disabled, - isDeleting, + ccPairStatus, size = "md", }: { status: ValidStatuses; - disabled: boolean; - isDeleting: boolean; + ccPairStatus: ConnectorCredentialPairStatus; size?: "xs" | "sm" | "md" | "lg"; }) { let badge; - if (isDeleting) { + if (ccPairStatus == ConnectorCredentialPairStatus.DELETING) { badge = ( Deleting ); - } else if (disabled) { + } else if (ccPairStatus == ConnectorCredentialPairStatus.PAUSED) { badge = ( Paused ); + } else if (ccPairStatus == ConnectorCredentialPairStatus.INVALID) { + badge = ( + + Invalid + + ); } else if (status === "failed") { badge = ( diff --git a/web/src/components/credentials/CredentialSection.tsx b/web/src/components/credentials/CredentialSection.tsx index 96e68ee54766..3f137a66077f 100644 --- a/web/src/components/credentials/CredentialSection.tsx +++ b/web/src/components/credentials/CredentialSection.tsx @@ -79,14 +79,24 @@ export default function CredentialSection({ selectedCredential: Credential, connectorId: number ) => { - await swapCredential(selectedCredential.id, connectorId); - mutate(buildSimilarCredentialInfoURL(sourceType)); - refresh(); + const response = await swapCredential(selectedCredential.id, connectorId); + if (response.ok) { + mutate(buildSimilarCredentialInfoURL(sourceType)); + refresh(); - setPopup({ - message: "Swapped credential succesfully!", - type: "success", - }); + setPopup({ + message: "Swapped credential successfully!", + type: "success", + }); + } else { + const errorData = await response.json(); + setPopup({ + message: `Issue swapping credential: ${ + errorData.detail || errorData.message || "Unknown error" + }`, + type: "error", + }); + } }; const onUpdateCredential = async ( diff --git a/web/src/components/ui/badge.tsx b/web/src/components/ui/badge.tsx index c92ef58ea01b..d4ffde5553ac 100644 --- a/web/src/components/ui/badge.tsx +++ b/web/src/components/ui/badge.tsx @@ -1,6 +1,11 @@ import * as React from "react"; import { cva, type VariantProps } from "class-variance-authority"; - +import { + Tooltip, + TooltipContent, + TooltipProvider, + TooltipTrigger, +} from "@/components/ui/tooltip"; import { cn } from "@/lib/utils"; const badgeVariants = cva( @@ -8,6 +13,8 @@ const badgeVariants = cva( { variants: { variant: { + invalid: + "border-orange-200 bg-orange-50 text-orange-600 dark:border-orange-700 dark:bg-orange-900 dark:text-orange-50", outline: "border-neutral-200 bg-neutral-50 text-neutral-600 dark:border-neutral-700 dark:bg-neutral-900 dark:text-neutral-50", purple: @@ -57,11 +64,13 @@ function Badge({ icon: Icon, size = "sm", circle, + tooltip, ...props }: BadgeProps & { icon?: React.ElementType; size?: "sm" | "md" | "xs"; circle?: boolean; + tooltip?: string; }) { const sizeClasses = { sm: "px-2.5 py-0.5 text-xs", @@ -69,7 +78,7 @@ function Badge({ xs: "px-1.5 py-0.25 text-[.5rem]", }; - return ( + const BadgeContent = (
{props.children}
); + + if (tooltip) { + return ( + + + {BadgeContent} + +

{tooltip}

+ + + + ); + } + + return BadgeContent; } export { Badge, badgeVariants }; diff --git a/web/src/components/ui/button.tsx b/web/src/components/ui/button.tsx index 6a6d46630d09..fa8fd9d6f9a4 100644 --- a/web/src/components/ui/button.tsx +++ b/web/src/components/ui/button.tsx @@ -88,7 +88,6 @@ export interface ButtonProps tooltip?: string; reverse?: boolean; } - const Button = React.forwardRef( ( { @@ -124,7 +123,9 @@ const Button = React.forwardRef( return ( - {button} + +
{button}
+

{tooltip}

 diff --git a/web/src/lib/ccPair.ts b/web/src/lib/ccPair.ts index 3a3a75e0ff8e..58ea470b3fd7 100644 --- a/web/src/lib/ccPair.ts +++ b/web/src/lib/ccPair.ts @@ -46,3 +46,23 @@ export async function setCCPairStatus( }); } } + +export const getCCPairStatusMessage = ( + isDisabled: boolean, + isIndexing: boolean, + ccPairStatus: ConnectorCredentialPairStatus +) => { + if (ccPairStatus === ConnectorCredentialPairStatus.INVALID) { + return "Connector is in an invalid state. Please update the credentials or configuration before re-indexing."; + } + if (ccPairStatus === ConnectorCredentialPairStatus.DELETING) { + return "Cannot index while connector is deleting"; + } + if (isIndexing) { + return "Indexing is already in progress"; + } + if (isDisabled) { + return "Connector must be re-enabled before indexing"; + } + return undefined; +}; diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts index 052fb7e32034..733bbfa028c5 100644 --- a/web/src/lib/types.ts +++ b/web/src/lib/types.ts @@ -98,6 +98,7 @@ export type ValidInputTypes = | "event" | "slim_retrieval"; export type ValidStatuses = + | "invalid" | "success" | "completed_with_errors" | "canceled"