-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integration tests #2256
Integration tests #2256
Changes from 24 commits
a72c4ce
588eaa3
7dbaa15
6cd352c
5b39499
c7129be
7d4054a
a74728d
d8e68ce
c26a0ac
20dff5a
a60c850
283d9cc
8651f0b
c46f80d
f222cff
30da5e6
f8b139d
bfcda3e
30feeae
efd9120
622a229
7abec59
aedb53b
724402c
51df90a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -334,9 +334,13 @@ def add_credential_to_connector( | |
raise HTTPException(status_code=404, detail="Connector does not exist") | ||
|
||
if credential is None: | ||
error_msg = ( | ||
f"Credential {credential_id} does not exist or does not belong to user" | ||
) | ||
logger.error(error_msg) | ||
raise HTTPException( | ||
status_code=401, | ||
detail="Credential does not exist or does not belong to user", | ||
detail=error_msg, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to look into long term fix for bubbling up failed api calls |
||
|
||
existing_association = ( | ||
|
@@ -350,7 +354,7 @@ def add_credential_to_connector( | |
if existing_association is not None: | ||
return StatusResponse( | ||
success=False, | ||
message=f"Connector already has Credential {credential_id}", | ||
message=f"Connector {connector_id} already has Credential {credential_id}", | ||
data=connector_id, | ||
) | ||
|
||
|
@@ -374,8 +378,8 @@ def add_credential_to_connector( | |
db_session.commit() | ||
|
||
return StatusResponse( | ||
success=False, | ||
message=f"Connector already has Credential {credential_id}", | ||
success=True, | ||
message=f"Creating new association between Connector {connector_id} and Credential {credential_id}", | ||
data=association.id, | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
from fastapi import APIRouter | ||
from fastapi import Depends | ||
from fastapi import HTTPException | ||
from pydantic import BaseModel | ||
from sqlalchemy.exc import IntegrityError | ||
from sqlalchemy.orm import Session | ||
|
||
|
@@ -21,12 +20,13 @@ | |
from danswer.db.index_attempt import cancel_indexing_attempts_past_model | ||
from danswer.db.index_attempt import get_index_attempts_for_connector | ||
from danswer.db.models import User | ||
from danswer.db.models import UserRole | ||
from danswer.server.documents.models import CCPairFullInfo | ||
from danswer.server.documents.models import CCStatusUpdateRequest | ||
from danswer.server.documents.models import ConnectorCredentialPairIdentifier | ||
from danswer.server.documents.models import ConnectorCredentialPairMetadata | ||
from danswer.server.models import StatusResponse | ||
from danswer.utils.logger import setup_logger | ||
from ee.danswer.db.user_group import validate_user_creation_permissions | ||
|
||
logger = setup_logger() | ||
|
||
|
@@ -84,10 +84,6 @@ def get_cc_pair_full_info( | |
) | ||
|
||
|
||
class CCStatusUpdateRequest(BaseModel): | ||
status: ConnectorCredentialPairStatus | ||
|
||
|
||
@router.put("/admin/cc-pair/{cc_pair_id}/status") | ||
def update_cc_pair_status( | ||
cc_pair_id: int, | ||
|
@@ -157,11 +153,12 @@ def associate_credential_to_connector( | |
user: User | None = Depends(current_curator_or_admin_user), | ||
db_session: Session = Depends(get_session), | ||
) -> StatusResponse[int]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. used a more robust existing function to check |
||
if user and user.role != UserRole.ADMIN and metadata.is_public: | ||
raise HTTPException( | ||
status_code=400, | ||
detail="Public connections cannot be created by non-admin users", | ||
) | ||
validate_user_creation_permissions( | ||
db_session=db_session, | ||
user=user, | ||
target_group_ids=metadata.groups, | ||
object_is_public=metadata.is_public, | ||
) | ||
|
||
try: | ||
response = add_credential_to_connector( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,7 +75,6 @@ | |
from danswer.file_store.file_store import get_default_file_store | ||
from danswer.server.documents.models import AuthStatus | ||
from danswer.server.documents.models import AuthUrl | ||
from danswer.server.documents.models import ConnectorBase | ||
from danswer.server.documents.models import ConnectorCredentialPairIdentifier | ||
from danswer.server.documents.models import ConnectorIndexingStatus | ||
from danswer.server.documents.models import ConnectorSnapshot | ||
|
@@ -93,6 +92,7 @@ | |
from danswer.server.documents.models import RunConnectorRequest | ||
from danswer.server.models import StatusResponse | ||
from danswer.utils.logger import setup_logger | ||
from ee.danswer.db.user_group import validate_user_creation_permissions | ||
|
||
logger = setup_logger() | ||
|
||
|
@@ -514,35 +514,6 @@ def _validate_connector_allowed(source: DocumentSource) -> None: | |
) | ||
|
||
|
||
def _check_connector_permissions( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. used a more robust existing function |
||
connector_data: ConnectorUpdateRequest, user: User | None | ||
) -> ConnectorBase: | ||
""" | ||
This is not a proper permission check, but this should prevent curators creating bad situations | ||
until a long-term solution is implemented (Replacing CC pairs/Connectors with Connections) | ||
""" | ||
if user and user.role != UserRole.ADMIN: | ||
if connector_data.is_public: | ||
raise HTTPException( | ||
status_code=400, | ||
detail="Public connectors can only be created by admins", | ||
) | ||
if not connector_data.groups: | ||
raise HTTPException( | ||
status_code=400, | ||
detail="Connectors created by curators must have groups", | ||
) | ||
return ConnectorBase( | ||
name=connector_data.name, | ||
source=connector_data.source, | ||
input_type=connector_data.input_type, | ||
connector_specific_config=connector_data.connector_specific_config, | ||
refresh_freq=connector_data.refresh_freq, | ||
prune_freq=connector_data.prune_freq, | ||
indexing_start=connector_data.indexing_start, | ||
) | ||
|
||
|
||
@router.post("/admin/connector") | ||
def create_connector_from_model( | ||
connector_data: ConnectorUpdateRequest, | ||
|
@@ -551,13 +522,19 @@ def create_connector_from_model( | |
) -> ObjectCreationIdResponse: | ||
try: | ||
_validate_connector_allowed(connector_data.source) | ||
connector_base = _check_connector_permissions(connector_data, user) | ||
|
||
validate_user_creation_permissions( | ||
db_session=db_session, | ||
user=user, | ||
target_group_ids=connector_data.groups, | ||
object_is_public=connector_data.is_public, | ||
) | ||
connector_base = connector_data.to_connector_base() | ||
return create_connector( | ||
db_session=db_session, | ||
connector_data=connector_base, | ||
) | ||
except ValueError as e: | ||
logger.error(f"Error creating connector: {e}") | ||
raise HTTPException(status_code=400, detail=str(e)) | ||
|
||
|
||
|
@@ -608,12 +585,18 @@ def create_connector_with_mock_credential( | |
def update_connector_from_model( | ||
connector_id: int, | ||
connector_data: ConnectorUpdateRequest, | ||
user: User = Depends(current_admin_user), | ||
user: User = Depends(current_curator_or_admin_user), | ||
db_session: Session = Depends(get_session), | ||
) -> ConnectorSnapshot | StatusResponse[int]: | ||
try: | ||
_validate_connector_allowed(connector_data.source) | ||
connector_base = _check_connector_permissions(connector_data, user) | ||
validate_user_creation_permissions( | ||
db_session=db_session, | ||
user=user, | ||
target_group_ids=connector_data.groups, | ||
object_is_public=connector_data.is_public, | ||
) | ||
connector_base = connector_data.to_connector_base() | ||
except ValueError as e: | ||
raise HTTPException(status_code=400, detail=str(e)) | ||
|
||
|
@@ -643,7 +626,7 @@ def update_connector_from_model( | |
@router.delete("/admin/connector/{connector_id}", response_model=StatusResponse[int]) | ||
def delete_connector_by_id( | ||
connector_id: int, | ||
_: User = Depends(current_admin_user), | ||
_: User = Depends(current_curator_or_admin_user), | ||
db_session: Session = Depends(get_session), | ||
) -> StatusResponse[int]: | ||
try: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,9 +48,12 @@ class ConnectorBase(BaseModel): | |
|
||
|
||
class ConnectorUpdateRequest(ConnectorBase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should test this somehow maybe |
||
is_public: bool | None = None | ||
is_public: bool = True | ||
groups: list[int] = Field(default_factory=list) | ||
|
||
def to_connector_base(self) -> ConnectorBase: | ||
return ConnectorBase(**self.model_dump(exclude={"is_public", "groups"})) | ||
|
||
|
||
class ConnectorSnapshot(ConnectorBase): | ||
id: int | ||
|
@@ -103,11 +106,6 @@ class CredentialSnapshot(CredentialBase): | |
user_id: UUID | None | ||
time_created: datetime | ||
time_updated: datetime | ||
name: str | None | ||
source: DocumentSource | ||
credential_json: dict[str, Any] | ||
admin_public: bool | ||
curator_public: bool | ||
|
||
@classmethod | ||
def from_credential_db_model(cls, credential: Credential) -> "CredentialSnapshot": | ||
|
@@ -261,6 +259,10 @@ class ConnectorCredentialPairMetadata(BaseModel): | |
groups: list[int] = Field(default_factory=list) | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ??? |
||
class CCStatusUpdateRequest(BaseModel): | ||
status: ConnectorCredentialPairStatus | ||
|
||
|
||
class ConnectorCredentialPairDescriptor(BaseModel): | ||
id: int | ||
name: str | None = None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to another file